Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1983883002:
Add UMA metric to track the time saved on making events passive before pageload (Closed)
|
Created:
4 years, 7 months ago by lanwei Modified:
4 years, 7 months ago CC:
chromium-reviews, blink-reviews, dtapuska+blinkwatch_chromium.org, asvitkine+watch_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 before pageload.
In the touch scrolling intervention proposal, we could treat touchstart and the
first touchmove event listeners as passive before the page is fully loaded.
We are adding a metric that records the time we saved on making touch events passive
before pageload.
BUG=601179
Committed: https://crrev.com/85c792f7387cf9f434057b26976f31ac4305dc23
Cr-Commit-Position: refs/heads/master@{#394188}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Change latency max and comment #
Total comments: 7
Patch Set 3 : Change bucket size and comments #
Messages
Total messages: 19 (7 generated)
Description was changed from ========== pageload pageload benifit pageload BUG=601179 ========== to ========== Add UMA metric to track the time saved on making events passive before pageload. In the touch scrolling intervention proposal, we could treat touchstart and the first touchmove event listeners as passive before the page is fully loaded. We are adding a metric that records the time we saved on making touch events passive before pageload. BUG=601179 ==========
lanwei@chromium.org changed reviewers: + dtapuska@chromium.org, tdresser@chromium.org
LGTM with nits. https://codereview.chromium.org/1983883002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/1983883002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/TouchEventManager.cpp:246: // Count the handled touch starts and first touch moves before and after the page is fully loaded respectively and also their latency. Record the disposition and latency of touch starts and first... https://codereview.chromium.org/1983883002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/TouchEventManager.cpp:252: DEFINE_STATIC_LOCAL(CustomCountHistogram, eventLatencyAfterPageLoadHistogram, ("Event.Touch.TouchesAfterPageLoadLatency", 1, 10000000, 100)); Let's bump up the maximum by a factor of 10. Right now it maxes out at 10 seconds, but we want to have some information about longer tasks.
lgtm
lanwei@chromium.org changed reviewers: + isherman@chromium.org, rbyers@chromium.org
https://codereview.chromium.org/1983883002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/1983883002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/TouchEventManager.cpp:246: // Count the handled touch starts and first touch moves before and after the page is fully loaded respectively and also their latency. On 2016/05/16 17:13:55, tdresser wrote: > Record the disposition and latency of touch starts and first... Done. https://codereview.chromium.org/1983883002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/TouchEventManager.cpp:252: DEFINE_STATIC_LOCAL(CustomCountHistogram, eventLatencyAfterPageLoadHistogram, ("Event.Touch.TouchesAfterPageLoadLatency", 1, 10000000, 100)); On 2016/05/16 17:13:55, tdresser wrote: > Let's bump up the maximum by a factor of 10. Right now it maxes out at 10 > seconds, but we want to have some information about longer tasks. Done.
https://codereview.chromium.org/1983883002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/1983883002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:252: DEFINE_STATIC_LOCAL(CustomCountHistogram, eventLatencyAfterPageLoadHistogram, ("Event.Touch.TouchesAfterPageLoadLatency", 1, 100000000, 100)); Do you need 100 buckets, or would 50 suffice? (50 is the default recommended bucket count) https://codereview.chromium.org/1983883002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:258: DEFINE_STATIC_LOCAL(CustomCountHistogram, eventLatencyBeforePageLoadHistogram, ("Event.Touch.TouchesBeforePageLoadLatency", 1, 100000000, 100)); Ditto https://codereview.chromium.org/1983883002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1983883002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12421: +<histogram name="Event.Touch.TouchesAfterPageLoadLatency" units="microseconds"> nit: Perhaps "TouchLatencyAfterPageLoad" and "TouchLatencyBeforePageLoad" for the histogram below? https://codereview.chromium.org/1983883002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12424: + Time between when a touch event was generated and the event was processed nit: "was process after the page is fully loaded" -> "was processed. Recorded only for touch events that occur after the page is fully loaded" https://codereview.chromium.org/1983883002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12434: + forcing passive event listeners before the page is fully loaded. Similar change here.
core LGTM
50 buckets should be fine.
https://codereview.chromium.org/1983883002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/1983883002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:252: DEFINE_STATIC_LOCAL(CustomCountHistogram, eventLatencyAfterPageLoadHistogram, ("Event.Touch.TouchesAfterPageLoadLatency", 1, 100000000, 100)); On 2016/05/17 01:24:55, Ilya Sherman (Away May 5-15) wrote: > Do you need 100 buckets, or would 50 suffice? (50 is the default recommended > bucket count) Change to 50. https://codereview.chromium.org/1983883002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1983883002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12421: +<histogram name="Event.Touch.TouchesAfterPageLoadLatency" units="microseconds"> On 2016/05/17 01:24:55, Ilya Sherman (Away May 5-15) wrote: > nit: Perhaps "TouchLatencyAfterPageLoad" and "TouchLatencyBeforePageLoad" for > the histogram below? Done.
histograms lgtm, thanks
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, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/1983883002/#ps40001 (title: "Change bucket size and comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1983883002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1983883002/40001
Message was sent while issue was closed.
Description was changed from ========== Add UMA metric to track the time saved on making events passive before pageload. In the touch scrolling intervention proposal, we could treat touchstart and the first touchmove event listeners as passive before the page is fully loaded. We are adding a metric that records the time we saved on making touch events passive before pageload. BUG=601179 ========== to ========== Add UMA metric to track the time saved on making events passive before pageload. In the touch scrolling intervention proposal, we could treat touchstart and the first touchmove event listeners as passive before the page is fully loaded. We are adding a metric that records the time we saved on making touch events passive before pageload. BUG=601179 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add UMA metric to track the time saved on making events passive before pageload. In the touch scrolling intervention proposal, we could treat touchstart and the first touchmove event listeners as passive before the page is fully loaded. We are adding a metric that records the time we saved on making touch events passive before pageload. BUG=601179 ========== to ========== Add UMA metric to track the time saved on making events passive before pageload. In the touch scrolling intervention proposal, we could treat touchstart and the first touchmove event listeners as passive before the page is fully loaded. We are adding a metric that records the time we saved on making touch events passive before pageload. BUG=601179 Committed: https://crrev.com/85c792f7387cf9f434057b26976f31ac4305dc23 Cr-Commit-Position: refs/heads/master@{#394188} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/85c792f7387cf9f434057b26976f31ac4305dc23 Cr-Commit-Position: refs/heads/master@{#394188} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
