Bug 188890 - Populate "text/uri-list" with multiple URLs when the pasteboard contains multiple URLs
Summary: Populate "text/uri-list" with multiple URLs when the pasteboard contains mult...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-23 10:36 PDT by Wenson Hsieh
Modified: 2018-09-05 12:25 PDT (History)
15 users (show)

See Also:


Attachments
First pass (41.92 KB, patch)
2018-09-01 22:04 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (42.37 KB, patch)
2018-09-02 21:27 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (42.72 KB, patch)
2018-09-03 11:25 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch for EWS (42.57 KB, patch)
2018-09-04 15:25 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2018-08-23 10:36:53 PDT
SSIA
Comment 1 Radar WebKit Bug Importer 2018-08-23 10:37:23 PDT
<rdar://problem/43648605>
Comment 2 Wenson Hsieh 2018-09-01 22:04:49 PDT
Created attachment 348725 [details]
First pass
Comment 3 Darin Adler 2018-09-01 22:20:42 PDT
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.
Comment 4 Wenson Hsieh 2018-09-02 15:43:22 PDT
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!
Comment 5 Wenson Hsieh 2018-09-02 21:27:43 PDT
Created attachment 348756 [details]
Patch
Comment 6 Sam Weinig 2018-09-02 21:46:49 PDT
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)?
Comment 7 Wenson Hsieh 2018-09-03 10:45:59 PDT
(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
Comment 8 Wenson Hsieh 2018-09-03 11:25:28 PDT
Created attachment 348783 [details]
Patch
Comment 9 Tim Horton 2018-09-04 12:27:15 PDT
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?
Comment 10 Wenson Hsieh 2018-09-04 14:38:54 PDT
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").
Comment 11 Wenson Hsieh 2018-09-04 15:25:42 PDT
Created attachment 348856 [details]
Patch for EWS
Comment 12 WebKit Commit Bot 2018-09-04 16:48:02 PDT
Comment on attachment 348856 [details]
Patch for EWS

Clearing flags on attachment: 348856

Committed r235647: <https://trac.webkit.org/changeset/235647>
Comment 13 WebKit Commit Bot 2018-09-04 16:48:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Truitt Savell 2018-09-05 08:11:28 PDT
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/
Comment 15 Ryan Haddad 2018-09-05 10:20:28 PDT
(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?
Comment 16 Wenson Hsieh 2018-09-05 12:25:59 PDT
(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.