Bug 76598 – [chromium] Refactor ClipboardChromium and DataTransferItemList/DataTransferItem to support HTML spec

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Daniel Cheng]

[chromium] Refactor ClipboardChromium and DataTransferItemList/DataTransferItem to support HTML spec

[Daniel Cheng]

Created attachment 123057 [details]
Patch

[Daniel Cheng]

Just a starter patch that shows the gist of what I think we should do. A lot of stuff is still unimplemented, such as the lazy population of ChromiumDataObject::items() but the general idea is there.

I'm not sure about the division of code between ClpboardChromium and ChromiumDataObject at the moment; I suspect it's something will become clearer with more refactoring.

[Tony Chang]

(In reply to comment #2)
> Just a starter patch that shows the gist of what I think we should do. A lot of stuff is still unimplemented, such as the lazy population of ChromiumDataObject::items() but the general idea is there.

Can you explain what the refactoring is doing and why?  It's not obvious to me from skimming the huge patch.

[Daniel Cheng]

There's a lot of duplicated logic today between ClipboardChromium and DataTransferItemListChromium/DataTransferItemChromium because the underlying representation between the two is so different, and it leads to a lot of situations where things are sort of implemented.

I'm trying to unify this by representing the drag store / clipboard store as a list of (kind, type, value) tuples. I was planning on having a generic DataTransferItemChromium virtual interface, and then having various specializations to handle the various quirks of each flavor, e.g:

DataTransferItemDragText -- for generic text or custom MIME type text
DataTransferItemDragUriList -- for URLs, since we try to remember the title associated with the URL in the case of dragging an <a>
DataTransferItemDragHtml -- for HTML, so we can associate a base URL where appropriate.
DataTransferItemDragFile -- generic encapsulation for files. Maybe even add proper drag out/drag in support for file stream stuff, like when images are dragged. This comes with security considerations though. This will hopefully also make it easier for kinuko's work.

DataTransferItemClipboardText
DataTransferItemClipboardHtml
DataTransferItemClipboardImage -- I may just rename this to Blob.

ChromiumDataObject will manage this list, which makes exposing the actual DataTransferItem/DataTransferItemList to the page much easier, since it's the same as the underlying representation. The existing DataTransfer legacy APIs can expressed in terms of operations on the data store list as well.

This also removes the huge if()s in ChromiumDataObject::getData()/setData() and DataTransferItemChromium::getAsString(). We pay a slight performance cost because of additional vtable dispatch, but I think it's worth it to make this code more readable, understandable, and extensible.

I'm planning on uploading another snapshot later today--I was hoping to have something more substantial by now, but a power outage took out my machines.

[Daniel Cheng]

Created attachment 123271 [details]
Patch

[Daniel Cheng]

Unfortunately, this patch is huge and I don't have good ideas on how to break it up. It does better show the intent of this CL though--I've implemented the basic plumbing between ChromiumDataObject and WebDragData, and the hierarchy in DataTransferItemChromium.h should give a good idea of how I'm cleaning up the getAsString() implementations.

[Daniel Cheng]

Created attachment 123698 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[WebKit Review Bot]

Comment on attachment 123698 [details]
Patch

Attachment 123698 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11336284

New failing tests:
fast/events/dropzone-002.html
editing/pasteboard/files-during-page-drags.html
editing/pasteboard/drop-link.html

[Daniel Cheng]

Created attachment 123763 [details]
Patch

Fix files-during-page-drags.html

[WebKit Review Bot]

Comment on attachment 123763 [details]
Patch

Attachment 123763 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11242274

New failing tests:
fast/events/dropzone-002.html
editing/pasteboard/drop-link.html

[Daniel Cheng]

Created attachment 123837 [details]
Patch

Rebased patch for EWS. The new layout tests are not included yet.

[Daniel Cheng]

Comment on attachment 123837 [details]
Patch

This patch is being broken up and landed as a series of patches. This bug will remain open for tracking reasons.

[Daniel Cheng]

Created attachment 131729 [details]
Patch

[Daniel Cheng]

Several things that'd make sense for a followup patch:
- Rename DataTransferItemListChromium and DataTransferItemChromium since they aren't (directly) DataTransferItemList/DataTransferItem anymore.
- Combine DataTransferItemListChromium and ChromiumDataObject.

[Tony Chang]

Comment on attachment 131729 [details]
Patch

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

This patch is still ginormous.  I probably missed lots of things, but I'm tired of scrolling up and down the patch.  Would it be possible to transition one type at a time?

> Source/WebCore/platform/chromium/ChromiumDataObject.cpp:92
> +    for (size_t i = 0; i < m_itemList->length(); ) {
> +        if (m_itemList->item(i)->kind() == DataTransferItem::kindString) {
> +            m_itemList->deleteItem(i, ASSERT_NO_EXCEPTION);
> +            continue;

I'm a bit worried that future kinds will be added and this code forgotten.  != kindFile would help insulate from that.  Alternately, you could make a new list, copy the file kinds into it then swap.  Deleting while iterating makes me a bit nervous.

> Source/WebCore/platform/chromium/ChromiumDataObject.cpp:190
> +ChromiumDataObject::ChromiumDataObject(PassRefPtr<DataTransferItemListChromium> itemList)
> +    : m_itemList(itemList)

Is it OK that we're sharing a pointer to itemList?  For example, ChromiumDataObject::copy() uses this constructor and will end up pointing to the same underlying itemList.  What is the expected behavior of copy()?

> Source/WebCore/platform/chromium/ChromiumDataObject.h:68
> +    void getURL(String& url, String* title) const;
> +    void setURL(const String& url, const String& title);
> +    void getHTML(String& html, KURL& baseURL) const;

WebKit style is to not use the get prefix on getters.  In getURL, should |title| have a default value of 0?

> Source/WebCore/platform/chromium/ClipboardChromium.cpp:152
> +    m_list->add(file, m_clipboard->frame()->document()->scriptExecutionContext());

Is it possible for document() to change (thereby pointing to a different scriptExecutionContext) from the time the wrapper was created? It looks like the old code was holding on to the context from an earlier time.  Maybe this can't happen?

> Source/WebCore/platform/chromium/ClipboardChromium.cpp:212
> +static String normalizeType(const String& type, bool& convertToURL)

Nit: I would make convertToURL a pointer to a bool with a default value of 0 since only one caller needs it.

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:82
> +DataTransferItemChromium::DataTransferItemChromium(const String& kind, const String& type, const String& data,
> +    PassRefPtr<File> file, PassRefPtr<SharedBuffer> sharedBuffer, const String& title, const KURL& baseURL)

Passing 7 parameters seems error prone.  Why not just have a constructor with kind and type and have the create methods set the other members?

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:119
> +        ASSERT(m_sharedBuffer);
> +        return 0; // FIXME: Support this?

By this, do you mean returning a shared buffer?

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:167
> +    return PlatformSupport::clipboardSequenceNumber(currentPasteboardBuffer()) == m_sequenceNumber
> +        ? data
> +        : String();

Nit: I wouldn't bother wrapping here. Actually, I would just early return if the sequence number doesn't match so we don't do the extra work.

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:173
> +    // FIXME: When we properly support File dragout, we'll need to make sure
> +    // this works as expected for DragDataChromium.

Is there a bug filed for supporting file drag outs?

> Source/WebCore/platform/chromium/DataTransferItemChromium.h:56
> +    static PassRefPtr<DataTransferItemChromium> create(PassRefPtr<File>);
> +    static PassRefPtr<DataTransferItemChromium> createFromURL(const String& url, const String& title);
> +    static PassRefPtr<DataTransferItemChromium> createFromHTML(const String& html, const KURL& baseURL);
> +    static PassRefPtr<DataTransferItemChromium> create(const String& filename, PassRefPtr<SharedBuffer>);
> +    static PassRefPtr<DataTransferItemChromium> createFromPasteboard(const String& type, uint64_t sequenceNumber);

Why are some of these createFrom* and some are just create?

> Source/WebCore/platform/chromium/DataTransferItemListChromium.cpp:61
> +    uint64 sequenceNumber = PlatformSupport::clipboardSequenceNumber(currentPasteboardBuffer());

It looks like sequenceNumber should be uint64_t to match the function.

> Source/WebCore/platform/chromium/DataTransferItemListChromium.cpp:85
>  void DataTransferItemListChromium::deleteItem(unsigned long index, ExceptionCode& ec)

|ec| appears to be unused. Doesn't clang complain about that?

> Source/WebKit/chromium/src/WebDragData.cpp:87
> +                continue; // FIXME: Support File properly.

Is this for the filename case that we were skipping before?  This FIXME is vague, can you expand on it more?

> LayoutTests/fast/events/clipboard-dataTransferItemList.html:117
> +    [runTest, [0, 0]],
> +    [runTest, [0, 1]],
> +    [runTest, [1, 0]],
> +    [runTest, [1, 1]],

I would just hardcode runTest in runNext().  No need to repeat it 4 times here.

> LayoutTests/fast/events/drag-dataTransferItemList.html:145
> +    [runTest, [0, 0]],
> +    [runTest, [0, 1]],
> +    [runTest, [1, 0]],
> +    [runTest, [1, 1]],

I would just hardcode runTest in runNext().

[Daniel Cheng]

Created attachment 132139 [details]
Patch

[Daniel Cheng]

Comment on attachment 131729 [details]
Patch

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

>> Source/WebCore/platform/chromium/ChromiumDataObject.cpp:92
>> +            continue;
> 
> I'm a bit worried that future kinds will be added and this code forgotten.  != kindFile would help insulate from that.  Alternately, you could make a new list, copy the file kinds into it then swap.  Deleting while iterating makes me a bit nervous.

Fixed to use kindFile.

>> Source/WebCore/platform/chromium/ChromiumDataObject.cpp:190
>> +    : m_itemList(itemList)
> 
> Is it OK that we're sharing a pointer to itemList?  For example, ChromiumDataObject::copy() uses this constructor and will end up pointing to the same underlying itemList.  What is the expected behavior of copy()?

Yes, it's OK to do this. We implement this operator for WebDragData, which asserts that only one ref is held before updating it.

>> Source/WebCore/platform/chromium/ChromiumDataObject.h:68
>> +    void getHTML(String& html, KURL& baseURL) const;
> 
> WebKit style is to not use the get prefix on getters.  In getURL, should |title| have a default value of 0?

Done for both. The default arg isn't really used currently though.

>> Source/WebCore/platform/chromium/ClipboardChromium.cpp:152
>> +    m_list->add(file, m_clipboard->frame()->document()->scriptExecutionContext());
> 
> Is it possible for document() to change (thereby pointing to a different scriptExecutionContext) from the time the wrapper was created? It looks like the old code was holding on to the context from an earlier time.  Maybe this can't happen?

I don't think this can happen. It's a slight optimization to avoid holding on to pointers that have a high chance of not being used.

>> Source/WebCore/platform/chromium/ClipboardChromium.cpp:212
>> +static String normalizeType(const String& type, bool& convertToURL)
> 
> Nit: I would make convertToURL a pointer to a bool with a default value of 0 since only one caller needs it.

Done.

>> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:82
>> +    PassRefPtr<File> file, PassRefPtr<SharedBuffer> sharedBuffer, const String& title, const KURL& baseURL)
> 
> Passing 7 parameters seems error prone.  Why not just have a constructor with kind and type and have the create methods set the other members?

This lets the data members be const. It'd be nice to keep that, but I don't see how without losing the const members or using lots of const casts.

>> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:119
>> +        return 0; // FIXME: Support this?
> 
> By this, do you mean returning a shared buffer?

Yes, we'd need some way to turn sharedBuffer into a file or blob.

>> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:167
>> +        : String();
> 
> Nit: I wouldn't bother wrapping here. Actually, I would just early return if the sequence number doesn't match so we don't do the extra work.

This is intended to fix a slight race condition (the sequence number may have changed between the time we got the data and the time we checked the sequence number)

>> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:173
>> +    // this works as expected for DragDataChromium.
> 
> Is there a bug filed for supporting file drag outs?

There isn't. I made one, though I have no plans to address it at the moment.

>> Source/WebCore/platform/chromium/DataTransferItemChromium.h:56
>> +    static PassRefPtr<DataTransferItemChromium> createFromPasteboard(const String& type, uint64_t sequenceNumber);
> 
> Why are some of these createFrom* and some are just create?

Originally it was just to avoid the clash from create() and createFromURL() which have the same type signature. I've just updated it so all of them are createFrom*

>> Source/WebCore/platform/chromium/DataTransferItemListChromium.cpp:61
>> +    uint64 sequenceNumber = PlatformSupport::clipboardSequenceNumber(currentPasteboardBuffer());
> 
> It looks like sequenceNumber should be uint64_t to match the function.

Done.

> Source/WebCore/platform/chromium/DataTransferItemListChromium.cpp:85
>  void DataTransferItemListChromium::deleteItem(unsigned long index, ExceptionCode& ec)

Context: unused |ec|. I've removed it, but I can't seem to reply directly because of some weird bug...

>> Source/WebKit/chromium/src/WebDragData.cpp:87
>> +                continue; // FIXME: Support File properly.
> 
> Is this for the filename case that we were skipping before?  This FIXME is vague, can you expand on it more?

Done. This would be needed to support DataTransferItemList.add(File)

>> LayoutTests/fast/events/clipboard-dataTransferItemList.html:117
>> +    [runTest, [1, 1]],
> 
> I would just hardcode runTest in runNext().  No need to repeat it 4 times here.

Done.

>> LayoutTests/fast/events/drag-dataTransferItemList.html:145
>> +    [runTest, [1, 1]],
> 
> I would just hardcode runTest in runNext().

Done.

[Tony Chang]

Comment on attachment 132139 [details]
Patch

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

I'm really only confident that I've checked the coding style.  The patch is too big for me to know if it's correct or not.  I think it's fine to land, but we should try to cover more test types in the test (e.g., files, text/html, etc)

> Source/WebCore/platform/chromium/ChromiumDataObject.h:66
> +    void urlAndTitle(String& url, String* title = 0) const;

If all callers provide title, we can remove the default param (I would just make it pass by reference so it's more like htmlAndBaseURL).

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:82
> +DataTransferItemChromium::DataTransferItemChromium(const String& kind, const String& type, const String& data,
> +    PassRefPtr<File> file, PassRefPtr<SharedBuffer> sharedBuffer, const String& title, const KURL& baseURL)

Given the simplicity of this class and that all the methods are const, I'm not worried that the members are const or not.  When introducing a new type, I would find "adoptRef(new DataTransferItemChromium(DataTransferItem::kindFile, String(), String(), 0, buffer, name, KURL()));" harder to understand and more error prone than accidentally mutating a member variable.

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:119
> +        ASSERT(m_sharedBuffer);
> +        return 0; // FIXME: Support this?

Please make the comment more clear.  E.g., FIXME: Support converting a shared buffer into a file.

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:165
> +    return PlatformSupport::clipboardSequenceNumber(currentPasteboardBuffer()) == m_sequenceNumber ? data : String();

Is there a test covering this bug fix?  Hiding a bug fix in an 80k patch is likely to regress in the future.  I would be better to fix the bug in a separate patch.

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:171
> +    // FIXME: When we properly support File dragout, we'll need to make sure
> +    // this works as expected for DragDataChromium.

Please include a link to the bug you created here.

[Daniel Cheng]

Committed r111061: <http://trac.webkit.org/changeset/111061>