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

[bugzilla-daemon at webkit.org]

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





--- Comment #9 from Enrica Casucci <enrica at apple.com>  2011-04-26 16:58:51 PST ---
(From update of attachment 91167)
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

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