[webkit-reviews] review granted: [Bug 178301] On ToT, event.dataTransfer.getData("text/uri-list") returns an empty string when dragging an image : [Attachment 323842] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 178301: On ToT, event.dataTransfer.getData("text/uri-list") returns an
empty string when dragging an image
https://bugs.webkit.org/show_bug.cgi?id=178301

Attachment 323842: Patch

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




--- Comment #21 from Darin Adler <darin at apple.com> ---
Comment on attachment 323842
  --> https://bugs.webkit.org/attachment.cgi?id=323842
Patch

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

> Source/WebCore/platform/Pasteboard.h:192
>      static bool isSafeTypeForDOMToReadAndWrite(const String&);
> +    static bool canExposeURLToDOMWhenPasteboardContainsFiles(const String&);

I don’t love the word "DOM" here to mean "what we expose to the web in
JavaScript bindings" even though it is the term of art. Partly, just like URL,
it just doesn’t flow well in these styles of function names since it’s an
acronym. Don’t currently have a better idea.

The older function uses the terminology "is safe", and the newer function use
the terminology "can" to mean the same thing.

But despite those thoughts I don’t have new function names to propose at this
time.

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:128
> +    RetainPtr<id> value = [m_pasteboard valuesForPasteboardType:type
inItemSet:[NSIndexSet indexSetWithIndex:0]].firstObject;

If we used the retainPtr function here:

    auto value = retainPtr(xxx);

we might get a more specific type than RetainPtr<id>, which would be nice for
passing to pasteboardMayContainFilePaths; the compiler doesn’t currently
complain about passing id to a more specific type and maybe it never will, but
still I think it might be better.

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:137
> +    if ([value isKindOfClass:[NSURL class]])
> +	   result = [(NSURL *)value absoluteString];
> +
> +    if ([value isKindOfClass:[NSAttributedString class]])
> +	   result = [(NSAttributedString *)value string];
> +
> +    if ([value isKindOfClass:[NSString class]])
> +	   result = (NSString *)value;

Maybe chain these with else if?

> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:92
> +    if (pasteboardType == String(NSURLPboardType)) {

Above you used the format String { kUTTypeURL }, but here it’s the old style.

> LayoutTests/editing/pasteboard/files-during-page-drags.html:84
> +	   testFailed("This test is not interactive, please run using
DumpRenderTree");

DumpRenderTree is the old one, WebKItTestRunner is the new one. If we are just
mentioning one, than I suggest it be the new one.