Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2924943004:
Add ScrollBegin.Touch/Wheel Event to UKM (Closed)
DescriptionAdd ScrollBegin.Touch/Wheel Event to UKM
Report ScrollBegin.wheel/touch TimeToScrollUpdateSwapBegin
via UKM. And add test for them.
BUG=719605, 728707
Review-Url: https://codereview.chromium.org/2924943004
Cr-Commit-Position: refs/heads/master@{#479496}
Committed: https://chromium.googlesource.com/chromium/src/+/223574a945b7d2e60bbceb96abdc8fe86b6769ba
Patch Set 1 #
Total comments: 7
Patch Set 2 : Rebase & edit UKM summary #
Messages
Total messages: 24 (13 generated)
The CQ bit was checked by eirage@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 eirage@chromium.org
eirage@chromium.org changed reviewers: + dtapuska@chromium.org, nzolghadr@chromium.org
On 2017/06/07 15:58:32, eirage wrote: seems reasonable but I defer to Navid.
nzolghadr@chromium.org changed reviewers: + tdresser@chromium.org
lgtm https://codereview.chromium.org/2924943004/diff/1/tools/metrics/ukm/ukm.xml File tools/metrics/ukm/ukm.xml (right): https://codereview.chromium.org/2924943004/diff/1/tools/metrics/ukm/ukm.xml#n... tools/metrics/ukm/ukm.xml:275: Metrics related to a scroll action caused by the generated ScrollUpdate Somewhere we need to mention this Begin thing. Maybe "first scroll action"? https://codereview.chromium.org/2924943004/diff/1/tools/metrics/ukm/ukm.xml#n... tools/metrics/ukm/ukm.xml:292: Metrics related to a scroll action caused by the generated ScrollUpdate Same here.
LGTM % rename. https://codereview.chromium.org/2924943004/diff/1/ui/latency/latency_tracker.cc File ui/latency/latency_tracker.cc (right): https://codereview.chromium.org/2924943004/diff/1/ui/latency/latency_tracker.... ui/latency/latency_tracker.cc:163: "TimeToScrollUpdateSwapBegin", original_component, TimeToScrollUpdateSwapBegin -> ".TimeToScrollUpdateSwapBegin", for consistency.
https://codereview.chromium.org/2924943004/diff/1/tools/metrics/ukm/ukm.xml File tools/metrics/ukm/ukm.xml (right): https://codereview.chromium.org/2924943004/diff/1/tools/metrics/ukm/ukm.xml#n... tools/metrics/ukm/ukm.xml:275: Metrics related to a scroll action caused by the generated ScrollUpdate On 2017/06/09 15:23:59, Navid Zolghadr wrote: > Somewhere we need to mention this Begin thing. Maybe "first scroll action"? Done. https://codereview.chromium.org/2924943004/diff/1/tools/metrics/ukm/ukm.xml#n... tools/metrics/ukm/ukm.xml:292: Metrics related to a scroll action caused by the generated ScrollUpdate On 2017/06/09 15:24:00, Navid Zolghadr wrote: > Same here. Done. https://codereview.chromium.org/2924943004/diff/1/ui/latency/latency_tracker.cc File ui/latency/latency_tracker.cc (right): https://codereview.chromium.org/2924943004/diff/1/ui/latency/latency_tracker.... ui/latency/latency_tracker.cc:163: "TimeToScrollUpdateSwapBegin", original_component, On 2017/06/09 15:36:36, tdresser wrote: > TimeToScrollUpdateSwapBegin -> ".TimeToScrollUpdateSwapBegin", for consistency. I don't think it should be a '.' before the UKM metric name. Other UMA metrics don't have it either.
https://codereview.chromium.org/2924943004/diff/1/ui/latency/latency_tracker.cc File ui/latency/latency_tracker.cc (right): https://codereview.chromium.org/2924943004/diff/1/ui/latency/latency_tracker.... ui/latency/latency_tracker.cc:163: "TimeToScrollUpdateSwapBegin", original_component, On 2017/06/09 18:54:05, eirage wrote: > On 2017/06/09 15:36:36, tdresser wrote: > > TimeToScrollUpdateSwapBegin -> ".TimeToScrollUpdateSwapBegin", for > consistency. > > I don't think it should be a '.' before the UKM metric name. Other UMA metrics > don't have it either. Sorry, you're right. I momentarily forgot how UKM structures things differently.
The CQ bit was checked by eirage@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, nzolghadr@chromium.org Link to the patchset: https://codereview.chromium.org/2924943004/#ps20001 (title: "Rebase & edit UKM summary")
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 eirage@chromium.org
Description was changed from ========== Add ScrollBegin.Touch/Wheel Event to UKM Report ScrollBegin.wheel/touch TimeToScrollUpdateSwapBegin via UKM. And add test for them. BUG=719605 ========== to ========== Add ScrollBegin.Touch/Wheel Event to UKM Report ScrollBegin.wheel/touch TimeToScrollUpdateSwapBegin via UKM. And add test for them. BUG=719605, 728707 ==========
eirage@chromium.org changed reviewers: + rkaplow@chromium.org
rkaplow@chromium.org: Please review changes in metrics/ukm/ukm.xml
lgtm
The CQ bit was checked by eirage@chromium.org
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": 20001, "attempt_start_ts": 1497465121488720,
"parent_rev": "e0bcc6b51511f5f1e6692dfe6f75cad978452cd7", "commit_rev":
"223574a945b7d2e60bbceb96abdc8fe86b6769ba"}
Message was sent while issue was closed.
Description was changed from ========== Add ScrollBegin.Touch/Wheel Event to UKM Report ScrollBegin.wheel/touch TimeToScrollUpdateSwapBegin via UKM. And add test for them. BUG=719605, 728707 ========== to ========== Add ScrollBegin.Touch/Wheel Event to UKM Report ScrollBegin.wheel/touch TimeToScrollUpdateSwapBegin via UKM. And add test for them. BUG=719605, 728707 Review-Url: https://codereview.chromium.org/2924943004 Cr-Commit-Position: refs/heads/master@{#479496} Committed: https://chromium.googlesource.com/chromium/src/+/223574a945b7d2e60bbceb96abdc... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/223574a945b7d2e60bbceb96abdc... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
