Bug 53727 – Clone WebClipboard to be frame-specific

• Component: New Bugs
• Status: RESOLVED FIXED

[Daniel Cheng]

Clone WebClipboard to be view-specific

[Daniel Cheng]

Created attachment 81130 [details]
Patch

[Daniel Cheng]

Comment on attachment 81130 [details]
Patch

webkit-patch didn't quite do what I expected.

[Daniel Cheng]

Created attachment 81142 [details]
Patch

[Tony Chang]

Comment on attachment 81142 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        For drop operations, Chrome currently snapshots the data and copies it
> +        into the renderer process. As we add more supported drag data types, the
> +        copy will become increasingly expensive. Instead, we'd like to snapshot
> +        data in the browser to reduce the amount of data copied and to support
> +        Blob in DataTransferItem. In order to allow this, we associated

I'm sorry, I'm not sure I follow.  Is the copy you're referring to the IPC?  I thought the slow part was reading from the drag source?

When you say you snapshot data in the browser, you mean the browser process?  Wouldn't that still need to be copied over IPC?

[Daniel Cheng]

(In reply to comment #4)
> (From update of attachment 81142 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81142&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        For drop operations, Chrome currently snapshots the data and copies it
> > +        into the renderer process. As we add more supported drag data types, the
> > +        copy will become increasingly expensive. Instead, we'd like to snapshot
> > +        data in the browser to reduce the amount of data copied and to support
> > +        Blob in DataTransferItem. In order to allow this, we associated
> 
> I'm sorry, I'm not sure I follow.  Is the copy you're referring to the IPC?  I thought the slow part was reading from the drag source?

It is, but given that Chrome doesn't actually wait for WebKit to finish drop handling before signaling the OS that the drop is complete, it's not possible to implement the original design on OS X. There probably would have been corner cases that failed on Windows as well. It worked well on Linux though. =)

> 
> When you say you snapshot data in the browser, you mean the browser process?  Wouldn't that still need to be copied over IPC?

Yes, but only the data requested (through getData or otherwise) would be copied over. This saves copying over any data we don't care about (for example, images).

[Daniel Cheng]

Created attachment 81159 [details]
Patch

[Dmitry Titov]

Adding Darin Fisher since there is a change to Chromium API

[Dmitry Titov]

Comment on attachment 81159 [details]
Patch

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

A few comments (r- to clarify the lifetime of stored Document*):

> Source/WebCore/ChangeLog:16
> +        No new tests.

Lets explain more here. Is it because this code path is not yet enabled and requires more changes in Chromium so when it'll be enabled the existing tests will cover?

> Source/WebCore/platform/chromium/ChromiumDataObject.cpp:60
> +    const Document* document, Clipboard::ClipboardType clipboardType)

WebKit doesn't try to fit in 80 chars. Same for the other files in patch.

> Source/WebCore/platform/chromium/ReadableDataObject.h:67
> +    const Document* m_document;

2 questions here: 
- If it is not a RefPtr, we should have a good theory why the document is guaranteed to be alive for the lifetime of ReadableDataObject, or we should clear it (and check in getClipboard), or make this a RefPtr.
- Why isn't it Frame?

> Source/WebKit/chromium/public/WebFrameClient.h:92
> +    // A frame specific clipboard. May return null, in which case... you're done.

It's be better to say a bit more, "the caller should assume there is no data in clipboard" or something like that?

[Tony Chang]

(In reply to comment #5)
> It is, but given that Chrome doesn't actually wait for WebKit to finish drop handling before signaling the OS that the drop is complete, it's not possible to implement the original design on OS X. There probably would have been corner cases that failed on Windows as well. It worked well on Linux though. =)

I see, so you read all the data and keep a copy in the browser process.  Then the renderer process determines what types of data it wants and asks the browser process for it.  The browser process then sends back the requested data.

Since this is async, it's possible for another drag to start before the renderer process asks for data, in which case it's ambiguous what the renderer is asking for.  To disambiguate, you include which frame is asking for the data.

Sounds reasonable to me.

[Darin Fisher (:fishd, Google)]

Comment on attachment 81159 [details]
Patch

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

>> Source/WebKit/chromium/public/WebFrameClient.h:92
>> +    // A frame specific clipboard. May return null, in which case... you're done.
> 
> It's be better to say a bit more, "the caller should assume there is no data in clipboard" or something like that?

This API change looks good to me.  Not sure what the "... you're done" bit is supposed to imply.  If it means that you cannot use a null clipboard, then I think that follows without saying :)

[Daniel Cheng]

Created attachment 81295 [details]
Patch

[Dmitry Titov]

Comment on attachment 81295 [details]
Patch

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

Thanks! 
r=me with a small nit. Change on landing or re-submit a patch for cq.

> Source/WebCore/platform/chromium/ReadableDataObject.h:65
> +    // The owner document. Used to send IPCs back to the correspdonging view

Lets change "document"->"frame"

[Daniel Cheng]

Committed r77687: <http://trac.webkit.org/changeset/77687>