Bug 81261 – [chromium] Support file drag out using DataTransferItemList::add(File)

• Component: Platform
• Status: RESOLVED FIXED

[Daniel Cheng]

Right now, file drag out isn't very well supported. We have downloadurl and the default drag handler for images, but we don't actually support image dragout using the specced method.

[Varun Jain]

Created attachment 133417 [details]
Patch

[Tony Chang]

Comment on attachment 133417 [details]
Patch

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

r- for no test.  Please add one to ManualTests or create a layout test.

> Source/WebCore/ChangeLog:8
> +        No new tests. Manually tested that file drag and drop doesnot crash chrome.

Please create a layout test or at least make a manual test and put it in ManualTests.  There are a few other drag&drop tests in that directory already.

> Source/WebCore/platform/chromium/DataTransferItemListChromium.cpp:123
> +    // Note that passing file to createFromFile below transfers ownership of
> +    // it, so file is 0 after this call.
> +    m_itemList.append(DataTransferItemChromium::createFromFile(file));

Are you sure about this comment?  |file| is a ref counted pointer so createFromFile shouldn't delete anything.  Is the order that we add items important here?  Daniel would know.

[Rick Byers]

(In reply to comment #2)
> (From update of attachment 133417 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133417&action=review
> 
> r- for no test.  Please add one to ManualTests or create a layout test.

Daniel, are you taking this over now that you're back in the office?

> > Source/WebCore/platform/chromium/DataTransferItemListChromium.cpp:123
> > +    // Note that passing file to createFromFile below transfers ownership of
> > +    // it, so file is 0 after this call.
> > +    m_itemList.append(DataTransferItemChromium::createFromFile(file));
> 
> Are you sure about this comment?  |file| is a ref counted pointer so createFromFile shouldn't delete anything.  Is the order that we add items important here?  Daniel would know.

Moving this definitely fixed the crash I saw: crbug.com/119591
I'm new to these WebKit types, but http://www.webkit.org/coding/RefPtr.html says that assigning a PassRefPtr to a RefPtr clears the original pointer, and inside createFromFile we have:
  item->m_file = file;
Transferring ownership of the File to the DataTransferItemChromium.

I assumed the order of the items was irrelevant, but Daniel is the expert.

[Tony Chang]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 133417 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=133417&action=review
> > > Source/WebCore/platform/chromium/DataTransferItemListChromium.cpp:123
> > > +    // Note that passing file to createFromFile below transfers ownership of
> > > +    // it, so file is 0 after this call.
> > > +    m_itemList.append(DataTransferItemChromium::createFromFile(file));
> > 
> > Are you sure about this comment?  |file| is a ref counted pointer so createFromFile shouldn't delete anything.  Is the order that we add items important here?  Daniel would know.
> 
> Moving this definitely fixed the crash I saw: crbug.com/119591
> I'm new to these WebKit types, but http://www.webkit.org/coding/RefPtr.html says that assigning a PassRefPtr to a RefPtr clears the original pointer, and inside createFromFile we have:
>   item->m_file = file;
> Transferring ownership of the File to the DataTransferItemChromium.

createFromFile gets a copy of PassRefPtr. The original copy in DataTransferItemListChromium is unmodified.

[Daniel Cheng]

Created attachment 133568 [details]
Patch

[Tony Chang]

Comment on attachment 133568 [details]
Patch

Will this test pass on other ports or should it be in the Skipped files?

[Daniel Cheng]

Committed r111919: <http://trac.webkit.org/changeset/111919>