Bug 170449 – Image pasting is not working on tineye.com / gmail.com / GitHub.com due to lack of support for DataTransfer.items

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Ebrahim Byagowi]

(It seems Bug 49141 is not completely fixed pasting issue as my test on r214532 (Version 10.1.1 (12603.2.1, r214532), or perhaps is a different issue?, anyway)

Steps to repro:
1. Copy an image, or Command+Ctrl+Shift+4 to take screenshot of part of your screen which actually copies it into clipboard
2. Open https://tineye.com/ and Command+V

Excepted:
Like Chrome, image be pasted and searched with tineye (the issue is not limited to tineye)

Actual:
Nothing happens

[Alfonso Martínez de Lizarrondo]

Bug #49141 is fixed and it works as designed in the latest released Safari Version 10.1 (12603.1.30.0.34)

Now the issue at hand is that it has been done in a new way that matches no other browser, so you have to wait until the developers of every library updates their code to handle the new Safari, and then those new versions are deployed through every website in order to be able to use Safari.

[Ebrahim Byagowi]

(In reply to Alfonso Martínez de Lizarrondo from comment #1)
> Bug #49141 is fixed and it works as designed in the latest released Safari
> Version 10.1 (12603.1.30.0.34)

Yes, I can also confirm that using the testcase available on #12. Thanks

> Now the issue at hand is that it has been done in a new way that matches no
> other browser, so you have to wait until the developers of every library
> updates their code to handle the new Safari, and then those new versions are
> deployed through every website in order to be able to use Safari.

Is there any sample page on how that is done which would works on the steps 1 I mentioned here? A stripped down working code or documentation would be nice also so I could use it on my apps also. I hope you could support the old way most sites support which would make web users more happy.

[Chris Dumez]

@Ryosuke: Are you still opposed to using data URLs instead of Blob URLs? Even though Blob URLs have benefits here, it is unfortunate that we do it differently than other browsers.

[Chris Dumez]

Actually, tineye.com does not work in WebKit, even if I switch to using Data URLs. The reason is that the page's JavaScript relies on DataTransfer.items being implemented and it is not in WebKit.

JS:
_onPaste: function(b) {
            var c = b.originalEvent && b.originalEvent.clipboardData && b.originalEvent.clipboardData.items,
                d = {
                    files: []
                };
            c && c.length && (a.each(c, function(a, b) {
                var c = b.getAsFile && b.getAsFile();
                c && d.files.push(c)
            }), this._trigger("paste", a.Event("paste", {
                delegatedEvent: b
            }), d) !== !1 && this._onAdd(b, d))
        },


b.originalEvent.clipboardData.items is undefined in WebKit. So the tineye.com issue has nothing to do with the approach we chose in Bug 49141.

[Chris Dumez]

(In reply to Alfonso Martínez de Lizarrondo from comment #1)
> Bug #49141 is fixed and it works as designed in the latest released Safari
> Version 10.1 (12603.1.30.0.34)
> 
> Now the issue at hand is that it has been done in a new way that matches no
> other browser, so you have to wait until the developers of every library
> updates their code to handle the new Safari, and then those new versions are
> deployed through every website in order to be able to use Safari.

If you have example of sites that fail in Safari but work in Chrome because we are using Blob URLs instead of Data URLs, please let me know. It seems tineye.com is not representative of such case.

[Alfonso Martínez de Lizarrondo]

data: urls are legacy behavior that was originally implemented in Firefox.
Chrome never used them and they have always used DataTransfer.items

Currently Firefox also uses DataTransfer.items, although a good script that reads pasted files should try to handle both schemes because data: urls can be generated also by LibreOffice and Edge for example (I mean: tineye is not a good example, or maybe they're using UA sniffing and don't expect Webkit to paste base64)

Safari should have implemented .items instead of that weird way to read a Blob

[Ebrahim Byagowi]

Hey guys, just one more comment and I leave this to you :) I've also issue with Gmail mail compose and Gmail integrated chat, this is its steps to reproduce:

0. Open gmail.com with a gmail account and open new email (Compose)
1. Try to paste a copied image.

This works on browsers other than Safari so I have to switch to another browser just for this. Perhaps that also can be considered for the fix. Thanks

[Ebrahim Byagowi]

Hard to sniff GWT compiled JS of gmail but it seems a similar code with tineye available on their code: (but multiple places I see with similar code on Gmail)

        var b = ka("clipboardData.items", a.fe);
        b && (a = rc(b, function(a) {
            return !!a && qb(a.type, "text/html")
        }), b = rc(b, function(a) {
            return !!a && qb(a.type, "image")
        }), 1 == b.length && this.JI(b[0].getAsFile()), 1 == b.length && 1 == a.length && this.CJ(a[0]))

[Chris Dumez]

(In reply to ebraminio from comment #7)
> Hey guys, just one more comment and I leave this to you :) I've also issue
> with Gmail mail compose and Gmail integrated chat, this is its steps to
> reproduce:
> 
> 0. Open gmail.com with a gmail account and open new email (Compose)
> 1. Try to paste a copied image.
> 
> This works on browsers other than Safari so I have to switch to another
> browser just for this. Perhaps that also can be considered for the fix.
> Thanks

Using data URLs does not help the Gmail case either. Looking at the Gmail's JS, it seems they rely on DataTransfer.items as well.

[Radar WebKit Bug Importer]

<rdar://problem/31432525>

[Ebrahim Byagowi]

Also GitHub, try to paste an image when you want file a bug or comment on a bug,

    function H(e) {
        if (e.clipboardData && e.clipboardData.items) {
            var t = Array.from(e.clipboardData.items).map(function(e) {
                return [e, _(e.type)]
    ...

These are really my everyday use websites that are forcing me to use another browser just for copy pasting an image.

[Ebrahim Byagowi]

May I request to know the current working screenshot pasting way other than perhaps using something like hidden contenteditable? I was implementing this on my personal project http://pad.js.org and even on nightly WebKit I couldn't find a way to make this happen because as the current lack of .Items on DataTransfer, other possible solution like DataTransfer.files and DataTransfer.getData("Image/png") also didn't provide the image Blob but as Bug #49141 there should be someway for this I guess? It seems this a question for web community also https://github.com/layerssss/paste.js/issues/21

[Dan]

Hi, it's not at all clear for me how the new Safari handles pasting images. I can't seem to be able to extract the image from event.clipboardData (on a "paste" event). Could you point me in the right direction?

[Ryosuke Niwa]

Created attachment 321620 [details]
WIP patch

With this patch, I can copy & paste image from Finder into Github & tineye.com. Generic copy & paste of images don't work because I'm not doing TIFF to PNG conversion yet.

[Ryosuke Niwa]

Created attachment 321774 [details]
WIP: Fully functional on Mac

[Ryosuke Niwa]

Created attachment 321788 [details]
WTP: Cleaned up - still needs iOS fix

[Build Bot]

Attachment 321788 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/PasteboardMac.mm:506:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 3 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 321937 [details]
WIP - now with tests on Mac

[Build Bot]

Attachment 321937 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mac/PasteboardMac.mm:489:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/mac/PasteboardMac.mm:490:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/mac/PasteboardMac.mm:491:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/mac/PasteboardMac.mm:492:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 4 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 322074 [details]
WIP - updated after r222595

[Ryosuke Niwa]

Created attachment 322142 [details]
Adds the feature

[Build Bot]

Attachment 322142 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/cocoa/PasteboardCocoa.mm:54:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/cocoa/PasteboardCocoa.mm:55:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/cocoa/PasteboardCocoa.mm:56:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/cocoa/PasteboardCocoa.mm:57:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 4 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 322143 [details]
Adds the feature

[Build Bot]

Attachment 322143 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/cocoa/PasteboardCocoa.mm:54:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/cocoa/PasteboardCocoa.mm:55:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/cocoa/PasteboardCocoa.mm:56:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/cocoa/PasteboardCocoa.mm:57:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 4 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 322143 [details]
Adds the feature

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

I haven't fully read through this patch. I have some questions and noticed some very minor stylistic issues.

> Source/WebCore/ChangeLog:10
> +        Because there is no Web API to get binray data out of dataTransfer unlike text data, we need to treat any image

binray => binary

> Source/WebCore/ChangeLog:11
> +        data as files even if they're entirely in the memory.,

There is a trailing comma at the end of this line.

> Source/WebCore/ChangeLog:15
> +        as well as dataTransfer.items of type "file". Because in-memory images are stored as TIFF in macOS and websites
> +        don't typically support image/tiff, we convert all such in-memory TIFF images into PNG images ourselves for

Is this what other browsers do?

> Source/WebCore/ChangeLog:16
> +        a better Web compatibility. This is done inside read(PasteboardFileReader&) in PasteboardCocoa.mm.

Web => web

> Source/WebCore/ChangeLog:20
> +        MIME types. This was unwarranted since 1. we don't have any restriction on what an user can drag & drop into a web
> +        page via input element so there is no security benefit in this, and 2. the user should be able to copy & paste

What do other browser do? Does this match the spec?

> Source/WebCore/ChangeLog:49
> +        (WebCore::imageTypeToMimeType): Added. Pretends to have image/png when the cocoa type is image/tiff since most of

cocoa => Cocoa

(it's a proper noun)

> Source/WebCore/ChangeLog:56
> +        (WebCore::Pasteboard::read): Added. On macOS, we convert TIFF to PNG for Web compatibility. On iOS, most of apps like

Web => web

The last sentence does not read well.

> Source/WebCore/platform/Pasteboard.cpp:42
> +PasteboardImage::PasteboardImage()
> +{
> +}

PasteboardImage::PasteboardImage() = default;

> Source/WebCore/platform/Pasteboard.cpp:46
> +PasteboardImage::~PasteboardImage()
> +{
> +}

PasteboardImage::~PasteboardImage() = default;

> Source/WebCore/platform/Pasteboard.cpp:50
> +PasteboardFileReader::PasteboardFileReader()
> +{
> +}

PasteboardFileReader::PasteboardFileReader() = default;

> Source/WebCore/platform/Pasteboard.cpp:54
> +PasteboardFileReader::~PasteboardFileReader()
> +{
> +}

PasteboardFileReader::~PasteboardFileReader() = default;

> Source/WebCore/platform/Pasteboard.h:151
> +public:

Please remove this as it is unnecessary. By default all members of a struct have public visibility.

> Source/WebCore/platform/cocoa/PasteboardCocoa.mm:46
> +PasteboardWebContent::PasteboardWebContent()
> +{
> +}

PasteboardWebContent::PasteboardWebContent() = default;

> Source/WebCore/platform/cocoa/PasteboardCocoa.mm:50
> +PasteboardWebContent::~PasteboardWebContent()
> +{
> +}

PasteboardWebContent::~PasteboardWebContent() = default;

> Source/WebCore/platform/cocoa/PasteboardCocoa.mm:57
> +    GIF

For you consideration, I suggest adding a trailing comma after this enumerator. The benefit is that if a person appends an enumerator in the future then they will not need to take care to add a trailing comma to the end of this line.

> Source/WebCore/platform/cocoa/PasteboardCocoa.mm:63
> +    if (cocoaType == String(NSTIFFPboardType))

Is it necessary to explicitly call the String constructor? Same question for the other calls to the String constructor throughout this function.

> Source/WebCore/platform/cocoa/PasteboardCocoa.mm:147
> +        return Vector<String>();

return { };

> Source/WebCore/platform/mac/PasteboardMac.mm:540
> +        return; // skip this ancient type that gets auto-supplied by some system conversion

Please make this a proper sentence by capitalizing the 's' in "Skip" and adding a period at the end of this sentence.

> Source/WebCore/platform/mac/PasteboardMac.mm:-636
> +    for (size_t i = 0; i < types.size(); i++)
>          addHTMLClipboardTypesForCocoaType(result, types[i], m_pasteboardName);
> -    }

We are underutilizing the index variable i. I suggest we write this loop using a range-based for loop.

> Source/WebKit/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm:58
> +        for (size_t i = 0; i < pathnames.size(); i++) {

I suggest we write this using a range-based for-loop.

[Wenson Hsieh]

Comment on attachment 322143 [details]
Adds the feature

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

> Source/WebCore/platform/cocoa/PasteboardCocoa.mm:202
> +        data.append(buffer->data(), buffer->size());

I think it's a bit strange that on iOS, this line is indented even though the else is defined out. Can we just move the #if PLATFORM(MAC) inside the if statement, and let tiffWasTreatedAsPNGForWebCompatibility = false on iOS so that this append is at the right indentation level on iOS?

> Source/WebCore/platform/ios/PasteboardIOS.mm:424
> +Vector<String> Pasteboard::dataTypes()

Perhaps this could be dataTypesForBindings, to be consistent with readStringForBindings?

> Source/WebKit/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm:52
> +        auto pasteboard = PlatformPasteboard(pasteboardName);

I don't think the local variable here adds much.

>> Source/WebKit/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm:58
>> +        for (size_t i = 0; i < pathnames.size(); i++) {
> 
> I suggest we write this using a range-based for-loop.

...we do use i in this case though, for sandboxExtensions[i]

> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteImage.mm:54
> +    [[UIPasteboard generalPasteboard] setItems:@[[NSDictionary dictionaryWithObject:data forKey:type]]];

Probably not for this patch, but I'm a bit curious what happens on iOS with multiple images in the pasteboard (e.g. using something like [~ setItems:@[ @{ type1: data1 }, @{ type2: data2 } ]])

> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteImage.mm:60
> +    RetainPtr<TestWKWebView> webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 400, 400)]);

I would prefer auto for these, since declaring the type here doesn't add much information.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteImage.mm:81
> +    RetainPtr<TestWKWebView> webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 400, 400)]);

auto

> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteImage.mm:102
> +    RetainPtr<TestWKWebView> webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 400, 400)]);

