Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1955643002:
Add UMA metric to track the time saved on making events passive during fling (Closed)
|
Created:
4 years, 7 months ago by lanwei Modified:
4 years, 7 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, dtapuska+blinkwatch_chromium.org, blink-reviews-events_chromium.org, eae+blinkwatch, tdresser+watch_chromium.org, jam, dtapuska+chromiumwatch_chromium.org, blink-reviews, dglazkov+blink, darin-cc_chromium.org, asvitkine+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, kinuko+watch, blink-reviews-api_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA metric to track the time saved on making events passive during fling.
In the touch scrolling intervention proposal, we could treat all touchstart event
listeners as passive while there’s an active fling animation.
We are adding a metric that records the time we saved on making touchstarts passive
during fling.
This patch depends on issue 1923973002.
BUG=607245
Committed: https://crrev.com/6656a45aef6475f32b020285251f4782d8eaeb31
Cr-Commit-Position: refs/heads/master@{#394560}
Patch Set 1 : Set dispatch type to forcepassive #
Total comments: 2
Patch Set 2 : Create a new histogram #Patch Set 3 : #
Total comments: 2
Patch Set 4 : Add one more histogram #
Total comments: 3
Patch Set 5 : Add a test #
Total comments: 7
Patch Set 6 : Change bucket size and comments #
Messages
Total messages: 37 (18 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== benefit fling fling BUG=607245 ========== to ========== Add UMA metric for tracking the time saved on making events passive while fling is happening. In the touch scrolling intervention proposal, we could treat all touchstart event listeners as passive while there’s an active fling animation. We are adding a metric that records the time we saved on making touchstarts passive during fling. This patch depends on issue 1923973002. BUG=607245 ==========
lanwei@chromium.org changed reviewers: + dtapuska@chromium.org, tdresser@chromium.org
Description was changed from ========== Add UMA metric for tracking the time saved on making events passive while fling is happening. In the touch scrolling intervention proposal, we could treat all touchstart event listeners as passive while there’s an active fling animation. We are adding a metric that records the time we saved on making touchstarts passive during fling. This patch depends on issue 1923973002. BUG=607245 ========== to ========== Add UMA metric for tracking the time saved on making events passive while fling is happening. In the touch scrolling intervention proposal, we could treat all touchstart event listeners as passive while there’s an active fling animation. We are adding a metric that records the time we saved on making touchstarts passive during fling. This patch depends on issue 1923973002. BUG=607245 ==========
Description was changed from ========== Add UMA metric for tracking the time saved on making events passive while fling is happening. In the touch scrolling intervention proposal, we could treat all touchstart event listeners as passive while there’s an active fling animation. We are adding a metric that records the time we saved on making touchstarts passive during fling. This patch depends on issue 1923973002. BUG=607245 ========== to ========== Add UMA metric for tracking the time saved on making events passive while fling is happening. In the touch scrolling intervention proposal, we could treat all touchstart event listeners as passive while there’s an active fling animation. We are adding a metric that records the time we saved on making touchstarts passive during fling. This patch depends on issue 1923973002. BUG=607245 ==========
Description was changed from ========== Add UMA metric for tracking the time saved on making events passive while fling is happening. In the touch scrolling intervention proposal, we could treat all touchstart event listeners as passive while there’s an active fling animation. We are adding a metric that records the time we saved on making touchstarts passive during fling. This patch depends on issue 1923973002. BUG=607245 ========== to ========== Add UMA metric to track the time saved on making events passive during fling. In the touch scrolling intervention proposal, we could treat all touchstart event listeners as passive while there’s an active fling animation. We are adding a metric that records the time we saved on making touchstarts passive during fling. This patch depends on issue 1923973002. BUG=607245 ==========
Description was changed from ========== Add UMA metric to track the time saved on making events passive during fling. In the touch scrolling intervention proposal, we could treat all touchstart event listeners as passive while there’s an active fling animation. We are adding a metric that records the time we saved on making touchstarts passive during fling. This patch depends on issue 1923973002. BUG=607245 ========== to ========== Add UMA metric to track the time saved on making events passive during fling. In the touch scrolling intervention proposal, we could treat all touchstart event listeners as passive while there’s an active fling animation. We are adding a metric that records the time we saved on making touchstarts passive during fling. This patch depends on issue 1923973002. BUG=607245 ==========
Patchset #1 (id:20001) has been deleted
https://codereview.chromium.org/1955643002/diff/10016/content/renderer/input/... File content/renderer/input/main_thread_event_queue.cc (right): https://codereview.chromium.org/1955643002/diff/10016/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:79: blink::WebInputEvent::ListenersForcedNonBlockingPassive; If you set this then you need to make sure an ack is returned. I think this might be the wrong place to do this. I think it needs to be an action in the input_handler_proxy because it needs to take action on doing the default action and then setting the event as being dispatched passively. https://codereview.chromium.org/1955643002/diff/10016/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/PlatformTouchEvent.h (left): https://codereview.chromium.org/1955643002/diff/10016/third_party/WebKit/Sour... third_party/WebKit/Source/platform/PlatformTouchEvent.h:42: bool cancelable() const { return m_dispatchType == PlatformEvent::Blocking; } ok this seems wrong to me; if listeners are forced to be passive; then it shouldn't be cancelable.
This looks like it's doing a lot more than adding an UMA metric.
https://codereview.chromium.org/1955643002/diff/70001/content/renderer/input/... File content/renderer/input/render_widget_input_handler.cc (right): https://codereview.chromium.org/1955643002/diff/70001/content/renderer/input/... content/renderer/input/render_widget_input_handler.cc:343: "Event.Touch.ExperiencedForcedPassiveLatency", We'll want to record during fling and outside of fling, so we can compare them.
Patchset #4 (id:90001) has been deleted
https://codereview.chromium.org/1955643002/diff/70001/content/renderer/input/... File content/renderer/input/render_widget_input_handler.cc (right): https://codereview.chromium.org/1955643002/diff/70001/content/renderer/input/... content/renderer/input/render_widget_input_handler.cc:343: "Event.Touch.ExperiencedForcedPassiveLatency", On 2016/05/06 18:49:19, tdresser wrote: > We'll want to record during fling and outside of fling, so we can compare them. Done.
https://codereview.chromium.org/1955643002/diff/110001/content/renderer/input... File content/renderer/input/render_widget_input_handler.cc (right): https://codereview.chromium.org/1955643002/diff/110001/content/renderer/input... content/renderer/input/render_widget_input_handler.cc:341: if (static_cast<const WebTouchEvent&>(input_event) Let's only perform the cast once, as soon as we know it's a touch event. https://codereview.chromium.org/1955643002/diff/110001/content/renderer/input... content/renderer/input/render_widget_input_handler.cc:349: "Event.Touch.TouchStartNoFlingLatency", TouchStartNoFlingLatency -> TouchStartOutsideFlingLatency, to be more parallel to TouchStartDuringFlingLatency. https://codereview.chromium.org/1955643002/diff/110001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1955643002/diff/110001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:12331: + while there is no active fling animation. The tenses are a bit weird here. "while there is" -> "when there was"?
Also, let's add a test here. See https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... for a similar example.
Patchset #5 (id:130001) has been deleted
Patchset #5 (id:150001) has been deleted
LGTM, thanks. https://codereview.chromium.org/1955643002/diff/170001/content/renderer/input... File content/renderer/input/render_widget_input_handler.cc (right): https://codereview.chromium.org/1955643002/diff/170001/content/renderer/input... content/renderer/input/render_widget_input_handler.cc:332: WebTouchEvent touch = static_cast<const WebTouchEvent&>(input_event); Make this a const ref.
On 2016/05/13 13:32:59, tdresser wrote: > LGTM, thanks. > > https://codereview.chromium.org/1955643002/diff/170001/content/renderer/input... > File content/renderer/input/render_widget_input_handler.cc (right): > > https://codereview.chromium.org/1955643002/diff/170001/content/renderer/input... > content/renderer/input/render_widget_input_handler.cc:332: WebTouchEvent touch = > static_cast<const WebTouchEvent&>(input_event); > Make this a const ref. lgtm % tim's comment.
isherman@chromium.org changed reviewers: + isherman@chromium.org
https://codereview.chromium.org/1955643002/diff/170001/content/renderer/input... File content/renderer/input/render_widget_input_handler.cc (right): https://codereview.chromium.org/1955643002/diff/170001/content/renderer/input... content/renderer/input/render_widget_input_handler.cc:345: 10000000, 100); Would 50 buckets suffice here? https://codereview.chromium.org/1955643002/diff/170001/content/renderer/input... content/renderer/input/render_widget_input_handler.cc:350: 10000000, 100); Ditto https://codereview.chromium.org/1955643002/diff/170001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1955643002/diff/170001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:12318: +<histogram name="Event.Touch.TouchStartDuringFlingLatency" units="microseconds"> nit: Perhaps "TouchStartLatencyDuringFling", and a similar change below? https://codereview.chromium.org/1955643002/diff/170001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:12332: + processed when there was no active fling animation. nit: "was processed when" -> "was processed. Recorded only when" (and a similar change above)
lanwei@chromium.org changed reviewers: + sievers@chromium.org
sievers@chromium.org: Please review changes in content/renderer/render_widget_unittest.cc Thank you very much. https://codereview.chromium.org/1955643002/diff/170001/content/renderer/input... File content/renderer/input/render_widget_input_handler.cc (right): https://codereview.chromium.org/1955643002/diff/170001/content/renderer/input... content/renderer/input/render_widget_input_handler.cc:332: WebTouchEvent touch = static_cast<const WebTouchEvent&>(input_event); On 2016/05/13 13:32:59, tdresser wrote: > Make this a const ref. Done. https://codereview.chromium.org/1955643002/diff/170001/content/renderer/input... content/renderer/input/render_widget_input_handler.cc:345: 10000000, 100); On 2016/05/17 01:27:29, Ilya Sherman wrote: > Would 50 buckets suffice here? Yes.
Histograms LGTM, thanks.
Patchset #6 (id:190001) has been deleted
lgtm
The CQ bit was checked by lanwei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtapuska@chromium.org, tdresser@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1955643002/#ps210001 (title: "Change bucket size and comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1955643002/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1955643002/210001
Message was sent while issue was closed.
Description was changed from ========== Add UMA metric to track the time saved on making events passive during fling. In the touch scrolling intervention proposal, we could treat all touchstart event listeners as passive while there’s an active fling animation. We are adding a metric that records the time we saved on making touchstarts passive during fling. This patch depends on issue 1923973002. BUG=607245 ========== to ========== Add UMA metric to track the time saved on making events passive during fling. In the touch scrolling intervention proposal, we could treat all touchstart event listeners as passive while there’s an active fling animation. We are adding a metric that records the time we saved on making touchstarts passive during fling. This patch depends on issue 1923973002. BUG=607245 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:210001)
Message was sent while issue was closed.
Description was changed from ========== Add UMA metric to track the time saved on making events passive during fling. In the touch scrolling intervention proposal, we could treat all touchstart event listeners as passive while there’s an active fling animation. We are adding a metric that records the time we saved on making touchstarts passive during fling. This patch depends on issue 1923973002. BUG=607245 ========== to ========== Add UMA metric to track the time saved on making events passive during fling. In the touch scrolling intervention proposal, we could treat all touchstart event listeners as passive while there’s an active fling animation. We are adding a metric that records the time we saved on making touchstarts passive during fling. This patch depends on issue 1923973002. BUG=607245 Committed: https://crrev.com/6656a45aef6475f32b020285251f4782d8eaeb31 Cr-Commit-Position: refs/heads/master@{#394560} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6656a45aef6475f32b020285251f4782d8eaeb31 Cr-Commit-Position: refs/heads/master@{#394560} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
