[webkit-reviews] review granted: [Bug 175639] Add the support for mutating clipboard data via DataTransferItemList : [Attachment 318293] Fixed builds

[bugzilla-daemon at webkit.org]

Wenson Hsieh <wenson_hsieh at apple.com> has granted Ryosuke Niwa
<rniwa at webkit.org>'s request for review:
Bug 175639: Add the support for mutating clipboard data via
DataTransferItemList
https://bugs.webkit.org/show_bug.cgi?id=175639

Attachment 318293: Fixed builds

https://bugs.webkit.org/attachment.cgi?id=318293&action=review




--- Comment #3 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 318293
  --> https://bugs.webkit.org/attachment.cgi?id=318293
Fixed builds

View in context: https://bugs.webkit.org/attachment.cgi?id=318293&action=review

This looks reasonable to be, but I'm a bit confused about one part. For
instance, if you call items.add("WebKit1", "text/plain") and then
items.add("WebKit2", "text/plain"), it is expected that the item list contains
two items, and dataTransfer.getData("text/plain") would return "WebKit2" since
the second add() call overwrote the first? Maybe we should include the case
where we add more than one item at a time in the new test case.

> Source/WebCore/ChangeLog:16
> +	   Because the identify and the order of DataTransferItem's need to be
preserved, we can't simply

I don't think there should be an apostrophe after DataTransferItem.

> Source/WebCore/ChangeLog:17
> +	   re-popluate m_itemList in DataTransferItemList. Instead, whenever
the clipboard content is mutated,

Just to make sure -- it doesn't seem like this handles something modifying the
NSPasteboard independently of the page, correct?

> Source/WebCore/dom/DataTransferItem.cpp:81
> +    bool isInDisabledMode = !m_list;

IMO, this would look clearer if we folded the places where we check !m_list
into a helper method on DataTransferItem called isDisabled() or
isInDisabledMode().

> LayoutTests/editing/pasteboard/datatransfer-items-copy-plaintext.html:7
> +description('This tests copying plain text using dataTransfer.items. To
manually test, click on "Copy text" and paste (Command+V on Mac Control+V
elsewhere).');

As an aside: this patch should mean we also support adding plain text to the
pasteboard when copying an image or link by tapping "Copy" on the share sheet
in iOS. Let's remember to write a LayoutTest or API test for this!