auto

> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteImage.mm:109
> +    EXPECT_STREQ("false", [webView stringByEvaluatingJavaScript:@"dataTransfer.types.includes('image/gif').toString()"].UTF8String);

You can EXPECT_WK_STREQ here and avoid the .UTF8String.

Or perhaps even cleaner, EXPECT_FALSE([webView ~:].boolValue);

> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteImage.mm:130
> +    RetainPtr<TestWKWebView> webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 400, 400)]);

auto

> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteImage.mm:150
> +    RetainPtr<TestWKWebView> webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 400, 400)]);

auto

> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteImage.mm:170
> +    RetainPtr<TestWKWebView> webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 400, 400)]);

auto

> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteImage.mm:190
> +    RetainPtr<TestWKWebView> webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 400, 400)]);

auto

> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteImage.mm:210
> +    RetainPtr<TestWKWebView> webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 400, 400)]);

auto

> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteImage.mm:230
> +    RetainPtr<TestWKWebView> webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 400, 400)]);

auto

[Ryosuke Niwa]

(In reply to Daniel Bates from comment #26)
> Comment on attachment 322143 [details]
> Adds the feature
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322143&action=review
> 
> I haven't fully read through this patch. I have some questions and noticed
> some very minor stylistic issues.
> 
> > Source/WebCore/ChangeLog:10
> > +        Because there is no Web API to get binray data out of dataTransfer unlike text data, we need to treat any image
> 
> binray => binary

Fixed.

> > Source/WebCore/ChangeLog:11
> > +        data as files even if they're entirely in the memory.,
> 
> There is a trailing comma at the end of this line.

Fixed.

> > Source/WebCore/ChangeLog:15
> > +        as well as dataTransfer.items of type "file". Because in-memory images are stored as TIFF in macOS and websites
> > +        don't typically support image/tiff, we convert all such in-memory TIFF images into PNG images ourselves for
> 
> Is this what other browsers do?

They do this conversion regardless of whether PNG/GIF is available in the pasteboard or not, presumably to trip out the location information embedded in the file but we decided not to go with that approach.

> > Source/WebCore/ChangeLog:16
> > +        a better Web compatibility. This is done inside read(PasteboardFileReader&) in PasteboardCocoa.mm.
> 
> Web => web

Hm... I thought Web is always capitalized in this context but okay.

> 
> > Source/WebCore/ChangeLog:20
> > +        MIME types. This was unwarranted since 1. we don't have any restriction on what an user can drag & drop into a web
> > +        page via input element so there is no security benefit in this, and 2. the user should be able to copy & paste
> 
> What do other browser do? Does this match the spec?

There is no spec for this since this is a security/privacy mitigation each browser is implementing separately. Having said that, no other browser implements the same restriction I had there.

> 
> > Source/WebCore/ChangeLog:49
> > +        (WebCore::imageTypeToMimeType): Added. Pretends to have image/png when the cocoa type is image/tiff since most of
> 
> cocoa => Cocoa
> 
> (it's a proper noun)
> 
> > Source/WebCore/ChangeLog:56
> > +        (WebCore::Pasteboard::read): Added. On macOS, we convert TIFF to PNG for Web compatibility. On iOS, most of apps like
> 
> Web => web
> 
> The last sentence does not read well.
> 
> > Source/WebCore/platform/Pasteboard.cpp:42
> > +PasteboardImage::PasteboardImage()
> > +{
> > +}
> 
> PasteboardImage::PasteboardImage() = default;
> 
> > Source/WebCore/platform/Pasteboard.cpp:46
> > +PasteboardImage::~PasteboardImage()
> > +{
> > +}
> 
> PasteboardImage::~PasteboardImage() = default;
> 
> > Source/WebCore/platform/Pasteboard.cpp:50
> > +PasteboardFileReader::PasteboardFileReader()
> > +{
> > +}
> 
> PasteboardFileReader::PasteboardFileReader() = default;
> 
> > Source/WebCore/platform/Pasteboard.cpp:54
> > +PasteboardFileReader::~PasteboardFileReader()
> > +{
> > +}
> 
> PasteboardFileReader::~PasteboardFileReader() = default;

Fixed.

> 
> > Source/WebCore/platform/Pasteboard.h:151
> > +public:
> 
> Please remove this as it is unnecessary. By default all members of a struct
> have public visibility.

Removed.

> > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:46
> > +PasteboardWebContent::PasteboardWebContent()
> > +{
> > +}
> 
> PasteboardWebContent::PasteboardWebContent() = default;
> 
> > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:50
> > +PasteboardWebContent::~PasteboardWebContent()
> > +{
> > +}
> 
> PasteboardWebContent::~PasteboardWebContent() = default;

Fixed.

> > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:57
> > +    GIF
> 
> For you consideration, I suggest adding a trailing comma after this
> enumerator. The benefit is that if a person appends an enumerator in the
> future then they will not need to take care to add a trailing comma to the
> end of this line.

Fixed.

> > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:63
> > +    if (cocoaType == String(NSTIFFPboardType))
> 
> Is it necessary to explicitly call the String constructor? Same question for
> the other calls to the String constructor throughout this function.

We do. I got rid of it form "Apple PNG pasteboard type". But kUTTypeGIF, etc... needs an explicit conversion.

> > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:147
> > +        return Vector<String>();
> 
> return { };

Fixed.

> > Source/WebCore/platform/mac/PasteboardMac.mm:540
> > +        return; // skip this ancient type that gets auto-supplied by some system conversion
> 
> Please make this a proper sentence by capitalizing the 's' in "Skip" and
> adding a period at the end of this sentence.

Fixed.

> > Source/WebCore/platform/mac/PasteboardMac.mm:-636
> > +    for (size_t i = 0; i < types.size(); i++)
> >          addHTMLClipboardTypesForCocoaType(result, types[i], m_pasteboardName);
> > -    }
> 
> We are underutilizing the index variable i. I suggest we write this loop
> using a range-based for loop.

Fixed.

> > Source/WebKit/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm:58
> > +        for (size_t i = 0; i < pathnames.size(); i++) {
> 
> I suggest we write this using a range-based for-loop.

Well, we need access an element in sandboxExtensions by sandboxExtensions[i] so we can't do that.

[Wenson Hsieh]

Comment on attachment 322143 [details]
Adds the feature

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

(Just a few more minor things I noticed)

> Source/WebCore/dom/DataTransfer.cpp:202
> +                reader.type = type;

Nit - I think it's a bit confusing to pass in a struct (PasteboardFileReader) whose first member is essentially an argument to Pasteboard::read, and whose second member is the result of Pasteboard::read. Can we just pass in the type as the first argument to Pasteboard::read, so the method signature is read(const String&, PasteboardFileReader&)?

>>> Source/WebCore/platform/cocoa/PasteboardCocoa.mm:63
>>> +    if (cocoaType == String(NSTIFFPboardType))
>> 
>> Is it necessary to explicitly call the String constructor? Same question for the other calls to the String constructor throughout this function.
> 
> We do. I got rid of it form "Apple PNG pasteboard type". But kUTTypeGIF, etc... needs an explicit conversion.

We need to make a String out of these UTIs, else the compiler will complain that '==' is ambiguous with operands of type String and CFStringRef.

> Source/WebCore/platform/cocoa/PasteboardCocoa.mm:81
> +static const char* imageTypeToMimeType(ImageType type)

Nit - I would've thought that since MIME is an acronym, this function name should be imageTypeToMIMEType.

> Source/WebCore/platform/cocoa/PasteboardCocoa.mm:197
> +        RetainPtr<NSBitmapImageRep> image = adoptNS([[NSBitmapImageRep alloc] initWithData: tiffData.get()]);

auto

> Source/WebCore/platform/cocoa/PasteboardCocoa.mm:198
> +        NSData *pngData = [image.get() representationUsingType: NSPNGFileType properties:@{ }];

Extra space after the : here.

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #27)
> Comment on attachment 322143 [details]
> Adds the feature
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322143&action=review
> 
> > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:202
> > +        data.append(buffer->data(), buffer->size());
> 
> I think it's a bit strange that on iOS, this line is indented even though
> the else is defined out. Can we just move the #if PLATFORM(MAC) inside the
> if statement, and let tiffWasTreatedAsPNGForWebCompatibility = false on iOS
> so that this append is at the right indentation level on iOS?

Yeah, this code isn't great. Extracted createFileFromBuffer and rewrote it like:

#if PLATFORM(MAC)
    if (tiffWasTreatedAsPNGForWebCompatibility) {
...
        createFileFromBuffer(reader, imageType, reinterpret_cast<const char*>(pngData.bytes), pngData.length);
        return;
    }
#endif
    createFileFromBuffer(reader, imageType, buffer->data(), buffer->size());

> > Source/WebCore/platform/ios/PasteboardIOS.mm:424
> > +Vector<String> Pasteboard::dataTypes()
> 
> Perhaps this could be dataTypesForBindings, to be consistent with
> readStringForBindings?

So *ForBindings functions are supposed to only exist on objects that have wrappers so that we can't differentiate ones exposed to the Web versus ones only used internally. In this case, DataTransfer is the one that's exposed to the Web and Pasteboard is our internal object so we don't really want to have *ForBindings functions.

Having said that, let's just rename it to dataTypesForBindings for now since this member function is not used anywhere other than DataTransfer.

> > Source/WebKit/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm:52
> > +        auto pasteboard = PlatformPasteboard(pasteboardName);
> 
> I don't think the local variable here adds much.

Removed.

> >> Source/WebKit/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm:58
> >> +        for (size_t i = 0; i < pathnames.size(); i++) {
> > 
> > I suggest we write this using a range-based for-loop.
> 
> ...we do use i in this case though, for sandboxExtensions[I]


Yup.
 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteImage.mm:54
> > +    [[UIPasteboard generalPasteboard] setItems:@[[NSDictionary dictionaryWithObject:data forKey:type]]];
> 
> Probably not for this patch, but I'm a bit curious what happens on iOS with
> multiple images in the pasteboard (e.g. using something like [~ setItems:@[
> @{ type1: data1 }, @{ type2: data2 } ]])

Indeed, we should test that...

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteImage.mm:60
> > +    RetainPtr<TestWKWebView> webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 400, 400)]);
> 
> I would prefer auto for these, since declaring the type here doesn't add
> much information.

