[webkit-reviews] review denied: [Bug 58210] webkit should implement the dropzone attribute : [Attachment 92175] Patch.

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Yael <yael.aharon at nokia.com>'s
request for review:
Bug 58210: webkit should implement the dropzone attribute
https://bugs.webkit.org/show_bug.cgi?id=58210

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

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=92175&action=review

I haven't looked that closely at the whole patch, but I have some general
concerns about implementing this as the current spec seems a bit rough around
the edges.  Here are my concerns about the spec:

- Multiple copy/move/link values is confusing.	The spec says it's invalid, but
the algorithm describes just taking the first value.  Does that mean it's
valid, but we just take the first value?

- The f: s: syntax unnecessarily terse to me.  Why can't we write file: or
string:?  If we're worried about duplicates, it could be a semicolon separated
list or something.

- Should f:image/* and s:text/* be valid?  The spec seems to say they're not
valid, but it seems like that would be the common case (e.g., Chromium might
want to accept bmps or the webm image format but requiring to the web author be
explicit about it seems unfortuante).

Also, since this is an early draft, should we prefix the attribute with webkit?
 It looks like we have webkitallowfullscreen and webkitdirectory currently.

It would be nice if we could clarify some of these issues on the whatwg mailing
list before committing an implementation.

> Source/WebCore/dom/Clipboard.cpp:189
> +    if (equalIgnoringCase(dragOperation, "copy"))
> +	   return DragOperationCopy;

Nit: Seems like we should only lower case this once.

> Source/WebCore/dom/Clipboard.cpp:207
> +    default:
> +	   return String("copy");

Is it possible to just enumerate all values here?  Then when a new enum value
is added, we get a compile error so someone can update this code.

> Source/WebCore/dom/Clipboard.cpp:216
> +    switch (keyword[0]) {

I think you need to allow F and S here.  The spec says "Let kind code be the
first character in keyword, converted to ASCII lowercase."  Maybe we can
lowercase dropZoneStr above.

> Source/WebCore/dom/Clipboard.h:113
> +    private:

Nit: Why two contiguous private sections?

> LayoutTests/fast/events/dropzone-004.html:33
> +    dropTarget.setAttribute("dropzone",
dropEffectElem.options[dropEffectElem.selectedIndex].value + " f:text/html");

It would be nice if there were some tests that change the order of the tokens
in the dropzone attribute or try invalid values.

> LayoutTests/fast/events/script-tests/dropzone.js:2
> +function drop(e)
> +{

This file should go in fast/events/resources/ since the test logic isn't in
this file and you're not using js-test-post.js.