[webkit-reviews] review denied: [Bug 76598] [chromium] Refactor ClipboardChromium and DataTransferItemList/DataTransferItem to support HTML spec : [Attachment 131729] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Daniel Cheng <dcheng at chromium.org>'s
request for review:
Bug 76598: [chromium] Refactor ClipboardChromium and
DataTransferItemList/DataTransferItem to support HTML spec
https://bugs.webkit.org/show_bug.cgi?id=76598

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

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