Fixed and the rest of webView declarations. However, in Objective-C, it's particularly tricky to figure out whether a given object is autorelease'd or RetainPtr'ed in Webkit although in this case we just called adoptNS so that makes it clear.

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteImage.mm:109
> > +    EXPECT_STREQ("false", [webView stringByEvaluatingJavaScript:@"dataTransfer.types.includes('image/gif').toString()"].UTF8String);
> 
> You can EXPECT_WK_STREQ here and avoid the .UTF8String.

Done this.

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #29)
> Comment on attachment 322143 [details]
> Adds the feature
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322143&action=review
> 
> (Just a few more minor things I noticed)
> 
> > Source/WebCore/dom/DataTransfer.cpp:202
> > +                reader.type = type;
> 
> Nit - I think it's a bit confusing to pass in a struct
> (PasteboardFileReader) whose first member is essentially an argument to
> Pasteboard::read, and whose second member is the result of Pasteboard::read.
> Can we just pass in the type as the first argument to Pasteboard::read, so
> the method signature is read(const String&, PasteboardFileReader&)?

Maybe but all other read(~) functions take a single argument, and its arguments are passed in its associated structure so it's probably best to stick with that.

> > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:81
> > +static const char* imageTypeToMimeType(ImageType type)
> 
> Nit - I would've thought that since MIME is an acronym, this function name
> should be imageTypeToMIMEType.

