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.
Created attachment 92851 [details] Patch
Created attachment 92852 [details] Patch
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?
Adding some people to provide feedback about climbing the render tree vs climbing the dom tree.
(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 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
Created attachment 92894 [details] Patch
(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 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.
Committed r86128: <http://trac.webkit.org/changeset/86128>