Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2492793004:
Add TimeToScrollUpdateSwapBegin2 in RAPPOR (Closed)
|
Created:
4 years, 1 month ago by Navid Zolghadr Modified:
4 years ago CC:
chromium-reviews, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, dtapuska+chromiumwatch_chromium.org, darin-cc_chromium.org, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd TimeToScrollUpdateSwapBegin2 in RAPPOR
Add first rappor metric in content for input.
The metric uses the same data as the existing
UMA metric:
Event.Latency.ScrollUpdate.Touch.
TimeToScrollUpdateSwapBegin2
adding the domain information on top.
BUG=663471
Committed: https://crrev.com/3fddfc508938ba41dbaacebb204cbf45e6e99b83
Cr-Commit-Position: refs/heads/master@{#438297}
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 9
Patch Set 3 : Rebase on top of plumbing #
Total comments: 5
Patch Set 4 : add tests #Messages
Total messages: 40 (25 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
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: + dtapuska@chromium.org, holte@chromium.org, tdresser@chromium.org
I used one way of exposing the url and the rappor service from chrome/browser to content. There were quite a lot of interfaces and abstractions in the path. Let me know if you think of a better way around or I missed anything. I also had a few questions inline. https://codereview.chromium.org/2492793004/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2492793004/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:301: render_widget_host_impl->delegate()->getRapporCommittedUrl()); I was going to create some helper methods in my next CL along with the next Rappor metric maybe somewhere in rappor_utils similar to the UMA ones we have. Steven, is that okay or are there maybe existing ones? I couldn't find any as far as I searched. https://codereview.chromium.org/2492793004/diff/20001/tools/metrics/rappor/ra... File tools/metrics/rappor/rappor.xml (right): https://codereview.chromium.org/2492793004/diff/20001/tools/metrics/rappor/ra... tools/metrics/rappor/rappor.xml:913: name="Event.Latency.ScrollUpdate.Touch.TimeToScrollUpdateSwapBegin2.Rappor" Steven, can these name be the same as the ones we use for UMA assuming they are being reported differently and different paths? Can I just remove the ".rappor" at the end then? https://codereview.chromium.org/2492793004/diff/20001/tools/metrics/rappor/ra... tools/metrics/rappor/rappor.xml:919: membership in a coarse histogram of Touch.TimeToScrollUpdateSwapBegin2. Tim, can you suggest something for the description of the metric? I couldn't find any existing description for the UMA version.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2492793004/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2492793004/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:301: render_widget_host_impl->delegate()->getRapporCommittedUrl()); On 2016/11/10 21:49:19, Navid Zolghadr wrote: > I was going to create some helper methods in my next CL along with the next > Rappor metric maybe somewhere in rappor_utils similar to the UMA ones we have. > > Steven, is that okay or are there maybe existing ones? I couldn't find any as > far as I searched. It seems like this value is non-PII, so it should be fine to send this as a non-noised int field e.g SetUInt64Field("Latency", delta.InMicroseconds, rappor::NO_NOISE); https://codereview.chromium.org/2492793004/diff/20001/tools/metrics/rappor/ra... File tools/metrics/rappor/rappor.xml (right): https://codereview.chromium.org/2492793004/diff/20001/tools/metrics/rappor/ra... tools/metrics/rappor/rappor.xml:913: name="Event.Latency.ScrollUpdate.Touch.TimeToScrollUpdateSwapBegin2.Rappor" On 2016/11/10 21:49:19, Navid Zolghadr wrote: > Steven, can these name be the same as the ones we use for UMA assuming they are > being reported differently and different paths? Can I just remove the ".rappor" > at the end then? There shouldn't be any issues with using the same name as a histogram.
We should probably split this into two separate patches, one with just the plumbing (and tests for the plumbing) and a separate patch adding the actual metric. https://codereview.chromium.org/2492793004/diff/20001/tools/metrics/rappor/ra... File tools/metrics/rappor/rappor.xml (right): https://codereview.chromium.org/2492793004/diff/20001/tools/metrics/rappor/ra... tools/metrics/rappor/rappor.xml:919: membership in a coarse histogram of Touch.TimeToScrollUpdateSwapBegin2. On 2016/11/10 21:49:19, Navid Zolghadr wrote: > Tim, can you suggest something for the description of the metric? I couldn't > find any existing description for the UMA version. From histograms.xml. Time between initial creation of a touch event and the start of the frame swap on the GPU service caused by the generated ScrollUpdate gesture event if that ScrollUpdate is the first such event in a given scroll gesture event sequence. If no swap was induced by the event, no recording is made.
nick@chromium.org changed reviewers: + nick@chromium.org
https://codereview.chromium.org/2492793004/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2492793004/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:253: return 5; What about 6?
Description was changed from ========== Add TimeToScrollUpdateSwapBegin2 in RAPPOR Add first rappor metric in content with necessary plumbing. The metric uses the same data as the existing UMA metric: Event.Latency.ScrollUpdate.Touch. TimeToScrollUpdateSwapBegin2 BUG=663471 ========== to ========== Add TimeToScrollUpdateSwapBegin2 in RAPPOR Add first rappor metric in content for input. The metric uses the same data as the existing UMA metric: Event.Latency.ScrollUpdate.Touch. TimeToScrollUpdateSwapBegin2 adding the domain information on top. BUG=663471 ==========
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: + jam@chromium.org
ptal. Any idea how/where I can test this functionality? https://codereview.chromium.org/2492793004/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2492793004/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:253: return 5; On 2016/12/01 23:33:46, ncarter wrote: > What about 6? This is now removed as I reported it without any bucket. https://codereview.chromium.org/2492793004/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:301: render_widget_host_impl->delegate()->getRapporCommittedUrl()); On 2016/11/10 22:49:45, Steven Holte wrote: > On 2016/11/10 21:49:19, Navid Zolghadr wrote: > > I was going to create some helper methods in my next CL along with the next > > Rappor metric maybe somewhere in rappor_utils similar to the UMA ones we have. > > > > Steven, is that okay or are there maybe existing ones? I couldn't find any as > > far as I searched. > > It seems like this value is non-PII, so it should be fine to send this as a > non-noised int field e.g SetUInt64Field("Latency", delta.InMicroseconds, > rappor::NO_NOISE); Done. I didn't find anywhere else that this is used. I used uint64-field in rappor.xml. Is that correct?
lgtm https://codereview.chromium.org/2492793004/diff/40001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.h (right): https://codereview.chromium.org/2492793004/diff/40001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.h:26: RenderWidgetHostLatencyTracker(RenderWidgetHostImpl*); single-arg ctor should be explicit
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/2492793004/diff/40001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2492793004/diff/40001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:299: rappor::NO_NOISE); Regarding testing, the simplest thing to do would be a unittest that calls this function directly (you'd have to make it private static in RWHLatencyTracker w/ a test friend), since it's pretty well parameterized. If your test derives from RenderViewHostImplTestHarness you'll have a WebContents available from the test fixture, which you can set the URL of and obtain a RWHImpl from. For testing the rappor side effects, you can create a version of TestContentBrowserClient that provides a mock implementation of RapporService. A unittest can install its own content browser client using the pattern here: https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_vie... The UMA side effects can be tested via base/histogram_tester.h
Can you inject a MockRapporService by overriding GetRapporService? https://codereview.chromium.org/2492793004/diff/40001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2492793004/diff/40001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:301: "Event.Latency.ScrollUpdate.Touch.TimeToScrollUpdateSwapBegin2", Should we add Event.Latency.ScrollBegin.Touch.TimeToScrollUpdateSwapBegin2 at the same time?
On 2016/12/08 17:37:29, Navid Zolghadr wrote: > ptal. > > Any idea how/where I can test this functionality? > > https://codereview.chromium.org/2492793004/diff/20001/content/browser/rendere... > File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc > (right): > > https://codereview.chromium.org/2492793004/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:253: > return 5; > On 2016/12/01 23:33:46, ncarter wrote: > > What about 6? > > This is now removed as I reported it without any bucket. > > https://codereview.chromium.org/2492793004/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:301: > render_widget_host_impl->delegate()->getRapporCommittedUrl()); > On 2016/11/10 22:49:45, Steven Holte wrote: > > On 2016/11/10 21:49:19, Navid Zolghadr wrote: > > > I was going to create some helper methods in my next CL along with the next > > > Rappor metric maybe somewhere in rappor_utils similar to the UMA ones we > have. > > > > > > Steven, is that okay or are there maybe existing ones? I couldn't find any > as > > > far as I searched. > > > > It seems like this value is non-PII, so it should be fine to send this as a > > non-noised int field e.g SetUInt64Field("Latency", delta.InMicroseconds, > > rappor::NO_NOISE); > > Done. I didn't find anywhere else that this is used. I used uint64-field in > rappor.xml. Is that correct? This should be correct. For testing, you could plumb TestRapporServiceImpl into your function, and use GetRecordedSampleForMetric to test the recorded values.
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/2492793004/diff/40001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2492793004/diff/40001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:301: "Event.Latency.ScrollUpdate.Touch.TimeToScrollUpdateSwapBegin2", On 2016/12/08 18:00:17, tdresser wrote: > Should we add Event.Latency.ScrollBegin.Touch.TimeToScrollUpdateSwapBegin2 at > the same time? Let me work on adding that and some refactoring between the multiple metrics in another CL. I just didn't want to make this first metric patch unnecessarily big. https://codereview.chromium.org/2492793004/diff/40001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.h (right): https://codereview.chromium.org/2492793004/diff/40001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.h:26: RenderWidgetHostLatencyTracker(RenderWidgetHostImpl*); On 2016/12/08 17:47:14, ncarter wrote: > single-arg ctor should be explicit Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/09 19:13:28, Navid Zolghadr wrote: > ptal. > > https://codereview.chromium.org/2492793004/diff/40001/content/browser/rendere... > File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc > (right): > > https://codereview.chromium.org/2492793004/diff/40001/content/browser/rendere... > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:301: > "Event.Latency.ScrollUpdate.Touch.TimeToScrollUpdateSwapBegin2", > On 2016/12/08 18:00:17, tdresser wrote: > > Should we add Event.Latency.ScrollBegin.Touch.TimeToScrollUpdateSwapBegin2 at > > the same time? > > Let me work on adding that and some refactoring between the multiple metrics in > another CL. I just didn't want to make this first metric patch unnecessarily > big. > > https://codereview.chromium.org/2492793004/diff/40001/content/browser/rendere... > File content/browser/renderer_host/input/render_widget_host_latency_tracker.h > (right): > > https://codereview.chromium.org/2492793004/diff/40001/content/browser/rendere... > content/browser/renderer_host/input/render_widget_host_latency_tracker.h:26: > RenderWidgetHostLatencyTracker(RenderWidgetHostImpl*); > On 2016/12/08 17:47:14, ncarter wrote: > > single-arg ctor should be explicit > > Done. Steven, do the rappor change for the test and rappor.xml sound alright?
On 2016/12/13 15:54:33, Navid Zolghadr wrote: > On 2016/12/09 19:13:28, Navid Zolghadr wrote: > > ptal. > > > > > https://codereview.chromium.org/2492793004/diff/40001/content/browser/rendere... > > File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc > > (right): > > > > > https://codereview.chromium.org/2492793004/diff/40001/content/browser/rendere... > > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:301: > > "Event.Latency.ScrollUpdate.Touch.TimeToScrollUpdateSwapBegin2", > > On 2016/12/08 18:00:17, tdresser wrote: > > > Should we add Event.Latency.ScrollBegin.Touch.TimeToScrollUpdateSwapBegin2 > at > > > the same time? > > > > Let me work on adding that and some refactoring between the multiple metrics > in > > another CL. I just didn't want to make this first metric patch unnecessarily > > big. > > > > > https://codereview.chromium.org/2492793004/diff/40001/content/browser/rendere... > > File content/browser/renderer_host/input/render_widget_host_latency_tracker.h > > (right): > > > > > https://codereview.chromium.org/2492793004/diff/40001/content/browser/rendere... > > content/browser/renderer_host/input/render_widget_host_latency_tracker.h:26: > > RenderWidgetHostLatencyTracker(RenderWidgetHostImpl*); > > On 2016/12/08 17:47:14, ncarter wrote: > > > single-arg ctor should be explicit > > > > Done. > > Steven, do the rappor change for the test and rappor.xml sound alright? lgtm
The CQ bit was checked by nzolghadr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org Link to the patchset: https://codereview.chromium.org/2492793004/#ps60001 (title: "add tests")
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": 60001, "attempt_start_ts": 1481658995426500,
"parent_rev": "be11cc6c29cfa7684b73aa55e2c09c48dca4a586", "commit_rev":
"d4a59da1b0b492d79624fcef4e6ab4641836ab39"}
Message was sent while issue was closed.
Description was changed from ========== Add TimeToScrollUpdateSwapBegin2 in RAPPOR Add first rappor metric in content for input. The metric uses the same data as the existing UMA metric: Event.Latency.ScrollUpdate.Touch. TimeToScrollUpdateSwapBegin2 adding the domain information on top. BUG=663471 ========== to ========== Add TimeToScrollUpdateSwapBegin2 in RAPPOR Add first rappor metric in content for input. The metric uses the same data as the existing UMA metric: Event.Latency.ScrollUpdate.Touch. TimeToScrollUpdateSwapBegin2 adding the domain information on top. BUG=663471 Review-Url: https://codereview.chromium.org/2492793004 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add TimeToScrollUpdateSwapBegin2 in RAPPOR Add first rappor metric in content for input. The metric uses the same data as the existing UMA metric: Event.Latency.ScrollUpdate.Touch. TimeToScrollUpdateSwapBegin2 adding the domain information on top. BUG=663471 Review-Url: https://codereview.chromium.org/2492793004 ========== to ========== Add TimeToScrollUpdateSwapBegin2 in RAPPOR Add first rappor metric in content for input. The metric uses the same data as the existing UMA metric: Event.Latency.ScrollUpdate.Touch. TimeToScrollUpdateSwapBegin2 adding the domain information on top. BUG=663471 Committed: https://crrev.com/3fddfc508938ba41dbaacebb204cbf45e6e99b83 Cr-Commit-Position: refs/heads/master@{#438297} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3fddfc508938ba41dbaacebb204cbf45e6e99b83 Cr-Commit-Position: refs/heads/master@{#438297} |
