Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2408133007:
Check the root frame pointer events as well for capture (Closed)
|
Created:
4 years, 2 months ago by Navid Zolghadr Modified:
4 years, 2 months ago CC:
chromium-reviews, blink-reviews, nzolghadr+blinkwatch_chromium.org, dtapuska+blinkwatch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCheck the root pointer events as well for capture
Touch pointer events events are being routed to
the target directly without passing all the
iframes' EventHandlers. As a result the inner
EventHandler has no clue whether a given pointerId
is active or capturable if that is a touch pointer
event.
This CL adds the check to every iframe to also
checks the root frame for the pointer event and
it allows the capture if that pointer event was
sent to a node inside the requested iframe.
BUG=653860
Committed: https://crrev.com/85c41444f11d3c24ea24d3c776d96dbe913cad03
Cr-Commit-Position: refs/heads/master@{#426205}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Applying Mustaq's comments #
Total comments: 2
Patch Set 3 : Only lookup the root frame for touch pointer events #
Total comments: 4
Patch Set 4 : adding comments #
Total comments: 2
Patch Set 5 : Fix the function naming #Messages
Total messages: 47 (30 generated)
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...
Description was changed from ========== Check the root pointer events as well for capture Touch pointer events events are being routed to the target directly without passing all the iframes' EventHandlers. As a result the inner EventHandler has no clue whether a given pointerId is active or capturable if that is a touch pointer event. This CL adds the check to every iframe to also checks the root frame for the pointer event and it allows the capture if that pointer event was sent to a node inside the requested iframe. BUG=653860 ========== to ========== Check the root pointer events as well for capture Touch pointer events events are being routed to the target directly without passing all the iframes' EventHandlers. As a result the inner EventHandler has no clue whether a given pointerId is active or capturable if that is a touch pointer event. This CL adds the check to every iframe to also checks the root frame for the pointer event and it allows the capture if that pointer event was sent to a node inside the requested iframe. BUG=653860 ==========
nzolghadr@chromium.org changed reviewers: + dtapuska@chromium.org, mustaq@chromium.org
https://codereview.chromium.org/2408133007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right): https://codereview.chromium.org/2408133007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/PointerEventFactory.cpp:352: int type = p.pointerTypeInt(); Nit: s/type/typeInt One more below. https://codereview.chromium.org/2408133007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2408133007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:1202: if (m_frame != m_frame->localFrameRoot() && The same check is repeated 3 times---this calls for a private bool function. https://codereview.chromium.org/2408133007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:1209: m_pointerEventManager->setPointerCapture(pointerId, target); Not clear why we shouldn't check |isActiveOnFrame| in both cases. Could you please clarify? https://codereview.chromium.org/2408133007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.h (right): https://codereview.chromium.org/2408133007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.h:258: bool isTouchPointerEventActiveOnFrame(int, LocalFrame*); Why is this touch specific? Could be useful for all pointerTypes. https://codereview.chromium.org/2408133007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.h (right): https://codereview.chromium.org/2408133007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.h:73: bool isTouchPointerEventActiveOnFrame(int, LocalFrame*); Again, there's nothing specific to pointerType=touch here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/2408133007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right): https://codereview.chromium.org/2408133007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/PointerEventFactory.cpp:352: int type = p.pointerTypeInt(); On 2016/10/13 16:38:25, mustaq wrote: > Nit: s/type/typeInt > > One more below. Done. https://codereview.chromium.org/2408133007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2408133007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:1202: if (m_frame != m_frame->localFrameRoot() && On 2016/10/13 16:38:25, mustaq wrote: > The same check is repeated 3 times---this calls for a private bool function. Done. https://codereview.chromium.org/2408133007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:1209: m_pointerEventManager->setPointerCapture(pointerId, target); On 2016/10/13 16:38:25, mustaq wrote: > Not clear why we shouldn't check |isActiveOnFrame| in both cases. Could you > please clarify? setPointerCapture just ignores if the pointer is not active on the frame that m_pointerEventManager belongs to. The check that whether a pointer is active in the root or not is to just determine whose setPointerCapture to call. https://codereview.chromium.org/2408133007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.h (right): https://codereview.chromium.org/2408133007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.h:258: bool isTouchPointerEventActiveOnFrame(int, LocalFrame*); On 2016/10/13 16:38:25, mustaq wrote: > Why is this touch specific? Could be useful for all pointerTypes. Sure. https://codereview.chromium.org/2408133007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.h (right): https://codereview.chromium.org/2408133007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.h:73: bool isTouchPointerEventActiveOnFrame(int, LocalFrame*); On 2016/10/13 16:38:25, mustaq wrote: > Again, there's nothing specific to pointerType=touch here. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2408133007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2408133007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:1202: if (m_frame != m_frame->localFrameRoot() && On 2016/10/13 17:32:32, Navid Zolghadr wrote: > On 2016/10/13 16:38:25, mustaq wrote: > > The same check is repeated 3 times---this calls for a private bool function. > > Done. I think IsPointerActiveInRootFrame is a bit misnomer because it returns true iff the root frame knows that the pointer is active in m_frame, right? Not sure. There is clearly two frames involved in the predicate. Is RootFramePointerActiveInCurrentFrame a better choice? :-P Another thought: instead of returning true/false on a given frame, can't the PEManager return the active frame for a given pointer id so that this method here will directly call the active frame's setPointerCapture? https://codereview.chromium.org/2408133007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:1209: m_pointerEventManager->setPointerCapture(pointerId, target); On 2016/10/13 17:32:32, Navid Zolghadr wrote: > On 2016/10/13 16:38:25, mustaq wrote: > > Not clear why we shouldn't check |isActiveOnFrame| in both cases. Could you > > please clarify? > > setPointerCapture just ignores if the pointer is not active on the frame that > m_pointerEventManager belongs to. The check that whether a pointer is active in > the root or not is to just determine whose setPointerCapture to call. Does that mean there is a possibility setPointerCapture could fail silently just because IsPointerActiveInRootFrame returned false incorrectly? This could theoretically happen if lastNodeReceivingEvent == null there somehow but I don't see any way this is possible. Please consider adding a check for non-null there. https://codereview.chromium.org/2408133007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2408133007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:1185: bool EventHandler::isPointerEventActiveOnFrame(int pointerId, Nit: s/Event/Id/ in these predicates.
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/2408133007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2408133007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:1202: if (m_frame != m_frame->localFrameRoot() && On 2016/10/13 19:55:38, mustaq wrote: > On 2016/10/13 17:32:32, Navid Zolghadr wrote: > > On 2016/10/13 16:38:25, mustaq wrote: > > > The same check is repeated 3 times---this calls for a private bool function. > > > > Done. > > I think IsPointerActiveInRootFrame is a bit misnomer because it returns true iff > the root frame knows that the pointer is active in m_frame, right? Not sure. > There is clearly two frames involved in the predicate. Is > RootFramePointerActiveInCurrentFrame a better choice? :-P > > Another thought: instead of returning true/false on a given frame, can't the > PEManager return the active frame for a given pointer id so that this method > here will directly call the active frame's setPointerCapture? I updated the name. The second paragraph is not applicable. It is not correct that we should always setPointerCapture in the active frame. Because sometimes there is no active pointer on a frame and just because someone in that frame calls setPointerCapture that frame shouldn't go and call setPointerCapture on a frame that actually has that active pointer. Basically the else path is needed not only when we have the active pointer inside this frame but also when there is no such pointer for this frame which causes setPointerCapture has no effect which matches one of the steps in "Setting Pointer Capture" algorithm in the spec. https://codereview.chromium.org/2408133007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:1209: m_pointerEventManager->setPointerCapture(pointerId, target); On 2016/10/13 19:55:38, mustaq wrote: > On 2016/10/13 17:32:32, Navid Zolghadr wrote: > > On 2016/10/13 16:38:25, mustaq wrote: > > > Not clear why we shouldn't check |isActiveOnFrame| in both cases. Could you > > > please clarify? > > > > setPointerCapture just ignores if the pointer is not active on the frame that > > m_pointerEventManager belongs to. The check that whether a pointer is active > in > > the root or not is to just determine whose setPointerCapture to call. > > Does that mean there is a possibility setPointerCapture could fail silently just > because IsPointerActiveInRootFrame returned false incorrectly? This could > theoretically happen if lastNodeReceivingEvent == null there somehow but I don't > see any way this is possible. Please consider adding a check for non-null there. If that lastNodeReceivingEvent is null that means that frame doesn't have that pointerevent anymore and returing false and going to the else is correct. In that case setPointerCapture may silently do nothing which matches one of the steps in the spec. https://codereview.chromium.org/2408133007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2408133007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:1185: bool EventHandler::isPointerEventActiveOnFrame(int pointerId, On 2016/10/13 19:55:38, mustaq wrote: > Nit: s/Event/Id/ in these predicates. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm https://codereview.chromium.org/2408133007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/PointerEventFactory.h (right): https://codereview.chromium.org/2408133007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventFactory.h:76: WebPointerProperties::PointerType getPointerType(int pointerId) const; Document that it returns Unknown for an inactive id. https://codereview.chromium.org/2408133007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2408133007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:661: if (m_pointerEventFactory.getPointerType(pointerId) != Please add a comment about why this is Touch-only. It's easy to miss the subtlety here given that the rest of the code /seems/ independent of pointerType.
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/2408133007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/PointerEventFactory.h (right): https://codereview.chromium.org/2408133007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventFactory.h:76: WebPointerProperties::PointerType getPointerType(int pointerId) const; On 2016/10/17 20:18:56, mustaq wrote: > Document that it returns Unknown for an inactive id. Done. https://codereview.chromium.org/2408133007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2408133007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:661: if (m_pointerEventFactory.getPointerType(pointerId) != On 2016/10/17 20:18:56, mustaq wrote: > Please add a comment about why this is Touch-only. It's easy to miss the > subtlety here given that the rest of the code /seems/ independent of > pointerType. Done.
nzolghadr@chromium.org changed reviewers: + bokan@chromium.org
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
The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org Link to the patchset: https://codereview.chromium.org/2408133007/#ps60001 (title: "adding comments")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm % nit https://codereview.chromium.org/2408133007/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandler.h (right): https://codereview.chromium.org/2408133007/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.h:341: bool RootFrameTouchPointerActiveInCurrentFrame(int pointerId) const; I don't think we name functions with a capital as the first letter.
lgtm % nit https://codereview.chromium.org/2408133007/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandler.h (right): https://codereview.chromium.org/2408133007/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.h:341: bool RootFrameTouchPointerActiveInCurrentFrame(int pointerId) const; I don't think we name functions with a capital as the first letter.
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/2408133007/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandler.h (right): https://codereview.chromium.org/2408133007/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.h:341: bool RootFrameTouchPointerActiveInCurrentFrame(int pointerId) const; On 2016/10/19 00:22:29, dtapuska wrote: > I don't think we name functions with a capital as the first letter. Done.
The CQ bit was checked by nzolghadr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtapuska@chromium.org, mustaq@chromium.org, bokan@chromium.org Link to the patchset: https://codereview.chromium.org/2408133007/#ps80001 (title: "Fix the function naming")
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.
Description was changed from ========== Check the root pointer events as well for capture Touch pointer events events are being routed to the target directly without passing all the iframes' EventHandlers. As a result the inner EventHandler has no clue whether a given pointerId is active or capturable if that is a touch pointer event. This CL adds the check to every iframe to also checks the root frame for the pointer event and it allows the capture if that pointer event was sent to a node inside the requested iframe. BUG=653860 ========== to ========== Check the root pointer events as well for capture Touch pointer events events are being routed to the target directly without passing all the iframes' EventHandlers. As a result the inner EventHandler has no clue whether a given pointerId is active or capturable if that is a touch pointer event. This CL adds the check to every iframe to also checks the root frame for the pointer event and it allows the capture if that pointer event was sent to a node inside the requested iframe. BUG=653860 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Check the root pointer events as well for capture Touch pointer events events are being routed to the target directly without passing all the iframes' EventHandlers. As a result the inner EventHandler has no clue whether a given pointerId is active or capturable if that is a touch pointer event. This CL adds the check to every iframe to also checks the root frame for the pointer event and it allows the capture if that pointer event was sent to a node inside the requested iframe. BUG=653860 ========== to ========== Check the root pointer events as well for capture Touch pointer events events are being routed to the target directly without passing all the iframes' EventHandlers. As a result the inner EventHandler has no clue whether a given pointerId is active or capturable if that is a touch pointer event. This CL adds the check to every iframe to also checks the root frame for the pointer event and it allows the capture if that pointer event was sent to a node inside the requested iframe. BUG=653860 Committed: https://crrev.com/85c41444f11d3c24ea24d3c776d96dbe913cad03 Cr-Commit-Position: refs/heads/master@{#426205} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/85c41444f11d3c24ea24d3c776d96dbe913cad03 Cr-Commit-Position: refs/heads/master@{#426205} |
