[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 #6 from Yael <yael.aharon at nokia.com>  2011-04-23 18:53:59 PST ---
(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

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