Bug 60503 – Refactor RenderObject::draggableNode

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Daniel Cheng]

RenderObject::draggableNode is in an awkward location. I'm trying to refactor the drag start handling, and if I keep the existing structure, I have to add three new functions to EventHandler whose only purpose is to forward calls to three new functions in DragController. Given that, it seems much more sense to move the determination of the node to drag into DragController. I'm also pulling DragState out into its own header so I can share the drag state between EventHandler and DragController in the future.

[Daniel Cheng]

Created attachment 92851 [details]
Patch

[Daniel Cheng]

Created attachment 92852 [details]
Patch

[Tony Chang]

Comment on attachment 92852 [details]
Patch

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

I mentioned to dcheng that the previous code walked up RenderObjects and this code walks up DOM nodes.  I think this is equivalent because anonymous RenderObjects don't influence drag behavior, but I'm not sure about things like absolute or fixed position element.

> Source/WebCore/page/DragController.cpp:572
> +Node* DragController::draggableNode(const Frame* src, Node* node, bool dhtmlOK, bool uaOK, int x, int y, bool& dhtmlWillDrag) const

src -> source or sourceFrame or frame.

> Source/WebCore/page/DragController.cpp:599
> +            if (uaOK && dragMode == DRAG_AUTO
> +                    && mayStartDragAtEventLocation(src, IntPoint(x, y), node)) {

Nit: This indention looks off.  Maybe just put it on a single line.

> Source/WebCore/page/DragController.h:78
> +        Node* draggableNode(const Frame*, Node*, bool dhtmlOK, bool uaOK, int x, int y, bool& dhtmlWillDrag) const;

Can we rename uaOK to userAgentOK?

> Source/WebCore/page/EventHandler.h:-149
> -    bool shouldDragAutoNode(Node*, const IntPoint&) const; // -webkit-user-drag == auto

Which layout tests cover -webkit-user-drag == auto?  They still pass, right?

[Tony Chang]

Adding some people to provide feedback about climbing the render tree vs climbing the dom tree.

[Daniel Cheng]

(In reply to comment #3)
> (From update of attachment 92852 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92852&action=review
> 
> I mentioned to dcheng that the previous code walked up RenderObjects and this code walks up DOM nodes.  I think this is equivalent because anonymous RenderObjects don't influence drag behavior, but I'm not sure about things like absolute or fixed position element.
> 
> > Source/WebCore/page/DragController.cpp:572
> > +Node* DragController::draggableNode(const Frame* src, Node* node, bool dhtmlOK, bool uaOK, int x, int y, bool& dhtmlWillDrag) const
> 
> src -> source or sourceFrame or frame.
> 

There is a mixed convention between using src and source in DragController. I opted to use src for consistency since all references to source Frames use the identifier src in DragController.

> > Source/WebCore/page/DragController.cpp:599
> > +            if (uaOK && dragMode == DRAG_AUTO
> > +                    && mayStartDragAtEventLocation(src, IntPoint(x, y), node)) {
> 
> Nit: This indention looks off.  Maybe just put it on a single line.
> 
> > Source/WebCore/page/DragController.h:78
> > +        Node* draggableNode(const Frame*, Node*, bool dhtmlOK, bool uaOK, int x, int y, bool& dhtmlWillDrag) const;
> 
> Can we rename uaOK to userAgentOK?
> 

I opted for a straight copy and paste from RenderObject with only fixes as necessary to prevent check-webkit-style from complaining. I am planning on rewriting most of this block anyway. If you feel strongly about it, I will update this patch.

> > Source/WebCore/page/EventHandler.h:-149
> > -    bool shouldDragAutoNode(Node*, const IntPoint&) const; // -webkit-user-drag == auto
> 
> Which layout tests cover -webkit-user-drag == auto?  They still pass, right?

Yes. There's a couple. user-drag-none is the primary one.

[James Robinson]

Comment on attachment 92852 [details]
Patch

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

Do you need to refactor and change the implementation at the same time?

> Source/WebCore/page/DragState.h:45
> +    bool m_dragSrcIsLink;
> +    bool m_dragSrcIsImage;
> +    bool m_dragSrcInSelection;
> +    bool m_dragSrcMayBeDHTML;
> +    bool m_dragSrcMayBeUA; // Are DHTML and/or the UserAgent allowed to drag out?
> +    bool m_dragSrcIsDHTML;

this looks a lot like it should be an enum

[Daniel Cheng]

Created attachment 92894 [details]
Patch

[Daniel Cheng]

(In reply to comment #6)
> (From update of attachment 92852 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92852&action=review
> 
> Do you need to refactor and change the implementation at the same time?
> 
> > Source/WebCore/page/DragState.h:45
> > +    bool m_dragSrcIsLink;
> > +    bool m_dragSrcIsImage;
> > +    bool m_dragSrcInSelection;
> > +    bool m_dragSrcMayBeDHTML;
> > +    bool m_dragSrcMayBeUA; // Are DHTML and/or the UserAgent allowed to drag out?
> > +    bool m_dragSrcIsDHTML;
> 
> this looks a lot like it should be an enum

Probably. In my refactored version, it has been changed. As I mentioned though, this is a straight move of the code from RenderObject with fixes as necessary for compile/stylechecker reasons.

[Tony Chang]

Comment on attachment 92894 [details]
Patch

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

Yes, this looks just like a refactor.

> Source/WebCore/page/DragController.cpp:598
> +            if (uaOK && dragMode == DRAG_AUTO
> +                    && mayStartDragAtEventLocation(src, IntPoint(x, y), node)) {

I think it's ok to do indention fixes when refactoring.  I mean, you do that when you pull out functions, etc.

[Daniel Cheng]

Committed r86128: <http://trac.webkit.org/changeset/86128>