Fixed.

> > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:197
> > +        RetainPtr<NSBitmapImageRep> image = adoptNS([[NSBitmapImageRep alloc] initWithData: tiffData.get()]);
> 
> auto

Fixed.

> > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:198
> > +        NSData *pngData = [image.get() representationUsingType: NSPNGFileType properties:@{ }];
> 
> Extra space after the : here.

Removed.

[Ryosuke Niwa]

Created attachment 322170 [details]
Addressed review comments and removed layering violation

[Build Bot]

Attachment 322170 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/cocoa/PasteboardCocoa.mm:49:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/cocoa/PasteboardCocoa.mm:50:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/cocoa/PasteboardCocoa.mm:51:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/cocoa/PasteboardCocoa.mm:52:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 4 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 322180 [details]
Fixed mac builds

[Build Bot]

Attachment 322180 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/cocoa/PasteboardCocoa.mm:49:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/cocoa/PasteboardCocoa.mm:50:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/cocoa/PasteboardCocoa.mm:51:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/cocoa/PasteboardCocoa.mm:52:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 4 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wenson Hsieh]

Comment on attachment 322180 [details]
Fixed mac builds

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

> Source/WebCore/platform/Pasteboard.cpp:29
> +#include "File.h"

Do we still need to include File.h in Pasteboard.cpp?

