Bug 58210 – webkit should implement the dropzone attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ojan Vafai]

This was added to the spec in Nov 2010. http://www.whatwg.org/specs/web-apps/current-work/#the-dropzone-attribute

[Yael]

I am planning to work on this...

[Yael]

Created attachment 90859 [details]
Work in progress

This is work in progress, no layout tests yet.
Just wanted to get some feedback while I am testing.
thanks :)

[Hajime Morrita]

Comment on attachment 90859 [details]
Work in progress

View in context: https://bugs.webkit.org/attachment.cgi?id=90859&action=review

Hi, I tried to make some comment. Many of these will be pointless for your review-ready version though ;-)

In my preference, every code other than element traversal and dropZone value retrieval doen't need to be a part of EventHandler.
EventHandler is already complex enough and it will be good to keep this class small.

> Source/WebCore/page/EventHandler.cpp:1794
> +            if (equalIgnoringCase(keywords[i], "copy") || equalIgnoringCase(keywords[i], "move") || equalIgnoringCase(keywords[i], "link")) {

How about to define a string to enum conversion function?
This type checking facility looks a bit hacky.

> Source/WebCore/page/EventHandler.cpp:1815
> +                if (dragState().m_dragSrcIsDHTML)

How about to ask DragState to choose appropriate clipboard object?
The dragState object looks to know everything to decide it.

> Source/WebCore/page/EventHandler.cpp:1838
> +                for (unsigned int f = 0; f < clipboard->files()->length() && !hasType; f++) {

How about to ask hasType() to Clipboard?

> Source/WebCore/page/EventHandler.cpp:1849
> +            matched = true;

How about just break; ?

> Source/WebCore/page/EventHandler.cpp:1889
> +            if (!accept)

Why isn't findDropZone() a part of (or called inside) dispatchDragEvent() ?

[Ryosuke Niwa]

Comment on attachment 90859 [details]
Work in progress

View in context: https://bugs.webkit.org/attachment.cgi?id=90859&action=review

Exciting patch! I know you're not posting for a review but I concur with Morrita-san's points. The current definition of findDropZone is really long and should be split into smaller functions.

> Source/WebCore/ChangeLog:11
> +        element and continue processing the drag and drop operation.

It'll be nice to have a link to the relevant spec.

> Source/WebCore/page/EventHandler.cpp:1779
> +    Element* element = target->isElementNode() ? toElement(target) : target->parentElement();
> +    bool matched = false;
> +    for (; element && !matched; element = element->parentElement()) {

What if the hit test ended inside a shadow DOM?

> Source/WebCore/page/EventHandler.cpp:1809
> +            if (keywords[i].length() < 3)
> +                continue;
> +            if (keywords[i][1] != ':')
> +                continue;
> +            if (keywords[i][0] == 's')
> +                isFile = false;
> +            else if (keywords[i][0] == 'f')
> +                isFile = true;
> +            else
> +                continue;
> +            type = keywords[i].string().substring(2).lower();

I have no idea what this code is trying to do.  As Morrita-san said, you should extract this code into a helper function that convers string into an enum or bit flags.

> Source/WebCore/page/EventHandler.cpp:1812
> +            if (!isFile) {

It seems like things in if/else clauses can each be a separate function.

[Yael]

(In reply to comment #3)
> (From update of attachment 90859 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90859&action=review
> 
Thank you for reviewing
> Hi, I tried to make some comment. Many of these will be pointless for your review-ready version though ;-)
> 
> In my preference, every code other than element traversal and dropZone value retrieval doen't need to be a part of EventHandler.
> EventHandler is already complex enough and it will be good to keep this class small.
> 
Agree. I was waiting for rniwa's hacking session on EventHandler to see what changes need to happen.
> > Source/WebCore/page/EventHandler.cpp:1794
> > +            if (equalIgnoringCase(keywords[i], "copy") || equalIgnoringCase(keywords[i], "move") || equalIgnoringCase(keywords[i], "link")) {
> 

> How about to define a string to enum conversion function?
> This type checking facility looks a bit hacky.
> 
I started out with using the enum DropOperation, but then I decided to change because this is the only place it is used. I will change back.
> > Source/WebCore/page/EventHandler.cpp:1815
> > +                if (dragState().m_dragSrcIsDHTML)
> 
> How about to ask DragState to choose appropriate clipboard object?
> The dragState object looks to know everything to decide it.
> 
ok
> > Source/WebCore/page/EventHandler.cpp:1838
> > +                for (unsigned int f = 0; f < clipboard->files()->length() && !hasType; f++) {
> 

> How about to ask hasType() to Clipboard?
> 
ok
> > Source/WebCore/page/EventHandler.cpp:1849
> > +            matched = true;
> 
> How about just break; ?
> 
I can't just break because the drop operation could come after the matching type was found. I could probably break if aI added more checks throughout the function.
> > Source/WebCore/page/EventHandler.cpp:1889
> > +            if (!accept)
> 
> Why isn't findDropZone() a part of (or called inside) dispatchDragEvent() ?
We don't call findDropZone() if the event is DragLeave, Drag or Drop.

[Yael]

(In reply to comment #4)
> (From update of attachment 90859 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90859&action=review
> 
Thank you for reviewing.
> Exciting patch! I know you're not posting for a review but I concur with Morrita-san's points. The current definition of findDropZone is really long and should be split into smaller functions.
> 
ok
> > Source/WebCore/ChangeLog:11
> > +        element and continue processing the drag and drop operation.
> 
> It'll be nice to have a link to the relevant spec.
> 
ok
> > Source/WebCore/page/EventHandler.cpp:1779
> > +    Element* element = target->isElementNode() ? toElement(target) : target->parentElement();
> > +    bool matched = false;
> > +    for (; element && !matched; element = element->parentElement()) {
> 
> What if the hit test ended inside a shadow DOM?
> 
I assume that we will not find the dropzone attribute in an element that is part of shadow DOM, so it would be ok that calling shadowAncestorNode() at the top of updateDragAndDrop is taking us out of the shadow DOM.

> > Source/WebCore/page/EventHandler.cpp:1809
> > +            if (keywords[i].length() < 3)
> > +                continue;
> > +            if (keywords[i][1] != ':')
> > +                continue;
> > +            if (keywords[i][0] == 's')
> > +                isFile = false;
> > +            else if (keywords[i][0] == 'f')
> > +                isFile = true;
> > +            else
> > +                continue;
> > +            type = keywords[i].string().substring(2).lower();
> 
> I have no idea what this code is trying to do.  As Morrita-san said, you should extract this code into a helper function that convers string into an enum or bit flags.
> 
ok
> > Source/WebCore/page/EventHandler.cpp:1812
> > +            if (!isFile) {
> 
> It seems like things in if/else clauses can each be a separate function.
ok

[Yael]

Created attachment 91167 [details]
Patch.

Address comment #3 and comment #4 and add layout tests fr dragging
1. draggable element
2. image
3. link
4. file

[Eric Seidel (no email)]

Comment on attachment 91167 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=91167&action=review

> Source/WebCore/dom/Clipboard.h:95
> +        bool hasType(const String& type, bool isFile) const;

We generally prefer enums over bools.

> Source/WebCore/dom/Clipboard.h:108
> +        virtual String platformTypeForHTMLType(const String& type) const;

the named argument is redundant with the function name here.

[Enrica Casucci]

Comment on attachment 91167 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=91167&action=review

I have not looked at the tests yet, but I think the rest of the code requires some refactoring. Please consider my comments.

> Source/WebCore/dom/Clipboard.cpp:191
> +    

This method should be split in two separate methods, one for files and one for strings. It will make the code more readable and simplify the logic in processDropZoneKeyword.
Also, you don't need all the additional variables it and end, you can have everything in line in the for statement. You don't even need foundType, you can just use return true inside the for loop and return false outside.

> Source/WebCore/dom/Clipboard.cpp:201
> +

I think you should add a comment to indicate that different platforms could provide a platform specific implementation.
It is not clear to me if you are converting HTML types to platform types of vice-versa.

> Source/WebCore/page/EventHandler.cpp:1798
> +

I don't believe these two functions belong to EventHandler.cpp. They should go in Clipboard.cpp.

> Source/WebCore/page/EventHandler.cpp:1804
> +        return false;

I would suggest here:
if (keyword.length() < || keyword[1] != ':')
    return false;

> Source/WebCore/page/EventHandler.cpp:1820
> +    return processDropZoneKeywordKindString(clipboard, type);

Here I would get rid of isFile and rewrite it this way:
if (keyword[0] != 's' && keyword[1] != 'f')
    return false;

if (keyword[0] == 's' && clipboard->policy() == ClipboardNumb)
    return false;

return clipboard->hasType(keyword.substring(2).lower(), keyword[0] == 'f');

Then you can get eliminate the two methods below.

> Source/WebCore/page/EventHandler.cpp:1838
> +    bool matched = false;

This variable could be inside the for loop and not be used as control variable.

> Source/WebCore/page/EventHandler.h:249
> +        Clipboard* dragClipboardForDragState();

Where is this?

> Source/WebCore/page/EventHandler.h:323
> +    bool processDropZoneKeyword(Clipboard*, const String&);

Move to Clipboard

> Source/WebCore/page/EventHandler.h:325
> +    bool processDropZoneKeywordKindFile(Clipboard*, const String&);

I would remove them

[Daniel Cheng]

Comment on attachment 91167 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=91167&action=review

> Source/WebCore/dom/Clipboard.cpp:164
> +    String platformType = platformTypeForHTMLType(type);

Maybe you can just convert the type to lowercase and avoid having to call equalIgnoringCase.

"The drag data item type string
A Unicode string giving the type or format of the data, generally given by a MIME type. Some values that are not MIME types are special-cased for legacy reasons. The API does not enforce the use of MIME types; other values can be used as well. In all cases, however, the values are all converted to ASCII lowercase by the API."

I would consider it a bug if any implementation of Clipboard returned non-lowercased values for a file's type or from types().

> Source/WebCore/dom/Clipboard.cpp:167
> +        if (files()->isEmpty())

It would probably be better to call this once and hang on to the returned FileList. A lot of implementations create a new FileList every time you call files().

> Source/WebCore/dom/Clipboard.cpp:192
> +String Clipboard::platformTypeForHTMLType(const String& type) const

I do not think you need this function. From my reading of the spec, "Text" and "URL" are only supported via getData() / setData() for compatibility with IE. Since dropzone is not a legacy API, you should not need to support these values.

> Source/WebCore/page/EventHandler.cpp:1825
> +    if (clipboard->policy() == ClipboardNumb)

I don't think you should check ClipboardNumb here. Why not centralize all the checks in Clipboard::hasData() like the other Clipboard methods? For example, just several lines down, you don't check the access policy before calling clipboard->hasType().

[Yael]

(In reply to comment #10)
> (From update of attachment 91167 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91167&action=review
> 
> > Source/WebCore/dom/Clipboard.cpp:164
> > +    String platformType = platformTypeForHTMLType(type);
> 
> Maybe you can just convert the type to lowercase and avoid having to call equalIgnoringCase.
> 
> "The drag data item type string
> A Unicode string giving the type or format of the data, generally given by a MIME type. Some values that are not MIME types are special-cased for legacy reasons. The API does not enforce the use of MIME types; other values can be used as well. In all cases, however, the values are all converted to ASCII lowercase by the API."
> 
> I would consider it a bug if any implementation of Clipboard returned non-lowercased values for a file's type or from types().
> 
Filed https://bugs.webkit.org/show_bug.cgi?id=59606

[Yael]

Created attachment 91367 [details]
Patch.

Address review comments #8, #9 and #10.

[WebKit Review Bot]

Attachment 91367 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/dom/Clipboard.cpp:194:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/WebCore/dom/Clipboard.cpp:213:  Extra space between return and keyword  [whitespace/declaration] [3]
Source/WebCore/dom/Clipboard.h:125:  The parameter name "operation" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yael]

Created attachment 91375 [details]
Patch.

Fix style issue.

[Daniel Cheng]

Comment on attachment 91375 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=91375&action=review

> Source/WebCore/dom/Clipboard.cpp:164
> +    if (files()->isEmpty())

My comment about saving the return value of files() in a RefPtr still applies. If FileList has n entries, FileList will be created 2n + 1 times through this function if nothing matches.

> Source/WebCore/dom/Clipboard.cpp:176
> +    bool ok;

On some platforms, querying the types in a drag is not expensive but retrieving the data might be. It is probably better to use types().contains().

[Yael]

Created attachment 91608 [details]
Patch.

Address comment #15.

[Daniel Cheng]

Comment on attachment 91608 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=91608&action=review

I am not a WebKit reviewer, so these are just my thoughts.

> Source/WebCore/dom/Clipboard.cpp:162
> +bool Clipboard::hasFileOfType(const String& type) const

For consistency, should these new methods check that the policy of the clipboard matches what is expected?

> Source/WebCore/dom/Clipboard.cpp:168
> +    for (unsigned int f = 0; f < files()->length(); f++) {

You can use fileList here instead of calling files().

> Source/WebCore/dom/Clipboard.cpp:205
> +bool Clipboard::processDropZoneKeyword(const String& keyword)

Nit: I would keep the order of definitions the same as the order of declarations in the header file.

> Source/WebCore/dom/Clipboard.h:97
> +        bool hasStringOfType(const String&) const;

Should these methods be private since only Clipboard calls them?

> Source/WebCore/dom/Clipboard.h:98
> +        bool processDropZoneKeyword(const String&);

This method might be more clearly named hasDropZoneType.

> Source/WebCore/page/EventHandler.cpp:1796
> +                    clipboard->setDropEffect(convertDragOperationToDropZoneOperation(dragOperation));

It seems redundant to have call Clipboard::setDropEffect in 3 places. Can you consolidate it into one return point for the successfully matched case?

[Yael]

Created attachment 91672 [details]
Patch.

Address comment #17.

[Ryosuke Niwa]

Comment on attachment 91672 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=91672&action=review

> Source/WebCore/dom/Clipboard.cpp:219
> +    if (keyword[0] != 's' && keyword[0] != 'f')
> +        return false;
> +    
> +    return keyword[0] == 'f' ? hasFileOfType(keyword.substring(2).lower()) : hasStringOfType(keyword.substring(2).lower());

I'd do:
switch (keyboard[0]) {
case 'f':
    return hasFileOfType(keyword.substring(2).lower());
case 's':
    return hasStringOfType(keyword.substring(2).lower());
default:
    return false;
}

> Source/WebCore/page/EventHandler.cpp:1774
> +bool EventHandler::findDropZone(Node* target, Clipboard* clipboard)

Can this function be static local?  In general, it's nice to avoid adding new functions to already bloated EventHandler.

> Source/WebCore/page/EventHandler.cpp:1779
> +        String type;

Is this variable ever used?

> Source/WebCore/page/EventHandler.cpp:1780
> +        DragOperation dragOperation = DragOperationNone;

I'd declare dragOperation right before the for loop because it's not used before the for loop.

> Source/WebCore/page/EventHandler.cpp:1792
> +            if (matched && dragOperation != DragOperationNone)
> +                break;

Can we check this condition inside if (dragOperation == DragOperationNone) instead?  That'll make the purpose of early exit much clear.

> Source/WebCore/page/EventHandler.cpp:1797
> +                if (dragOperation == DragOperationNone)
> +                    dragOperation = op;

So you do:
if (dragOperation == DragOperationNone) {
    dragOperation = op;
    if (matched)
        break;
}

> LayoutTests/fast/events/dropzone-001.html:1
> +<html>

Missing DOCTYPE.

> LayoutTests/fast/events/dropzone-001.html:11
> +    var dragMe;

I don't think we normally indent script contents like this.  It bloats the file size.

> LayoutTests/fast/events/dropzone-002.html:43
> +    function changeDropZone()
> +    {
> +        dropTarget.setAttribute("dropzone", dropEffectElem.options[dropEffectElem.selectedIndex].value + " s:text/plain");
> +    }
> +
> +    function drop(e)
> +    {
> +        printDropEvent(e);
> +        cancelDrag(e);
> +    }

Can we add a new js file in resources and share all of these functions across tests?

[Yael]

(In reply to comment #19)
> (From update of attachment 91672 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91672&action=review
> > Source/WebCore/page/EventHandler.cpp:1792
> > +            if (matched && dragOperation != DragOperationNone)
> > +                break;
> 
> Can we check this condition inside if (dragOperation == DragOperationNone) instead?  That'll make the purpose of early exit much clear.
> 
> > Source/WebCore/page/EventHandler.cpp:1797
> > +                if (dragOperation == DragOperationNone)
> > +                    dragOperation = op;
> 
> So you do:
> if (dragOperation == DragOperationNone) {
>     dragOperation = op;
>     if (matched)
>         break;
> }
> 
I'll move the condition as you asked, but please note that there are 3 conditions that require me to break and I have to allow all of them :
1. operation is defined before type
2. type is defined before operation
3. operation is not defined at all.

> > LayoutTests/fast/events/dropzone-002.html:43
> > +    function changeDropZone()
> > +    {
> > +        dropTarget.setAttribute("dropzone", dropEffectElem.options[dropEffectElem.selectedIndex].value + " s:text/plain");
> > +    }
> > +
> > +    function drop(e)
> > +    {
> > +        printDropEvent(e);
> > +        cancelDrag(e);
> > +    }
> 
> Can we add a new js file in resources and share all of these functions across tests?

Only 2 functions are shared between all the files and can be isolated. All the other functions have small different differences. I'll separate out 
drop() and cancelDrag().

[Yael]

Created attachment 91752 [details]
Patch.

Address comment #19.

[Daniel Cheng]

Comment on attachment 91752 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=91752&action=review

> Source/WebCore/page/EventHandler.cpp:1795
> +                        break;

As discussed on IRC, this code can be restructured a bit to combine the 2 early breaks, continue, and early return into just one early return. That way we only have to check for termination conditions in one block.

[Yael]

Created attachment 92175 [details]
Patch.

Address dcheng's comment #22.

[Ryosuke Niwa]

Unfortunately, I don't know enough about drag & drop to r+ this patch.

[WebKit Review Bot]

Attachment 92175 [details] did not pass chromium-ews:
Output: http://queues.webkit.org/results/8552714
New failing tests:
fast/events/dropzone-001.html
fast/events/dropzone-002.html

[Tony Chang]

Comment on attachment 92175 [details]
Patch.

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.

[Yael]

(In reply to comment #26)
> (From update of attachment 92175 [details])
> 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:
> 
Thank you for the review. I'll e-mail the HTML mailing list to start a discussion.
Before starting this work, I searched archives to find any prior discussion about this feature and did not find much.
If you have a pointer to any such discussions, I will appreciate it.

[Tony Chang]

(In reply to comment #27)
> (In reply to comment #26)
> > (From update of attachment 92175 [details] [details])
> > 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:
> > 
> Thank you for the review. I'll e-mail the HTML mailing list to start a discussion.
> Before starting this work, I searched archives to find any prior discussion about this feature and did not find much.
> If you have a pointer to any such discussions, I will appreciate it.

I wasn't able to find much discussion either, which is why I think the spec is a little early.  The issues are relatively minor, but it should be easy to get them resolved before committing an implementation.

Here's one message about dropzone which also seems valid, but is less of a concern to me:
  http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2011-January/030138.html

[Yael]

(In reply to comment #26)
> (From update of attachment 92175 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92175&action=review

e-mail was sent http://old.nabble.com/Comments-on-the-dropzone-attributes-tt31599376.html.
While this is being discussed on the mailing list, I'll add the webkit prefix.
thanks.

[Yael]

Created attachment 93439 [details]
Patch.

Address coding part of comment #26 .
This patch will introduce the behavior as it is specified today, but with the webkit prefix.
Once there is an agreement on if and how to change the behavior, I will update the code.

[WebKit Review Bot]

Comment on attachment 93439 [details]
Patch.

Attachment 93439 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8695107

New failing tests:
fast/events/dropzone-001.html
fast/events/dropzone-002.html

[WebKit Review Bot]

Created attachment 93446 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Yael]

Created attachment 93541 [details]
Patch.

(In reply to comment #31)
> (From update of attachment 93439 [details])
> Attachment 93439 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/8695107
> 
> New failing tests:
> fast/events/dropzone-001.html
> fast/events/dropzone-002.html

Chromium's implementation of Clipboard::setData() in ClipboardObjectData.cpp does not allow arbitrary mime types, and it treats images being dragged as type "url". I made adjustments to the tests to take this into account.

[WebKit Review Bot]

Comment on attachment 93541 [details]
Patch.

Attachment 93541 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8699248

New failing tests:
fast/events/dropzone-002.html

[WebKit Review Bot]

Created attachment 93545 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Yael]

(In reply to comment #34)
> (From update of attachment 93541 [details])
> Attachment 93541 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/8699248
> 
> New failing tests:
> fast/events/dropzone-002.html

What command is the bot using for runing the tests?
When I used Tools/Scripts/new-run-webkit-tests on my Linux chromium build, this test did pass, so I need to know how to reproduce the failure.

[Hajime Morrita]

> What command is the bot using for runing the tests?
> When I used Tools/Scripts/new-run-webkit-tests on my Linux chromium build, this test did pass, so I need to know how to reproduce the failure.

You can use new-run-webkit-tests or DumpRenderTree directly (you can pass
to the full-path of the html.)

I can reproduce the problem. but I couldn't figure out why yet.
It looks Chromium's drag-drop code goes somewhat different path. I'm not sure what happens though.

[Tony Chang]

Comment on attachment 93541 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=93541&action=review

Please try to figure out why cr-linux is failing or add it to LayoutTests/platform/chromium/test_expectations.txt.  Something like:
BUGWKXXX : fast/events/dropzone-002.html = TEXT

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

Nit: Maybe add an ASSERT_NOT_REACHED()?

> Source/WebCore/page/EventHandler.cpp:1784
> +    for (; element; element = element->parentElement()) {

I think this is fine for now, but we may want to profile how much time we spend parsing the dropzone attribute since we do this on every mousemove during a drag.  If it's a lot of time, we may want to cache the parsed result.

[Yael]

(In reply to comment #38)
> (From update of attachment 93541 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93541&action=review
> 
> Please try to figure out why cr-linux is failing or add it to LayoutTests/platform/chromium/test_expectations.txt.  Something like:
> BUGWKXXX : fast/events/dropzone-002.html = TEXT
>
This test is not failing for me, so it is hard to debug.
Filed  https://bugs.webkit.org/show_bug.cgi?id=61625.

> > Source/WebCore/dom/Clipboard.cpp:207
> > +    default:
> > +        return String("copy");
> 
> Nit: Maybe add an ASSERT_NOT_REACHED()?
> 
> > Source/WebCore/page/EventHandler.cpp:1784
> > +    for (; element; element = element->parentElement()) {
> 
This code is reachable if "copy", "move" or "link" are not specified.

[Yael]

Committed r87499: <http://trac.webkit.org/changeset/87499>

[Olli Pettay (:smaug)]

Did you actually review the spec when implementing this?
Since I'll propose changes to the spec before implementing
dropzone in Gecko.
The f: and s: thing is just strange and dropzone should be split two attributes etc.

[Yael]

(In reply to comment #41)
> Did you actually review the spec when implementing this?
> Since I'll propose changes to the spec before implementing
> dropzone in Gecko.
> The f: and s: thing is just strange and dropzone should be split two attributes etc.

See comment #29 about f: and s: .
I already sent a comment to whatwg mailing list about that, and will change the implementation when the spec changes.

[Ian 'Hixie' Hickson]

I updated the spec to use file: and string: rather than f: and s:. Sorry it took so long.

[Yael]

(In reply to comment #43)
> I updated the spec to use file: and string: rather than f: and s:. Sorry it took so long.

Created https://bugs.webkit.org/show_bug.cgi?id=74834 for this change.