[Webkit-unassigned] [Bug 58210] webkit should implement the dropzone attribute

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=58210


Tony Chang <tony at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #92175|review?                     |review-
               Flag|                            |




--- Comment #26 from Tony Chang <tony at chromium.org>  2011-05-04 16:21:43 PST ---
(From update of attachment 92175)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.