Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1864523004:
Add UMA metric for tracking root level listeners for blocking touch. (Closed)
DescriptionAdd UMA metric for tracking root level listeners for blocking touch.
In order to quantify the benefit of making document level touch
listeners passive for a possible intervention measure the behavior
of touch events on the root document.
BUG=604790
Committed: https://crrev.com/c21fd3ea10e3111566b4c912a00d4c6cd8474d7f
Cr-Commit-Position: refs/heads/master@{#389122}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Move metric solely into EventHandler #Patch Set 3 : Revise comment #Patch Set 4 : Add histograms.xml #
Total comments: 4
Patch Set 5 : Fix tdresser's comments #
Total comments: 2
Patch Set 6 : Remove extra classification #
Messages
Total messages: 36 (12 generated)
Description was changed from ========== Add UMA metric for tracking root level listeners for blocking touch. In order to quantify the benefit of making document level touch listeners passive for a possible intervention measure the behavior of touch events on the root document. BUG= ========== to ========== Add UMA metric for tracking root level listeners for blocking touch. In order to quantify the benefit of making document level touch listeners passive for a possible intervention measure the behavior of touch events on the root document. BUG= ==========
dtapuska@chromium.org changed reviewers: + tdresser@chromium.org
On 2016/04/06 21:17:54, dtapuska wrote: > mailto:dtapuska@chromium.org changed reviewers: > + mailto:tdresser@chromium.org Is this what you were thinking? I am not handling passive dispatched events correctly; I'm not sure how we want to handle that.
dtapuska@chromium.org changed reviewers: + lanwei@chromium.org
Create a bug. We'll need to also track the potential benefit here, which is a bit more of a pain. (https://docs.google.com/document/d/10O_wMEpKgnhCfEQ8L3JckEKvkcclwEwCLtL0nSl7u...) I don't care if that's the same bug or a different bug. Is there any way to add an automated test for this? Blink has a HistogramTester: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Scroll anchoring has some good examples here. https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl... This could likely just live in https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/events/TouchEvent.cpp (right): https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/TouchEvent.cpp:36: #include "core/html/HTMLElement.h" Is this used? https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/TouchEvent.cpp:44: const size_t kRootScrollerOffset = 4; Add a comment indicating what these are offsets into, why we need them etc. https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/TouchEvent.cpp:63: if (node.isDocumentNode() || static_cast<Node*>(node.document().documentElement()) == &node || static_cast<Node*>(node.document().body()) == &node) { We need to check |window| too, don't we? Add a comment on what targets we're checking, and why. https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/TouchEvent.cpp:70: if (dispatchResult != DispatchEventResult::NotCanceled) { Let's be consistent about whether or not we use {} for if statements with single line bodies. https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/TouchEvent.cpp:73: return (ListenerHistogram)result; static_cast https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/TouchEvent.cpp:192: if (touchEvent.cancelable() && touchEvent.touches() && touchEvent.touches()->length() == 1 && !dispatcher.node().document().ownerElement() && (touchEvent.type() == EventTypeNames::touchstart || (touchEvent.type() == EventTypeNames::touchmove && touchEvent.firstTouchMoveAfterTouchStart()))) { This condition is pretty illegible. Maybe split this up into a few intermediate booleans? Or use a series of if's with early returns if the condition fails? https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/events/TouchEvent.h (right): https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/TouchEvent.h:52: bool causesScrollingIfUncanceled, bool firstTouchMoveAfterTouchStart, firstTouchMoveAfterScrollStart is a bit confusing, because it's actually the first touchmove than isn't filtered out by the slop region. Maybe touchMoveStartingScroll or firstTouchMoveAfterSlopRegion?
On 2016/04/07 12:22:08, tdresser wrote: > Create a bug. > > We'll need to also track the potential benefit here, which is a bit more of a > pain. > (https://docs.google.com/document/d/10O_wMEpKgnhCfEQ8L3JckEKvkcclwEwCLtL0nSl7u...) > > I don't care if that's the same bug or a different bug. > > Is there any way to add an automated test for this? > > Blink has a HistogramTester: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Scroll anchoring has some good examples here. > https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl... > > This could likely just live in > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/events/TouchEvent.cpp (right): > > https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/events/TouchEvent.cpp:36: #include > "core/html/HTMLElement.h" > Is this used? > > https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/events/TouchEvent.cpp:44: const size_t > kRootScrollerOffset = 4; > Add a comment indicating what these are offsets into, why we need them etc. > > https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/events/TouchEvent.cpp:63: if > (node.isDocumentNode() || static_cast<Node*>(node.document().documentElement()) > == &node || static_cast<Node*>(node.document().body()) == &node) { > We need to check |window| too, don't we? > > Add a comment on what targets we're checking, and why. > > https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/events/TouchEvent.cpp:70: if (dispatchResult != > DispatchEventResult::NotCanceled) { > Let's be consistent about whether or not we use {} for if statements with single > line bodies. > > https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/events/TouchEvent.cpp:73: return > (ListenerHistogram)result; > static_cast > > https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/events/TouchEvent.cpp:192: if > (touchEvent.cancelable() && touchEvent.touches() && > touchEvent.touches()->length() == 1 && > !dispatcher.node().document().ownerElement() && (touchEvent.type() == > EventTypeNames::touchstart || (touchEvent.type() == EventTypeNames::touchmove && > touchEvent.firstTouchMoveAfterTouchStart()))) { > This condition is pretty illegible. Maybe split this up into a few intermediate > booleans? > > Or use a series of if's with early returns if the condition fails? > > https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/events/TouchEvent.h (right): > > https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/events/TouchEvent.h:52: bool > causesScrollingIfUncanceled, bool firstTouchMoveAfterTouchStart, > firstTouchMoveAfterScrollStart is a bit confusing, because it's actually the > first touchmove than isn't filtered out by the slop region. > > Maybe touchMoveStartingScroll or firstTouchMoveAfterSlopRegion? There aren't histograms reported to Layout tests. I suppose I could write a browser test for this; although that seems like overkill. WDYT?
https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/events/TouchEvent.cpp (right): https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/TouchEvent.cpp:36: #include "core/html/HTMLElement.h" On 2016/04/07 12:22:07, tdresser wrote: > Is this used? Done. https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/TouchEvent.cpp:44: const size_t kRootScrollerOffset = 4; On 2016/04/07 12:22:07, tdresser wrote: > Add a comment indicating what these are offsets into, why we need them etc. Done. https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/TouchEvent.cpp:63: if (node.isDocumentNode() || static_cast<Node*>(node.document().documentElement()) == &node || static_cast<Node*>(node.document().body()) == &node) { On 2016/04/07 12:22:08, tdresser wrote: > We need to check |window| too, don't we? > > Add a comment on what targets we're checking, and why. > Done. https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/TouchEvent.cpp:70: if (dispatchResult != DispatchEventResult::NotCanceled) { On 2016/04/07 12:22:07, tdresser wrote: > Let's be consistent about whether or not we use {} for if statements with single > line bodies. Done. https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/TouchEvent.cpp:73: return (ListenerHistogram)result; On 2016/04/07 12:22:08, tdresser wrote: > static_cast Done. https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/TouchEvent.cpp:192: if (touchEvent.cancelable() && touchEvent.touches() && touchEvent.touches()->length() == 1 && !dispatcher.node().document().ownerElement() && (touchEvent.type() == EventTypeNames::touchstart || (touchEvent.type() == EventTypeNames::touchmove && touchEvent.firstTouchMoveAfterTouchStart()))) { On 2016/04/07 12:22:08, tdresser wrote: > This condition is pretty illegible. Maybe split this up into a few intermediate > booleans? > > Or use a series of if's with early returns if the condition fails? Done. https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/events/TouchEvent.h (right): https://codereview.chromium.org/1864523004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/TouchEvent.h:52: bool causesScrollingIfUncanceled, bool firstTouchMoveAfterTouchStart, On 2016/04/07 12:22:08, tdresser wrote: > firstTouchMoveAfterScrollStart is a bit confusing, because it's actually the > first touchmove than isn't filtered out by the slop region. > > Maybe touchMoveStartingScroll or firstTouchMoveAfterSlopRegion? Removed
Darn, I thought there was a way to read histogram entries from layout tests. Yeah, don't worry about doing something super complicated here. We should at least have a unit test here though. If we factor this logic out of EventHandler, it should be pretty straight forward to write a unit test. Also, can you link the bug in the description? https://codereview.chromium.org/1864523004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1864523004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:211: Could we factor all this logic out into a separate file? EventHandler is a bit of a dumping ground. https://codereview.chromium.org/1864523004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:219: enum TouchTargetAndDispatchResultTypeHistogram { I don't think we need "Histogram" in the type here.
Description was changed from ========== Add UMA metric for tracking root level listeners for blocking touch. In order to quantify the benefit of making document level touch listeners passive for a possible intervention measure the behavior of touch events on the root document. BUG= ========== to ========== Add UMA metric for tracking root level listeners for blocking touch. In order to quantify the benefit of making document level touch listeners passive for a possible intervention measure the behavior of touch events on the root document. BUG=604790 ==========
dtapuska@chromium.org changed reviewers: + isherman@chromium.org
https://codereview.chromium.org/1864523004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1864523004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:211: On 2016/04/19 20:49:58, tdresser wrote: > Could we factor all this logic out into a separate file? EventHandler is a bit > of a dumping ground. I think this is all going to change when Navid lands his EventHandler changes. I wanted to land this first before he re-organizes everything in change https://codereview.chromium.org/1892653003/ so that we can integrate this into 51 easily. https://codereview.chromium.org/1864523004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:219: enum TouchTargetAndDispatchResultTypeHistogram { On 2016/04/19 20:49:58, tdresser wrote: > I don't think we need "Histogram" in the type here. Done.
LGTM. It's a shame there isn't a better way to test this. I assume you've done thorough local testing?
On 2016/04/21 12:13:33, tdresser wrote: > LGTM. > > It's a shame there isn't a better way to test this. > > I assume you've done thorough local testing? Yes local testing done. It would be good if we could read the histograms via the GPU benchmark extension.
On 2016/04/21 12:59:14, dtapuska wrote: > On 2016/04/21 12:13:33, tdresser wrote: > > LGTM. > > > > It's a shame there isn't a better way to test this. > > > > I assume you've done thorough local testing? > > Yes local testing done. It would be good if we could read the histograms via the > GPU benchmark extension. We'd want it in window.internals, not gpu benchmarking extension. It's similar to reading usecounters, which is already implemented. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... I agree that would be a good change.
On 2016/04/21 13:21:20, tdresser wrote: > On 2016/04/21 12:59:14, dtapuska wrote: > > On 2016/04/21 12:13:33, tdresser wrote: > > > LGTM. > > > > > > It's a shame there isn't a better way to test this. > > > > > > I assume you've done thorough local testing? > > > > Yes local testing done. It would be good if we could read the histograms via > the > > GPU benchmark extension. > > We'd want it in window.internals, not gpu benchmarking extension. > > It's similar to reading usecounters, which is already implemented. > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > I agree that would be a good change. that would be a question of do you want renderer side histograms or do you want the conglomerate browser side histograms. Nonetheless this debate is for a bug to be opened.
On 2016/04/21 13:40:01, dtapuska wrote: > On 2016/04/21 13:21:20, tdresser wrote: > > On 2016/04/21 12:59:14, dtapuska wrote: > > > On 2016/04/21 12:13:33, tdresser wrote: > > > > LGTM. > > > > > > > > It's a shame there isn't a better way to test this. > > > > > > > > I assume you've done thorough local testing? > > > > > > Yes local testing done. It would be good if we could read the histograms via > > the > > > GPU benchmark extension. > > > > We'd want it in window.internals, not gpu benchmarking extension. > > > > It's similar to reading usecounters, which is already implemented. > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > I agree that would be a good change. > > that would be a question of do you want renderer side histograms or do you want > the conglomerate browser side histograms. Nonetheless this debate is for a bug > to be opened. crbug.com/605538
LGTM.
On 2016/04/21 19:00:57, lanwei wrote: > LGTM. isherman@; ping.
Metrics LGTM % a nit: https://codereview.chromium.org/1864523004/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1864523004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12082: + target classification whether it is a root scroll listener (window, nit: Duplicate "classification" -- please rephrase.
https://codereview.chromium.org/1864523004/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1864523004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12082: + target classification whether it is a root scroll listener (window, On 2016/04/22 08:22:56, Ilya Sherman wrote: > nit: Duplicate "classification" -- please rephrase. Done.
The CQ bit was checked by dtapuska@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, tdresser@chromium.org, lanwei@chromium.org Link to the patchset: https://codereview.chromium.org/1864523004/#ps100001 (title: "Remove extra classification")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1864523004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1864523004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dtapuska@chromium.org changed reviewers: + bokan@chromium.org
On 2016/04/22 13:40:59, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) bokan@ to review EventHandler.*
On 2016/04/22 13:40:59, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) bokan@ to review EventHandler.*
EventHandler lgtm
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1864523004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1864523004/100001
Message was sent while issue was closed.
Description was changed from ========== Add UMA metric for tracking root level listeners for blocking touch. In order to quantify the benefit of making document level touch listeners passive for a possible intervention measure the behavior of touch events on the root document. BUG=604790 ========== to ========== Add UMA metric for tracking root level listeners for blocking touch. In order to quantify the benefit of making document level touch listeners passive for a possible intervention measure the behavior of touch events on the root document. BUG=604790 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add UMA metric for tracking root level listeners for blocking touch. In order to quantify the benefit of making document level touch listeners passive for a possible intervention measure the behavior of touch events on the root document. BUG=604790 ========== to ========== Add UMA metric for tracking root level listeners for blocking touch. In order to quantify the benefit of making document level touch listeners passive for a possible intervention measure the behavior of touch events on the root document. BUG=604790 Committed: https://crrev.com/c21fd3ea10e3111566b4c912a00d4c6cd8474d7f Cr-Commit-Position: refs/heads/master@{#389122} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c21fd3ea10e3111566b4c912a00d4c6cd8474d7f Cr-Commit-Position: refs/heads/master@{#389122} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
