Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2635583002:
Add UseCounter for setPointerCapture outside dispatch (Closed)
|
Created:
3 years, 11 months ago by Rick Byers Modified:
3 years, 10 months ago CC:
chromium-reviews, blink-reviews, dtapuska+blinkwatch_chromium.org, asvitkine+watch_chromium.org, Navid Zolghadr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UseCounter for setPointerCapture outside dispatch
Attempt to measure whether any sites might break if setPointerCapture
became a no-op outside of a context that was dispatching an event
for that pointer.
BUG=606896
Review-Url: https://codereview.chromium.org/2635583002
Cr-Commit-Position: refs/heads/master@{#449007}
Committed: https://chromium.googlesource.com/chromium/src/+/a7a6a91e6090c759c6699e8062a034fb41900059
Patch Set 1 #Patch Set 2 : Merge branch 'master' into capture-metrics #
Total comments: 4
Patch Set 3 : CR feedback #Patch Set 4 : rebase #Patch Set 5 : rebase #Patch Set 6 : Rebase again #
Messages
Total messages: 30 (19 generated)
The CQ bit was checked by rbyers@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
rbyers@chromium.org changed reviewers: + nzolghadr@chromium.org
Navid PTAL
dtapuska@chromium.org changed reviewers: + dtapuska@chromium.org
https://codereview.chromium.org/2635583002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2635583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:169: m_dispatchingPointerId = pointerId; You able to use an AutoReset here?
lgtm https://codereview.chromium.org/2635583002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2635583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:635: UseCounter::PointerEventSetCaptureOutsideDispatch); There is one more place (as of a "temporary" hack) that we fire pointerevents: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Node.... This will mis-count the ones that are coming from the handler of those events.
rbyers@chromium.org changed reviewers: + rkaplow@chromium.org
Thanks. PTAL +rkaplow for metrics OWNER https://codereview.chromium.org/2635583002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2635583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:169: m_dispatchingPointerId = pointerId; On 2017/01/16 21:07:41, dtapuska wrote: > You able to use an AutoReset here? Done. https://codereview.chromium.org/2635583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:635: UseCounter::PointerEventSetCaptureOutsideDispatch); On 2017/01/16 21:15:01, Navid Zolghadr wrote: > There is one more place (as of a "temporary" hack) that we fire pointerevents: > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Node.... > > This will mis-count the ones that are coming from the handler of those events. Whoa, that looks potentially bad - commented on the bug. I don't see an easy way to handle that weird case, so I think the right answer is just to depend on the fix for that bug. In those cases pointer capture is generally going to be broken anyway because isActiveButtonsState above will return false since PointerEventManager never saw the pointerdown event. Even if I did get the metric hacked into those cases, it'll still be an innacurate measure of the risk due to setPointerCapture itself failing in some of those cases when it should be succeeding. So I suggest we just proceed with this code as-is with the understanding that the metric may be off until crbug.com/665924 is fixed. My hunch is that it won't be off by much (since setPointerCapture is probably rare in Flash/pointerlock/etc cases) - but I guess we'll find out...
lgtm
The CQ bit was checked by rbyers@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nzolghadr@chromium.org Link to the patchset: https://codereview.chromium.org/2635583002/#ps40001 (title: "CR feedback")
The CQ bit was unchecked by rbyers@chromium.org
The CQ bit was checked by rbyers@chromium.org
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rbyers@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, nzolghadr@chromium.org Link to the patchset: https://codereview.chromium.org/2635583002/#ps80001 (title: "rebase")
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
Failed to apply patch for third_party/WebKit/Source/core/frame/UseCounter.h:
While running git apply --index -p1;
error: patch failed: third_party/WebKit/Source/core/frame/UseCounter.h:1454
error: third_party/WebKit/Source/core/frame/UseCounter.h: patch does not apply
Patch: third_party/WebKit/Source/core/frame/UseCounter.h
Index: third_party/WebKit/Source/core/frame/UseCounter.h
diff --git a/third_party/WebKit/Source/core/frame/UseCounter.h
b/third_party/WebKit/Source/core/frame/UseCounter.h
index
5579ffa5c0fd904b1f40c1d2a867a8d2e99a28ce..13740978258a977d98bd0be5549b2a9a5edf89ea
100644
--- a/third_party/WebKit/Source/core/frame/UseCounter.h
+++ b/third_party/WebKit/Source/core/frame/UseCounter.h
@@ -1454,6 +1454,7 @@ class CORE_EXPORT UseCounter {
V8TextDetector_Detect_Method = 1801,
CSSValueOnDemand = 1802,
ServiceWorkerNavigationPreload = 1803,
+ PointerEventSetCaptureOutsideDispatch = 1804,
// Add new features immediately above this line. Don't change assigned
// numbers of any item, and don't reuse removed slots.
The CQ bit was checked by rbyers@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, nzolghadr@chromium.org Link to the patchset: https://codereview.chromium.org/2635583002/#ps100001 (title: "Rebase again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486563377060330,
"parent_rev": "804e1f6bf3075c200aebb5489ea7cb8b25b5275a", "commit_rev":
"a7a6a91e6090c759c6699e8062a034fb41900059"}
Message was sent while issue was closed.
Description was changed from ========== Add UseCounter for setPointerCapture outside dispatch Attempt to measure whether any sites might break if setPointerCapture became a no-op outside of a context that was dispatching an event for that pointer. BUG=606896 ========== to ========== Add UseCounter for setPointerCapture outside dispatch Attempt to measure whether any sites might break if setPointerCapture became a no-op outside of a context that was dispatching an event for that pointer. BUG=606896 Review-Url: https://codereview.chromium.org/2635583002 Cr-Commit-Position: refs/heads/master@{#449007} Committed: https://chromium.googlesource.com/chromium/src/+/a7a6a91e6090c759c6699e8062a0... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/a7a6a91e6090c759c6699e8062a0... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
