[webkit-reviews] review granted: [Bug 76598] [chromium] Refactor ClipboardChromium and DataTransferItemList/DataTransferItem to support HTML spec : [Attachment 132139] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Daniel Cheng <dcheng at chromium.org>'s
request for review:
Bug 76598: [chromium] Refactor ClipboardChromium and
DataTransferItemList/DataTransferItem to support HTML spec
https://bugs.webkit.org/show_bug.cgi?id=76598

Attachment 132139: Patch
https://bugs.webkit.org/attachment.cgi?id=132139&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=132139&action=review


I'm really only confident that I've checked the coding style.  The patch is too
big for me to know if it's correct or not.  I think it's fine to land, but we
should try to cover more test types in the test (e.g., files, text/html, etc)

> Source/WebCore/platform/chromium/ChromiumDataObject.h:66
> +    void urlAndTitle(String& url, String* title = 0) const;

If all callers provide title, we can remove the default param (I would just
make it pass by reference so it's more like htmlAndBaseURL).

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:82
> +DataTransferItemChromium::DataTransferItemChromium(const String& kind, const
String& type, const String& data,
> +    PassRefPtr<File> file, PassRefPtr<SharedBuffer> sharedBuffer, const
String& title, const KURL& baseURL)

Given the simplicity of this class and that all the methods are const, I'm not
worried that the members are const or not.  When introducing a new type, I
would find "adoptRef(new DataTransferItemChromium(DataTransferItem::kindFile,
String(), String(), 0, buffer, name, KURL()));" harder to understand and more
error prone than accidentally mutating a member variable.

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:119
> +	   ASSERT(m_sharedBuffer);
> +	   return 0; // FIXME: Support this?

Please make the comment more clear.  E.g., FIXME: Support converting a shared
buffer into a file.

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:165
> +    return
PlatformSupport::clipboardSequenceNumber(currentPasteboardBuffer()) ==
m_sequenceNumber ? data : String();

Is there a test covering this bug fix?	Hiding a bug fix in an 80k patch is
likely to regress in the future.  I would be better to fix the bug in a
separate patch.

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:171
> +    // FIXME: When we properly support File dragout, we'll need to make sure

> +    // this works as expected for DragDataChromium.

Please include a link to the bug you created here.