> Source/WebCore/platform/cocoa/PasteboardCocoa.mm:29
> +#include "File.h"

Here too -- now that we only need to know about PasteboardFileReader, do we need File.h?

[Daniel Bates]

Comment on attachment 322180 [details]
Fixed mac builds

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

> Source/WebCore/dom/DataTransfer.cpp:200
> +            auto fileTypes = m_pasteboard->typesTreatedAsFiles();

Can we inline this variable into the line below?

> Source/WebCore/editing/WebCorePasteboardFileReader.cpp:36
> +void WebCorePasteboardFileReader::read(const String& filename, Ref<SharedBuffer>&& buffer)

The buffer should be taken by lvalue-reference (not rvalue-reference) because this function does not take ownership of it. The string could be taken by rvalue-reference and moved into Blob::create() to remove a ref of the underlying string impl.

> Source/WebCore/editing/WebCorePasteboardFileReader.h:41
> +    void read(const String&, Ref<SharedBuffer>&&) final;

Ditto.

> Source/WebCore/platform/Pasteboard.h:155
> +    virtual void read(const String&, Ref<SharedBuffer>&&) = 0;

Ditto.

[Daniel Bates]

(In reply to Daniel Bates from comment #37)
> Comment on attachment 322180 [details]
> > Source/WebCore/editing/WebCorePasteboardFileReader.cpp:36
> > +void WebCorePasteboardFileReader::read(const String& filename, Ref<SharedBuffer>&& buffer)
> 
> The buffer should be taken by lvalue-reference (not rvalue-reference)
> because this function does not take ownership of it. The string could be
> taken by rvalue-reference and moved into Blob::create() to remove a ref of
> the underlying string impl.


Obviously the last sentence assumes there is a Blob::create() overload that can handle a String rvalue-reference.

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #36)
> Comment on attachment 322180 [details]
> Fixed mac builds
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322180&action=review
> 
> > Source/WebCore/platform/Pasteboard.cpp:29
> > +#include "File.h"
> 
> Do we still need to include File.h in Pasteboard.cpp?
> 
> > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:29
> > +#include "File.h"
> 
> Here too -- now that we only need to know about PasteboardFileReader, do we
> need File.h?

Nice catch. Fixed.

(In reply to Daniel Bates from comment #37)
> Comment on attachment 322180 [details]
> Fixed mac builds
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322180&action=review
> 
> > Source/WebCore/dom/DataTransfer.cpp:200
> > +            auto fileTypes = m_pasteboard->typesTreatedAsFiles();
> 
> Can we inline this variable into the line below?

Fixed.

> > Source/WebCore/editing/WebCorePasteboardFileReader.cpp:36
> > +void WebCorePasteboardFileReader::read(const String& filename, Ref<SharedBuffer>&& buffer)
> 
> The buffer should be taken by lvalue-reference (not rvalue-reference)
> because this function does not take ownership of it. The string could be
> taken by rvalue-reference and moved into Blob::create() to remove a ref of
> the underlying string impl.

Well, this function is supposed to "consume" the shared buffer so it semantically takes the ownership of the shared buffer. That is, the semantics of these read() functions on Pasteboard*Reader classes is that it's the last thing Pasteboard code calls to hand over the contents of the shared buffer, and the buffer itself. So I'm not gonna change this code.

[Ryosuke Niwa]

Committed r222656: <http://trac.webkit.org/changeset/222656>