Bug 188890 – Populate "text/uri-list" with multiple URLs when the pasteboard contains multiple URLs

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Wenson Hsieh]

SSIA

[Radar WebKit Bug Importer]

<rdar://problem/43648605>

[Wenson Hsieh]

Created attachment 348725 [details]
First pass

[Darin Adler]

Comment on attachment 348725 [details]
First pass

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

Did not finish reviewing but a couple initial comments

> Source/WebCore/dom/DataTransfer.cpp:153
> +            urlList.append(&newlineCharacter, 1);

StringBuilder has an append function that takes a single character. This can just be:

    uriList.append('\n');

or if you think it’s clearer you can include CharacterNames.h as you have done above and do:

    uriList.append(newlineCharacter);

> Source/WebCore/platform/ios/PasteboardIOS.mm:391
> +            if (NSURL *url = [NSURL URLWithString:value])
> +                values.append(url.absoluteString);

We should consider whether we want to use NSURL to do this conversion or WebCore::URL. I am not sure that it’s better to use NSURL. A round trip from string to NSURL and then back to string passing through the -[NSURL URLWithString:] method is almost certainly not exactly what we want. I am not sure that this new way of doing it is exactly the same as the code we had here before.

I’m concerned that the URL code isn’t quite right for URLs with non-ASCII characters in them and other tricky edge cases.

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:614
> +    Vector<String> strings;
> +    for (int index = 0; index < count(); ++index) {
> +        String value = readString(index, type);
> +        if (!value.isEmpty())
> +            strings.append(WTFMove(value));
> +    }
> +    return strings;

Using reserveInitialCapacity and uncheckedAppend rather than just append is slightly more efficient for both performance and memory use. The small bit of better memory use could matter if the result of this function is kept around.

[Wenson Hsieh]

Comment on attachment 348725 [details]
First pass

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

>> Source/WebCore/dom/DataTransfer.cpp:153
>> +            urlList.append(&newlineCharacter, 1);
> 
> StringBuilder has an append function that takes a single character. This can just be:
> 
>     uriList.append('\n');
> 
> or if you think it’s clearer you can include CharacterNames.h as you have done above and do:
> 
>     uriList.append(newlineCharacter);

Done!

>> Source/WebCore/platform/ios/PasteboardIOS.mm:391
>> +                values.append(url.absoluteString);
> 
> We should consider whether we want to use NSURL to do this conversion or WebCore::URL. I am not sure that it’s better to use NSURL. A round trip from string to NSURL and then back to string passing through the -[NSURL URLWithString:] method is almost certainly not exactly what we want. I am not sure that this new way of doing it is exactly the same as the code we had here before.
> 
> I’m concerned that the URL code isn’t quite right for URLs with non-ASCII characters in them and other tricky edge cases.

Good point. IIUC, it seems we just want to use -[NSURL absoluteString] here directly, instead of converting from string to NSURL using -URLWithString: in the process.

In this case, we shouldn't even need this extra bit of logic here to convert to NSURL and back, as long as PlatformPasteboard hands back absolute URL strings.

>> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:614
>> +    return strings;
> 
> Using reserveInitialCapacity and uncheckedAppend rather than just append is slightly more efficient for both performance and memory use. The small bit of better memory use could matter if the result of this function is kept around.

Done!

[Wenson Hsieh]

Created attachment 348756 [details]
Patch

[Sam Weinig]

Is it worth having a test of what gets put on the pasteboard when a page sets multiple URLs on the pasteboard using text/uri-list (e.g. via DataTransfer.setData in a copy event)?

[Wenson Hsieh]

