Code review comment for lp:~abreu-alexandre/oxide/clipboard

Thanks for working on this, this is probably the most-awaited feature ever :)
Here are a few comments/remarks from a first round of review:

== unit tests ==

We’re missing unit tests for (at least) HTML markup.

Indentation in qt/tests/qmltests/core/tst_Clipboard.* is inconsistent in places.

Why does clearClipboard() accept an (unused) 'data' parameter?

Is getClipboardImageData() really necessary? Can’t the test data be defined in tst_Clipboard.qml directly (and add a 'data' parameter to copyImageToClipboard())?
In fact, can’t we get rid entirely of copyImageToClipboard(), and make copyToClipboard() more generic by checking the mimeType to decide whether the data is base64-encoded or not?

In test_paste_data(), the 'isimage' parameter seems redundant, you could just verify whether the mimeType starts with "image/".

copyFromClipboard() would be better named getFromClipboard() or getClipboardContent()

> The image we get from QImage and the one we pasted are slightly
> different but overall is the same image. QImage does some "processing"
> on the raw image content that slightly alters it and make it hard to
> have an exact match.
How is that even possible? We’re talking about an uncompressed, lossless PNG, right?
And I don’t see where QImage is involved in test_paste(), only raw data is passed around as QByteArrays.
By the way, can’t we make the test image data smaller? A 1×1 PNG would suffice for testing purposes.

test_copy() seems to be text-specific, so I don’t really see the point of having it being data-driven.

== implementation ==

There is a leftover DLOG in shared/port/ui_base/clipboard_oxide.cc.

Includes are not properly sorted alphabetically in shared/port/ui_base/clipboard_oxide.cc.

In qt/core/browser/oxide_qt_clipboard.cc, Qt includes are not sorted alphabetically. Also QDebug is included but doesn’t appear to be used. Shouldn’t it be used instead of LOG() macros?

In ClipboardQt’s method implementations, isn’t the DCHECK(clipboard) redundant, given that the case where clipboard is null is explictly handled?

Same for a DCHECK(data) in ClipboardQt::IsFormatAvailable(…).

In ClipboardQt::ReadAvailableTypes():
  - instead of a if() followed by a NOTREACHED, wouldn’t a DCHECK be more appropriate? We don’t expect the caller to pass in null parameters anyway, do we?
  - why not push types directly to the passed vector, instead of using an temporary vector and swapping its contents afterwards?
  - shouldn’t contains_filenames be set accordingly if one of the formats is "chromium/filename" ?

The indentation of the definition of ClipboardQt::ReadAsciiText() is incorrect.

It looks like the following block is repeated many times, maybe it could be factored out as a macro?
  const QMimeData *data =
    clipboard->mimeData(
       type == ui::CLIPBOARD_TYPE_COPY_PASTE ?
       QClipboard::Clipboard
       : QClipboard::Selection);

In ClipboardQt::ReadHTML():
  - please use curly brackets to surround the contents of an if() block, even if it’s a one-liner.
  - do we really need to clear markup and src_url prior to setting their content?

In ClipboardQt::ReadImage():
  - I don’t understand the comment about ReadImage. Can you clarify it?
  - what if the format of the image is not ARGB32? Couldn’t we gracefully convert it to the right format with QImage::convertToFormat() ?

It seems ClipboardQt::EnsureAccumulatorMimeDataExists() is called only in one place. Does it really need a separate method?

In ClipboardQt::WriteHTML(), what prevents us from implementing the resolution of relative links?

In ClipboardQt::WriteBitmap(), can you implement the image format validation and/or conversion if relevant?