[webkit-reviews] review denied: [Bug 30416] dataTransfer.types (HTML5 drag & drop) should return DOMStringList : [Attachment 127027] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied  review:
Bug 30416: dataTransfer.types (HTML5 drag & drop) should return DOMStringList
https://bugs.webkit.org/show_bug.cgi?id=30416

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=127027&action=review


I strongly recommend reading http://www.webkit.org/coding/RefPtr.html if you
haven't already.  I think Señor Levin might also want to take another look.

> Source/WebCore/dom/Clipboard.cpp:151
> +    return types()->contains(type); 

It's sad that we're creating a new object every time. :(  I guess we were doing
that before, but it wasn't as obvious.

Also, this is no longer null-safe.  If platform Clipboard implementations ever
return NULL this will crash. :(

> Source/WebCore/platform/chromium/ChromiumDataObject.cpp:115
> +	   for (HashSet<String>::const_iterator it = hashedResults.begin();
> +		it != hashedResults.end();
> +		++it) {

WebKit has no 80c rule.  Also, I'm not sure if we use { } for single line for
statements.

> Source/WebCore/platform/chromium/ChromiumDataObject.cpp:118
> +	   return results;

This should be results.release();

> Source/WebCore/platform/chromium/ClipboardChromium.cpp:306
>      return results;

results.release();

> Source/WebCore/platform/efl/ClipboardEfl.cpp:84
> +    return 0;

This will cause crashes based on other changes you made. :(

> Source/WebCore/platform/win/ClipboardWin.cpp:489
> +static void addMimeTypesForFormat(PassRefPtr<DOMStringList> results, const
FORMATETC& format)

This should not be a PassRefPtr.  This function does not take ownership of the
List.  RefPtr& or just a raw ptr is fine.

> LayoutTests/ChangeLog:8
> +	   Update tests to use contains() instead of indexOf().

Lame that it doensn't have indexOf.  I wonder if this will break sites...

http://www.w3.org/TR/DOM-Level-3-Core/core.html#DOMStringList

> LayoutTests/ChangeLog:9
> +

I would like to see a test of clipboard.types == clipboard.types.  I suspect
they're !=.  I suspect they were != before (since it looks like our code
created a new array for each call, but it would be nice to confirm, and have an
expectation for that behavior.	(It would also be nice to know what FF does.)