[webkit-reviews] review denied: [Bug 81261] [chromium] Support file drag out using DataTransferItemList::add(File) : [Attachment 133417] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Varun Jain <varunjain at chromium.org>'s
request for review:
Bug 81261: [chromium] Support file drag out using
DataTransferItemList::add(File)
https://bugs.webkit.org/show_bug.cgi?id=81261

Attachment 133417: Patch
https://bugs.webkit.org/attachment.cgi?id=133417&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
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.