Bug 60503 - Refactor RenderObject::draggableNode
Summary: Refactor RenderObject::draggableNode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Cheng
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-09 14:16 PDT by Daniel Cheng
Modified: 2011-05-09 21:33 PDT (History)
4 users (show)

See Also:


Attachments
Patch (19.04 KB, patch)
2011-05-09 14:28 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (20.05 KB, patch)
2011-05-09 14:36 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (20.08 KB, patch)
2011-05-09 17:50 PDT, Daniel Cheng
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Cheng 2011-05-09 14:16:10 PDT
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.
Comment 1 Daniel Cheng 2011-05-09 14:28:32 PDT
Created attachment 92851 [details]
Patch
Comment 2 Daniel Cheng 2011-05-09 14:36:14 PDT
Created attachment 92852 [details]
Patch
Comment 3 Tony Chang 2011-05-09 15:12:03 PDT
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?
Comment 4 Tony Chang 2011-05-09 15:12:43 PDT
Adding some people to provide feedback about climbing the render tree vs climbing the dom tree.
Comment 5 Daniel Cheng 2011-05-09 15:19:34 PDT
(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.
Comment 6 James Robinson 2011-05-09 16:15:53 PDT
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
Comment 7 Daniel Cheng 2011-05-09 17:50:38 PDT
Created attachment 92894 [details]
Patch
Comment 8 Daniel Cheng 2011-05-09 17:51:35 PDT
(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.
Comment 9 Tony Chang 2011-05-09 19:42:06 PDT
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.
Comment 10 Daniel Cheng 2011-05-09 21:33:34 PDT
Committed r86128: <http://trac.webkit.org/changeset/86128>