Per the FIXME in the code: EventHandler::handleGestureEvent should hit test and send ScrollBeginType, ScrollUpdateType and ScrollEndType events to the correct subframe rather than always sending gestures to the main frame only. It should also ensure that if a frame gets a gesture begin gesture, it gets the corresponding end gesture as well.
Created attachment 106903 [details] Patch
Hi Adam, The attached patch works for Chrome but still needs some cleanup and multuple layout tests. I'm hoping you could give me some high-level feedback on the approach and suggest who I should get to review the final version. Thanks.
Comment on attachment 106903 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106903&action=review I didn't review the whole patch, but this needs a bunch of work. I'm sorry that I don't have time to help you with this patch. You might try dglazkov. He's done a bunch of work with event handling. > Source/WebCore/page/EventHandler.cpp:2206 > + Document* doc = m_frame->document(); This looks dangerous. This point could become junk while dispatching events. It's probably best to grab the document point from the frame each time you need it. > Source/WebCore/page/EventHandler.cpp:2226 > + if (m_gestureEventWidgetTarget.get() && m_gestureEventWidgetTarget->refCount() > 1 && passGestureEventToWidget(gestureEvent, m_gestureEventWidgetTarget.get())) Crazy. It's very rare to branch on the refCount of an object. I suspect this isn't the right thing to do. > Source/WebCore/page/EventHandler.cpp:2228 > + if (m_gestureEventWidgetTarget.get() && m_gestureEventWidgetTarget->hasOneRef()) No need to call get() to convert a RefPtr to a bool. > Source/WebCore/page/EventHandler.cpp:2232 > + RenderLayer* layer = ((RenderBox*)m_gestureTargetNode->renderer())->layer(); Please use C++ style casts. How do you know its a RenderBox?? > Source/WebCore/page/EventHandler.cpp:2249 > + LayoutPoint vPoint = view->windowToContents(gestureEvent.position()); vPoint => please use full words when naming variables. > Source/WebCore/page/EventHandler.cpp:2251 > + Node* node; > + bool isOverWidget; Please initialize scalars. > Source/WebCore/page/EventHandler.cpp:2257 > + node = result.innerNode(); Why not just declare node at this point? Then we don't need to worry about someone using it between when it is declared and when it is initialized. Also, consider taking a reference to the node so that it doesn't get deallocated out from under you. > Source/WebCore/page/EventHandler.cpp:2260 > + if (node) { Prefer early return. > Source/WebCore/page/EventHandler.cpp:2269 > + node = node->shadowAncestorNode(); Why are we re-using the same variable? /me is confused. > Source/WebCore/page/EventHandler.cpp:2281 > + while (target && !target->isRoot()) { > + if (target->isBox()) { > + RenderBox* rb = (RenderBox*)target; > + if (rb->canBeScrolledAndHasScrollableArea()) { > + m_gestureTargetNode = rb->node(); > + rb->layer()->handleGestureEvent(gestureEvent); > + return true; // FIXME: Prove that this is correct. > + } > + } > + target = target->parent(); This looks like a lot of render tree manipulation to be doing outside of the rendering directory.
Hi Adam, Thanks for the useful feedback. I know it still needs a lot of work -- I was looking for this kind of early feedback that will stop me going down the wrong rabbit hole. I'll ask Dimitri about taking a look but maybe you'd have time for some quick follow up questions to your review -- inlined in your comments.
> inlined in your comments. I don't see your comments. You might need to "publish" them for other folks to be able to see them.
(In reply to comment #5) > > inlined in your comments. > > I don't see your comments. You might need to "publish" them for other folks to be able to see them. Indeed. I forgot to hit publish in another browser window.
Comment on attachment 106903 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106903&action=review >> Source/WebCore/page/EventHandler.cpp:2226 >> + if (m_gestureEventWidgetTarget.get() && m_gestureEventWidgetTarget->refCount() > 1 && passGestureEventToWidget(gestureEvent, m_gestureEventWidgetTarget.get())) > > Crazy. It's very rare to branch on the refCount of an object. I suspect this isn't the right thing to do. my aim was to deal with the case where JS execution disappeared the node between successive PlatformGestureEvents. Is it reasonable to follow the example of how m_latchedWheelEventNode is handled (I plan on having a layout test for the case of ScrollStart ScrollUpdate* <remove target> ScrollEnd) >> Source/WebCore/page/EventHandler.cpp:2232 >> + RenderLayer* layer = ((RenderBox*)m_gestureTargetNode->renderer())->layer(); > > Please use C++ style casts. How do you know its a RenderBox?? Because I only set m_gestureTargetNode if its renderer() was a RenderBox instance. >> Source/WebCore/page/EventHandler.cpp:2269 >> + node = node->shadowAncestorNode(); > > Why are we re-using the same variable? /me is confused. I'll ask Dimitri about the correct way of shadowdom. I was copying from EventHandler::handleWheelEvent. >> Source/WebCore/page/EventHandler.cpp:2281 >> + target = target->parent(); > > This looks like a lot of render tree manipulation to be doing outside of the rendering directory. Having a GestureEventWithHitTestResult similar to MouseEventWithHitTestResult would be the right encapsulation of this search? It would reside in /rendering and would appear to get most/all of the necessary functionality from HitTestResult.
Created attachment 111852 [details] Patch
Hi Dimitri, per Adam's suggestion, could you have a look at this and give me some feedback on approach/further issues to address?
Comment on attachment 111852 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111852&action=review > Source/WebCore/page/EventHandler.cpp:2225 > + bool isOverWidget = result.isOverWidget(); This can move below the !node check. > Source/WebCore/page/EventHandler.cpp:2238 > + node = node->shadowAncestorNode(); What are you trying to do there? Can you explain a bit more? > Source/WebCore/page/EventHandler.cpp:2259 > + RenderBox* enclosingBox = renderer->enclosingBox(); > + while (enclosingBox && !enclosingBox->canBeScrolledAndHasScrollableArea()) { > + if (!enclosingBox->parent()) > + return false; > + enclosingBox = enclosingBox->parent()->enclosingBox(); > + } > + > + if (enclosingBox && enclosingBox->node() != enclosingBox->document()) { > + RenderLayer* layer = enclosingBox->layer(); > + if (layer) { > + layer->handleGestureEvent(gestureEvent); > + return true; > + } > + } looks like this should be something like enclosingScrollableLayer. I bet we already have code that does this. > Source/WebCore/page/EventHandler.cpp:2271 > +// REVIEWER: I hunted for something like this in wtf::. Does it exist? If not, > +// is it worth adding a generic version? > +// Helper to auto-unlatch on end of scope. Can you explain why you would need something like this?
Comment on attachment 111852 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111852&action=review >> Source/WebCore/page/EventHandler.cpp:2225 >> + bool isOverWidget = result.isOverWidget(); > > This can move below the !node check. done, next patch. >> Source/WebCore/page/EventHandler.cpp:2238 >> + node = node->shadowAncestorNode(); > > What are you trying to do there? Can you explain a bit more? I was following along with how mouse wheel events would seem to be sent into the shadow DOM -- http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/page/EventHandler.cpp&exact_package=chromium&q=EventHandler&type=cs&l=2184 was the inspiration. Reasonable or foolish? >> Source/WebCore/page/EventHandler.cpp:2259 >> + } > > looks like this should be something like enclosingScrollableLayer. I bet we already have code that does this. I tried using enclosingScrollableLayer and went on a voyage of discovery. In particular: it does not want to be used from EventHandler. (I would think I was attempting to create a layering violation.) So, I ended up dispatching the events a different way that seems "more correct" -- the code ended up shorter and simpler in the next patch. >> Source/WebCore/page/EventHandler.cpp:2271 >> +// Helper to auto-unlatch on end of scope. > > Can you explain why you would need something like this? to always clear the RefPtr on leaving the enclosing scope in handleGestureEvent so that I don't preserve an unnecessary reference to the target node for the Scroll* sequence after the concluding ScrollEndType event.
Created attachment 112607 [details] Patch
Please take another look. Notes: * rniwa: added per Dimitri's suggestion. Would you be a more appropriate reviewer for this change? * patch is not complete. Am looking for feedback on the approach/C++ code. The layout tests are not done yet to my satisfaction (they so far only show that the implementation is correct on Chromium) and there are still some missing bits for non-Chromium code paths.
Comment on attachment 112607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112607&action=review > Source/WebCore/ChangeLog:7 > + You need to explain what kind of changes you're making. > Source/WebCore/ChangeLog:16 > + (WebCore::wheelGranularityToScrollGranularity): > + Helper method. > + (WebCore::scrollNode): > + Refactored. Please put the description on the same line as the function name after :. > Source/WebCore/page/EventHandler.cpp:178 > +static inline bool gestureNode(const PlatformGestureEvent& e, Node* node) Please don't use one-letter variable like e. > Source/WebCore/page/EventHandler.cpp:2238 > +bool EventHandler::passGestureEventToWidgetHelper(PassRefPtr<Node> targetNode, const PlatformGestureEvent& gestureEvent, const HitTestResult& result) Why does this function need to be a member function of Event Handler? It can be a static non-member function, no? Why is this function called "helper"? We need a more descriptive name. > Source/WebCore/page/EventHandler.cpp:2249 > + if (isOverWidget && target && target->isWidget()) { > + Widget* widget = toRenderWidget(target)->widget(); > + if (widget && passGestureEventToWidget(gestureEvent, widget)) We prefer early exit over nested ifs. > Source/WebCore/page/EventHandler.cpp:2255 > +bool EventHandler::passGestureEventToFameView(const PlatformGestureEvent& gestureEvent) Ditto. Also, typo: Fame -> Frame. > Source/WebCore/page/EventHandler.cpp:2267 > +// Helper to always unlatch target node on leaving scope. > +class Unlatcher { > +public: What's the point of this class? I don't understand what you mean by "unlatch". Can't you just use RefPtr? What's the point of wrapping it with a class? > Source/WebCore/page/EventHandler.cpp:2283 > + Document* doc = m_frame->document(); Please don't use abbreviations like doc. > Source/WebCore/page/EventHandler.cpp:2292 > + LayoutPoint vPoint = view->windowToContents(gestureEvent.position()); What does v stand for? Please don't use abbreviations. > Source/WebCore/page/EventHandler.cpp:2303 > + // FIXME: Refactor this code to not hit test multiple times by using the code > + // written for scroll. Why are you splitting this comment into 2 lines? It fit the line perfectly. > Source/WebCore/page/EventHandler.cpp:2320 > + bool xscroll = node && scrollNode(gestureEvent.deltaX(), ScrollByPixel, ScrollLeft, ScrollRight, node, (Node**)0); Nit: camelCase. xScroll, and s/(Node**)0/0/; > Source/WebCore/page/EventHandler.cpp:2321 > + bool yscroll = node && scrollNode(gestureEvent.deltaY(), ScrollByPixel, ScrollUp, ScrollDown, node, (Node**)0); Ditto. > Source/WebCore/platform/ScrollAnimator.cpp:138 > + float deltaX = (event.deltaX() < 0.f) ? max(event.deltaX(), -float(maxForwardScrollDelta.width())) : min(event.deltaX(), float(maxBackwardScrollDelta.width())); Nit: two spaces before -float. Also, please use C++-style casting e.g. static_cast<float>(). > Source/WebCore/rendering/RenderBox.cpp:677 > + fprintf(stderr, "RenderBox::gesture\n"); What's up with this fprintf? > Source/WebCore/rendering/RenderBox.cpp:678 > + RenderLayer* layer = layer(); I'm surprised that this didn't cause a compilation error. r- because this is certainly going to cause a compilation failure on one of ports.
Created attachment 112759 [details] Patch
Comment on attachment 112607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112607&action=review >> Source/WebCore/ChangeLog:7 >> + > > You need to explain what kind of changes you're making. done in p4 >> Source/WebCore/ChangeLog:16 >> + Refactored. > > Please put the description on the same line as the function name after :. done in p4 >> Source/WebCore/page/EventHandler.cpp:178 >> +static inline bool gestureNode(const PlatformGestureEvent& e, Node* node) > > Please don't use one-letter variable like e. done, p4 >> Source/WebCore/page/EventHandler.cpp:2238 >> +bool EventHandler::passGestureEventToWidgetHelper(PassRefPtr<Node> targetNode, const PlatformGestureEvent& gestureEvent, const HitTestResult& result) > > Why does this function need to be a member function of Event Handler? It can be a static non-member function, no? > > Why is this function called "helper"? We need a more descriptive name. done, p4 >> Source/WebCore/page/EventHandler.cpp:2249 >> + if (widget && passGestureEventToWidget(gestureEvent, widget)) > > We prefer early exit over nested ifs. done, p4 >> Source/WebCore/page/EventHandler.cpp:2255 >> +bool EventHandler::passGestureEventToFameView(const PlatformGestureEvent& gestureEvent) > > Ditto. Also, typo: Fame -> Frame. done, p4. autocomplete for the non-win. >> Source/WebCore/page/EventHandler.cpp:2267 >> +public: > > What's the point of this class? I don't understand what you mean by "unlatch". > > Can't you just use RefPtr? What's the point of wrapping it with a class? Background: handleGestureEvent receives a sequence of ScrollDownType, ScrollUpdateType*, ScrollEndType PGE sub-events. The ScrollDownType event chooses the target node and "latches" it in m_gestureTargetNode. Why "latch": from the language in the WheelEvent handling code. The following ScrollUpdateType and ScrollEndType events all go to the original latching node (assuming it still exists.) The latching node (stored in m_gestureTargetNode) should always be cleared on an ScrollEndType event even in the presence of early return from the containing block. Unlatcher does that. If there is a better way, please let me know. >> Source/WebCore/page/EventHandler.cpp:2283 >> + Document* doc = m_frame->document(); > > Please don't use abbreviations like doc. fixed p4 >> Source/WebCore/page/EventHandler.cpp:2292 >> + LayoutPoint vPoint = view->windowToContents(gestureEvent.position()); > > What does v stand for? Please don't use abbreviations. done in p4 >> Source/WebCore/page/EventHandler.cpp:2303 >> + // written for scroll. > > Why are you splitting this comment into 2 lines? It fit the line perfectly. done in p4 >> Source/WebCore/page/EventHandler.cpp:2320 >> + bool xscroll = node && scrollNode(gestureEvent.deltaX(), ScrollByPixel, ScrollLeft, ScrollRight, node, (Node**)0); > > Nit: camelCase. xScroll, and s/(Node**)0/0/; done in p4. yScroll too. >> Source/WebCore/platform/ScrollAnimator.cpp:138 >> + float deltaX = (event.deltaX() < 0.f) ? max(event.deltaX(), -float(maxForwardScrollDelta.width())) : min(event.deltaX(), float(maxBackwardScrollDelta.width())); > > Nit: two spaces before -float. Also, please use C++-style casting e.g. static_cast<float>(). done, p4 >> Source/WebCore/rendering/RenderBox.cpp:677 >> + fprintf(stderr, "RenderBox::gesture\n"); > > What's up with this fprintf? removed. >> Source/WebCore/rendering/RenderBox.cpp:678 >> + RenderLayer* layer = layer(); > > I'm surprised that this didn't cause a compilation error. r- because this is certainly going to cause a compilation failure on one of ports. it did. fixed in p4
Please take another look.
Created attachment 120475 [details] Patch
Comment on attachment 120475 [details] Patch Attachment 120475 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11027213
Comment on attachment 120475 [details] Patch Attachment 120475 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11017407
Comment on attachment 120475 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120475&action=review > Source/WebCore/platform/ScrollAnimator.h:120 > +#if ENABLE(GESTURE_EVENTS) > + int m_gestureEventTestRecord; > +#endif It doesn't seem like a good idea to be adding state to the implementation that is only used for testing. Perhaps I misunderstand what this is being used for.
Comment on attachment 120475 [details] Patch Attachment 120475 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11017426
Comment on attachment 120475 [details] Patch Attachment 120475 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11017419
Comment on attachment 120475 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120475&action=review >> Source/WebCore/platform/ScrollAnimator.h:120 >> +#endif > > It doesn't seem like a good idea to be adding state to the implementation that is only used for testing. Perhaps I misunderstand what this is being used for. Sam: This is what it was for. Since gesture events don't all reach JavaScript, I needed some way to prove that the implementation was correct. A solution that occurred to me would be to create a testing subclass of ScrollAnimator for use in DumpRenderTree. Does this sound reasonable? Or is there an easier way? BTW: given that this patch addresses your FIXME, do you have any other general advice for improving the patch?
Created attachment 121315 [details] Patch
Ryosuke: could you take another look please? I've addressed weinig's comment but not perhaps in the nicest of ways.
Comment on attachment 121315 [details] Patch Attachment 121315 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11122059
Comment on attachment 121315 [details] Patch Attachment 121315 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11147086
Created attachment 121327 [details] Patch
Improved include guards.
Comment on attachment 121327 [details] Patch Attachment 121327 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11147119
Created attachment 121332 [details] Patch
Comment on attachment 121332 [details] Patch Attachment 121332 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11146211
Comment on attachment 121332 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121332&action=review At least some of the code this patch is patching was deleted a bit back, so this doesn't apply. If we plan to proceed with this patch I think you need to figure out how it fits in now. > Source/WebCore/platform/ScrollAnimator.cpp:136 > +void ScrollAnimator::handleGestureEvent(const PlatformGestureEvent& event) this was deleted in http://trac.webkit.org/changeset/107823
Created attachment 129738 [details] Patch
Comment on attachment 129738 [details] Patch Attachment 129738 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11757589
Comment on attachment 129738 [details] Patch We discussed this offline. I think we'll be better off if ScrollView / ScrollAnimator / etc do not have to be explicitly aware of GestureEvents, but that we translate the gesture into a logical scroll earlier in the pipeline and then pass that through.
per a discussion with jamesr: I'm going to simplify this patch and divide it into three sub-patches that: * improve chromium EventSender::gestureEvent to send fully-populated GestureScroll* gestures. * show that interim version in ToT using mouse wheel events works correctly for GestureScrollUpdate messages. * change to EventHandler::handleGestureEvent to dispatch fling as an additional kind of scroll rather than the approach that I took here. I'll leave this bug to track.
Obsolete bug.