(In reply to Sam Weinig from comment #6)
> Is it worth having a test of what gets put on the pasteboard when a page
> sets multiple URLs on the pasteboard using text/uri-list (e.g. via
> DataTransfer.setData in a copy event)?

I think it's something we want to support, and so it's certainly something worth testing! However, it's outside the scope of this patch, which only deals with handling a drop with multiple URLs rather than being able to write multiple URLs on drag.

I've filed a bug/ER on this: https://bugs.webkit.org/show_bug.cgi?id=189246

[Wenson Hsieh]

Created attachment 348783 [details]
Patch

[Tim Horton]

Comment on attachment 348783 [details]
Patch

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

> Source/WebCore/ChangeLog:74
> +        where the NSURL is constructed from absolute and relative parts using a Plist. While -absoluteString gets us the
> +        full URL string, URL::string() only returns the relative portion.

Surely there's a way to get the absolute string from a WebCore::URL?

> Source/WebCore/platform/ios/PasteboardIOS.mm:385
> +    bool isPlainTextType = [cocoaType isEqualToString:(NSString *)kUTTypePlainText];

Sometimes you __bridge, sometimes you don't?

> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:136
> +    if (NSURL *urlFromPasteboard = [NSURL URLWithString:[pasteboard stringForType:legacyURLPasteboardType()]])

If we already have a string, why do we bounce through NSURL?

> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:166
> +    if (typeIdentifier == String(kUTTypeURL)) {

Should this be case-insensitive?

[Wenson Hsieh]

Comment on attachment 348783 [details]
Patch

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

>> Source/WebCore/ChangeLog:74
>> +        full URL string, URL::string() only returns the relative portion.
> 
> Surely there's a way to get the absolute string from a WebCore::URL?

Chatted with Alex about this — in this case where the NSURL is specified with relative/base components, the base is lost when converting to WebCore::URL altogether, so WebCore::URL will no longer know the absolute string.

>> Source/WebCore/platform/ios/PasteboardIOS.mm:385
>> +    bool isPlainTextType = [cocoaType isEqualToString:(NSString *)kUTTypePlainText];
> 
> Sometimes you __bridge, sometimes you don't?

Oops! This should've been bridged, since it's going from CFStringRef => NSString. Added the 🌉

>> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:136
>> +    if (NSURL *urlFromPasteboard = [NSURL URLWithString:[pasteboard stringForType:legacyURLPasteboardType()]])
> 
> If we already have a string, why do we bounce through NSURL?

True, we should just be able to return the string. I'll refactor this helper function a bit to remove this extra NSURL conversion.

>> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:166
>> +    if (typeIdentifier == String(kUTTypeURL)) {
> 
> Should this be case-insensitive?

I think this should be okay as a case-sensitive comparison, since we only use the CoreServices constants provided by the platform when mapping MIME types to UTIs or legacy pasteboard types to UTIs. There shouldn't be any chance for us to get here with a version of a UTI with some differences in case (e.g. "Public.URL").

[Wenson Hsieh]

Created attachment 348856 [details]
Patch for EWS

[WebKit Commit Bot]

Comment on attachment 348856 [details]
Patch for EWS

Clearing flags on attachment: 348856

Committed r235647: <https://trac.webkit.org/changeset/235647>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

Looks like https://trac.webkit.org/changeset/235647

Caused this API test TestWebKitAPI.DragAndDropTests.ExposeMultipleURLsInDataTransfer

to fail on all Mac

stdio:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/6562/steps/run-api-tests/logs/stdio

Failed

    TestWebKitAPI.DragAndDropTests.ExposeMultipleURLsInDataTransfer
        
        /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/DragAndDropTests.mm:71
        Expected equality of these values:
          "text/plain, text/uri-list"
          [webView stringByEvaluatingJavaScript:@"types.textContent"]
            Which is: "text/plain, public.url, CorePasteboardFlavorType 0x75726C20, dyn.ah62d4rv4gu8yc6durvwwaznwmuuha2pxsvw0e55bsmwca7d3sbwu, text/uri-list"
        
        
        /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/DragAndDropTests.mm:74
        Expected equality of these values:
          "https://webkit.org/\nhttps://apple.com/"
          [webView stringByEvaluatingJavaScript:@"urlData.textContent"]
            Which is: "https://webkit.org/"
        With diff:
        @@ -1,2 @@
         https://webkit.org/
        -https://apple.com/

[Ryan Haddad]

(In reply to Truitt Savell from comment #14)
> Looks like https://trac.webkit.org/changeset/235647
> 
> Caused this API test
> TestWebKitAPI.DragAndDropTests.ExposeMultipleURLsInDataTransfer
> 
> to fail on all Mac
The failing test was added with this change.

Wenson, are you able to take a look at this or should we roll out the change?

[Wenson Hsieh]

(In reply to Ryan Haddad from comment #15)
> (In reply to Truitt Savell from comment #14)
> > Looks like https://trac.webkit.org/changeset/235647
> > 
> > Caused this API test
> > TestWebKitAPI.DragAndDropTests.ExposeMultipleURLsInDataTransfer
> > 
> > to fail on all Mac
> The failing test was added with this change.
> 
> Wenson, are you able to take a look at this or should we roll out the change?

Sorry for the delay — I'll take a look momentarily.