Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2174863002:
Change mouse pointer event targets to the capturing node (Closed)
|
Created:
4 years, 5 months ago by Navid Zolghadr Modified:
4 years, 4 months ago CC:
chromium-reviews, shans, rjwright, blink-reviews-animation_chromium.org, nzolghadr+blinkwatch_chromium.org, dtapuska+blinkwatch_chromium.org, darktears, blink-reviews, Eric Willigers Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange mouse pointer event targets to the capturing node
This is the first step to change to hit-test node of the
mouse events to the captured node.
Note that this change doesn't remove the hit-testing itself.
So it doesn't improve the performance while a pointer is
captured. To be able to do that we need some refactoring to
be done first. See crbug.com/625843.
BUG=629921
Committed: https://crrev.com/74eff8d7daba3c907a07f44c231a7d07d8866a18
Cr-Commit-Position: refs/heads/master@{#409521}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Applying comments #Patch Set 3 : Fix the test #Patch Set 4 : Skip the wpt failing tests #
Total comments: 2
Patch Set 5 : Fix redundant compat mouse boundary events #
Total comments: 2
Patch Set 6 : Rebasing #
Total comments: 2
Patch Set 7 : Variable name change #Messages
Total messages: 43 (26 generated)
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
nzolghadr@chromium.org changed reviewers: + dtapuska@chromium.org, mustaq@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:1414: if (EventTarget* tmp = m_pointerEventManager.getMouseCapturingNode()) { You are going to have to use a better name than tmp :-) https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:637: EventTarget* pointercaptureTarget = (it != m_pointerCaptureTarget.end()) ? it->value : nullptr; collapse this into the if; like if(EventTarget* pointerCaptureTarget = ) https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:795: return getCapturingNode(1); This is an odd little constant. Can we name it formally? https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.h (left): https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.h:153: EventTarget* getCapturingNode(int); ditto https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.h (right): https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.h:69: EventTarget* getMouseCapturingNode() const; This is odd; returning a ptr from a const object; who owns this EventTarget* ptr? Usually you return a const * from a const method. So I'd likely remove the const attribute
https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:1411: Node* result = targetNode; s/result/newNodeUnderMouse https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:673: dispatchPointerEvent(pendingPointerCaptureTarget, Caught here again, sorry. Please add a comment why a null check for for ppct is unnecessary (from your comment in the last CL). https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:795: return getCapturingNode(1); May be reuse the constant from PE factory?
https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-capture-expected.txt (right): https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-capture-expected.txt:81: green received pointermove Shouldn't this (or one below) be blue? The capture has been release, right?
https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-capture-expected.txt (right): https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-capture-expected.txt:302: blue received pointerout 14 Seems we have been missing pointerout/leave before right before touchend or touchcancel. This is fixed now. I filed crbug.com/630650 to investigate the previous case, to be sure we didn't have any corner case unhandled.
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal. https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-capture-expected.txt (right): https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-capture-expected.txt:81: green received pointermove On 2016/07/22 16:07:28, mustaq wrote: > Shouldn't this (or one below) be blue? The capture has been release, right? Done. https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:1411: Node* result = targetNode; On 2016/07/22 16:01:01, mustaq wrote: > s/result/newNodeUnderMouse Done. https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:1414: if (EventTarget* tmp = m_pointerEventManager.getMouseCapturingNode()) { On 2016/07/22 15:57:39, dtapuska wrote: > You are going to have to use a better name than tmp :-) Done. :) https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:637: EventTarget* pointercaptureTarget = (it != m_pointerCaptureTarget.end()) ? it->value : nullptr; On 2016/07/22 15:57:39, dtapuska wrote: > collapse this into the if; > like if(EventTarget* pointerCaptureTarget = ) Done. https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:673: dispatchPointerEvent(pendingPointerCaptureTarget, On 2016/07/22 16:01:01, mustaq wrote: > Caught here again, sorry. Please add a comment why a null check for for ppct is > unnecessary (from your comment in the last CL). Done. https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:795: return getCapturingNode(1); On 2016/07/22 16:01:01, mustaq wrote: > May be reuse the constant from PE factory? :) yeah. I was trying to avoid making that constant public. But sure. I'll do that and probably make it private again later on. https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.h (left): https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.h:153: EventTarget* getCapturingNode(int); On 2016/07/22 15:57:39, dtapuska wrote: > ditto Done. https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.h (right): https://codereview.chromium.org/2174863002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.h:69: EventTarget* getMouseCapturingNode() const; On 2016/07/22 15:57:39, dtapuska wrote: > This is odd; returning a ptr from a const object; who owns this EventTarget* > ptr? > > Usually you return a const * from a const method. So I'd likely remove the const > attribute Done.
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2174863002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-capture-transition-events-expected.txt (right): https://codereview.chromium.org/2174863002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-capture-transition-events-expected.txt:62: blue received mouseout Why is blue receiving mouseout/leave w/o any mouseover/enter?
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal. This should be good now. I had to add a quick hack to update m_nodeUnderMouse after the dispatching of pointerevent. https://codereview.chromium.org/2174863002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-capture-transition-events-expected.txt (right): https://codereview.chromium.org/2174863002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-capture-transition-events-expected.txt:62: blue received mouseout On 2016/07/22 19:50:18, mustaq wrote: > Why is blue receiving mouseout/leave w/o any mouseover/enter? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nzolghadr@chromium.org changed reviewers: + rbyers@chromium.org
This l-g-t-m, thanks for fixing the tests. I will take another look after rebasing with current no-immediate-ppc-process code. https://codereview.chromium.org/2174863002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-capture-transition-events-expected.txt (right): https://codereview.chromium.org/2174863002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-capture-transition-events-expected.txt:22: green received pointerout Note that it is not ideal that green is receiving PEs while blue is capturing. We used to be ideal before IIRC. I will file a bug.
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Rebased. ptal. https://codereview.chromium.org/2174863002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-capture-transition-events-expected.txt (right): https://codereview.chromium.org/2174863002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-capture-transition-events-expected.txt:22: green received pointerout On 2016/08/02 19:31:07, mustaq wrote: > Note that it is not ideal that green is receiving PEs while blue is capturing. > We used to be ideal before IIRC. I will file a bug. I will address that issue separately in another CL. That needs a little bit of moving functions around to change the order. I believe doing it in a separate CL for that issue makes sense since it calls it out and gets more attention to what actually happens to achieve that.
LGTM. https://codereview.chromium.org/2174863002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2174863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.cpp:1512: const auto& retVal = m_pointerEventManager.sendMousePointerEvent( s/retVal/eventResult?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2174863002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2174863002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.cpp:1512: const auto& retVal = m_pointerEventManager.sendMousePointerEvent( On 2016/08/02 21:47:12, mustaq wrote: > s/retVal/eventResult? Done.
The CQ bit was unchecked by nzolghadr@chromium.org
The CQ bit was checked by nzolghadr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/2174863002/#ps120001 (title: "Variable name change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Change mouse pointer event targets to the capturing node This is the first step to change to hit-test node of the mouse events to the captured node. Note that this change doesn't remove the hit-testing itself. So it doesn't improve the performance while a pointer is captured. To be able to do that we need some refactoring to be done first. See crbug.com/625843. BUG=629921 ========== to ========== Change mouse pointer event targets to the capturing node This is the first step to change to hit-test node of the mouse events to the captured node. Note that this change doesn't remove the hit-testing itself. So it doesn't improve the performance while a pointer is captured. To be able to do that we need some refactoring to be done first. See crbug.com/625843. BUG=629921 Committed: https://crrev.com/74eff8d7daba3c907a07f44c231a7d07d8866a18 Cr-Commit-Position: refs/heads/master@{#409521} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/74eff8d7daba3c907a07f44c231a7d07d8866a18 Cr-Commit-Position: refs/heads/master@{#409521} |
