[webkit-reviews] review granted: [Bug 172526] Drag event DataTransfer has unexpected types "dyn.ah62d4..." : [Attachment 321805] Remove commitStaticPasteboardToPasteboard (2)

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 172526: Drag event DataTransfer has unexpected types "dyn.ah62d4..."
https://bugs.webkit.org/show_bug.cgi?id=172526

Attachment 321805: Remove commitStaticPasteboardToPasteboard (2)

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




--- Comment #31 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 321805
  --> https://bugs.webkit.org/attachment.cgi?id=321805
Remove commitStaticPasteboardToPasteboard (2)

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

> Source/WebCore/platform/mac/PasteboardMac.mm:555
> +    // For compatibility on Mac, we need to additionally supply the 'Files'
pasteboard type to indicate
> +    // that files are present on the pasteboard. This is only on the
pasteboard reading side (i.e. there
> +    // is no mapping from the web-exposed 'Files' type to a platform Cocoa
type) because web content
> +    // shouldn't be able to write file URLs to the pasteboard using the
DataTransfer's setData API in the
> +    // first place. Since DataTransfer.getData('Files') should never return
an actual file URL, this is
> +    // effectively just a flag on DataTransfer that web content uses to
determine that DataTrasfer.files
> +    // contains a list of File objects.

I don't think we need this comment since this is a spec'ed behavior.

> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:95
> +	   // If we were unable to initialize a URL using -URLFromPasteboard:,
fall back to reading the string data and create a new NSURL with it.

This comment just repeats the code. Please remove.

> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:107
> +    // First, include all dynamically inserted types.

I don't think we need this comment.

> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:115
> +    // Then, add all web exposed types, coercing platform types into
web-exposed types as needed.

Or this comment.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/dump-datatransfer-types.html:66
> +    "baz/uri-list" : "https://www.webkit.org"

You should have types with emoji, CJK characters, & unicode control characters
to make sure they don't go haywire.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/dump-datatransfer-types.html:71
> +    let pasteboard = event.dataTransfer || event.clipboardData;
> +    let eventData = {};

const?

> LayoutTests/editing/pasteboard/data-transfer-get-data-on-drop-custom.html:36
> +    let eventData = {};

const?

>
LayoutTests/editing/pasteboard/data-transfer-get-data-on-paste-plain-text.html:
37
> +    let eventData = {};
> +    for (let type of event.clipboardData.types)

const?

>
LayoutTests/editing/pasteboard/data-transfer-get-data-on-paste-rich-text.html:3
7
> +    let eventData = {};
> +    for (let type of event.clipboardData.types)

const?