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

Fri Apr 29 10:01:35 PDT 2011

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

--- Comment #19 from Ryosuke Niwa <rniwa at webkit.org>  2011-04-29 10:01:34 PST ---
(From update of attachment 91672)
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?

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