Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2596473002:
Add more input metrics to Rappor (Closed)
|
Created:
4 years ago by Navid Zolghadr Modified:
4 years ago CC:
chromium-reviews, jam, darin-cc_chromium.org, asvitkine+watch_chromium.org, dtapuska+chromiumwatch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd more input metrics to Rappor
Add the rest of the important metrics in
RenderWidgetHostLatencyTracker to Rappor.
BUG=674985
Committed: https://crrev.com/c2e3738eb2f1d3fb4e7560cb955b17452aba4470
Cr-Commit-Position: refs/heads/master@{#440449}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Remove the empty line #
Total comments: 2
Patch Set 3 : Update summary #
Total comments: 4
Patch Set 4 : Move the assertion line out of the test function #Patch Set 5 : Move the assertion line out of the test function #
Messages
Total messages: 31 (21 generated)
The CQ bit was checked by nzolghadr@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...
nzolghadr@chromium.org changed reviewers: + avi@chromium.org, isherman@chromium.org, tdresser@chromium.org
avi@ please review the changes in: content/browser/renderer_host/input/* isherman@ please review the changes in: rappor.xml
lgtm https://codereview.chromium.org/2596473002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc (right): https://codereview.chromium.org/2596473002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc:187: one blank line
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nzolghadr@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2596473002/diff/20001/tools/metrics/rappor/ra... File tools/metrics/rappor/rappor.xml (right): https://codereview.chromium.org/2596473002/diff/20001/tools/metrics/rappor/ra... tools/metrics/rappor/rappor.xml:922: was induced by the event, no recording is made. This description looks identical to the description for "Event.Latency.ScrollUpdate.Touch.TimeToScrollUpdateSwapBegin2". Am I missing the difference? If I am, could you please try to find a way to make the difference easier to spot?
The CQ bit was checked by nzolghadr@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...
ptal https://codereview.chromium.org/2596473002/diff/20001/tools/metrics/rappor/ra... File tools/metrics/rappor/rappor.xml (right): https://codereview.chromium.org/2596473002/diff/20001/tools/metrics/rappor/ra... tools/metrics/rappor/rappor.xml:922: was induced by the event, no recording is made. On 2016/12/20 22:20:52, Ilya Sherman (Away De.29-Ja.4) wrote: > This description looks identical to the description for > "Event.Latency.ScrollUpdate.Touch.TimeToScrollUpdateSwapBegin2". Am I missing > the difference? If I am, could you please try to find a way to make the > difference easier to spot? The other summary was incorrect. I updated the other one.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks. Metrics LGTM.
LGTM with nits. https://codereview.chromium.org/2596473002/diff/40001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc (right): https://codereview.chromium.org/2596473002/diff/40001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc:94: void RapporSampleAssert(const char* rappor_name, int count) { Can this be a method returning an AssertionResult instead of having EXPECTs? This approach makes the reported line numbers hard to interpret. Details here: https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid... https://codereview.chromium.org/2596473002/diff/40001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc:187: // Rappor metrics Missing . at the end, and elsewhere.
The CQ bit was checked by nzolghadr@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 nzolghadr@chromium.org
https://codereview.chromium.org/2596473002/diff/40001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc (right): https://codereview.chromium.org/2596473002/diff/40001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc:94: void RapporSampleAssert(const char* rappor_name, int count) { On 2016/12/22 15:17:14, tdresser - OOO until Jan 3 wrote: > Can this be a method returning an AssertionResult instead of having EXPECTs? > This approach makes the reported line numbers hard to interpret. > > Details here: > https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid... Done. https://codereview.chromium.org/2596473002/diff/40001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc:187: // Rappor metrics On 2016/12/22 15:17:14, tdresser - OOO until Jan 3 wrote: > Missing . at the end, and elsewhere. Done.
The CQ bit was checked by nzolghadr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, isherman@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2596473002/#ps80001 (title: "Move the assertion line out of the test function")
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": 80001, "attempt_start_ts": 1482425935324550,
"parent_rev": "748375aa4d5ee92d2f6015782abb06d389f863ab", "commit_rev":
"9b558a52b237130ec5dcdbfdce238d57eb3b0738"}
Message was sent while issue was closed.
Description was changed from ========== Add more input metrics to Rappor Add the rest of the important metrics in RenderWidgetHostLatencyTracker to Rappor. BUG=674985 ========== to ========== Add more input metrics to Rappor Add the rest of the important metrics in RenderWidgetHostLatencyTracker to Rappor. BUG=674985 Review-Url: https://codereview.chromium.org/2596473002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add more input metrics to Rappor Add the rest of the important metrics in RenderWidgetHostLatencyTracker to Rappor. BUG=674985 Review-Url: https://codereview.chromium.org/2596473002 ========== to ========== Add more input metrics to Rappor Add the rest of the important metrics in RenderWidgetHostLatencyTracker to Rappor. BUG=674985 Committed: https://crrev.com/c2e3738eb2f1d3fb4e7560cb955b17452aba4470 Cr-Commit-Position: refs/heads/master@{#440449} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c2e3738eb2f1d3fb4e7560cb955b17452aba4470 Cr-Commit-Position: refs/heads/master@{#440449} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
