Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2888153002:
Add TimeToScrollUpdateSwapBegin2 to UKM (Closed)
|
Created:
3 years, 7 months ago by Navid Zolghadr Modified:
3 years, 6 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org, asvitkine+watch_chromium.org, dtapuska+chromiumwatch_chromium.org, ncarter (slow), Charlie Reis Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd TimeToScrollUpdateSwapBegin2 to UKM
Report TimeToScrollUpdateSwapBegin2 via UKM
and also add the testing mechanisms in
render_widget_host_latency_tracker_unittest
BUG=723677, 728707
Review-Url: https://codereview.chromium.org/2888153002
Cr-Commit-Position: refs/heads/master@{#476670}
Committed: https://chromium.googlesource.com/chromium/src/+/04ce77fc2727d5100b89719d7bd0256797b6dfa4
Patch Set 1 #Patch Set 2 : Rebase and use new constructs #
Total comments: 18
Patch Set 3 : Removing the old plumbing and use UkmRecorder::Get() #
Total comments: 1
Patch Set 4 : Rename the test function #Messages
Total messages: 45 (26 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: + asvitkine@chromium.org, rkaplow@chromium.org, tdresser@chromium.org
https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:334: GetContentClient()->browser()->GetUkmRecorder(); I recently added this plumbing in this CL: https://codereview.chromium.org/2877673003/ But at the same time there was another change exposing a static function ukm::UkmRecorder::Get() https://cs.chromium.org/chromium/src/components/ukm/public/ukm_recorder.cc?ty... to get the UkmRecorder. So the question I have is which one to use? Is the plumbing I added is alright or is it better to just use ukm::UkmRecorder::Get() instead to get the UkmRecorder and remove my plumbing? Also do I need to do anything special to get the UkmRecorder instance created for this process (as per comment above ukm::UkmRecorder::Get())
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
On 2017/05/26 15:15:43, Navid Zolghadr wrote: > https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... > File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc > (right): > > https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:334: > GetContentClient()->browser()->GetUkmRecorder(); > I recently added this plumbing in this CL: > https://codereview.chromium.org/2877673003/ > > But at the same time there was another change exposing a static function > ukm::UkmRecorder::Get() > https://cs.chromium.org/chromium/src/components/ukm/public/ukm_recorder.cc?ty... > > to get the UkmRecorder. So the question I have is which one to use? Is the > plumbing I added is alright or is it better to just use ukm::UkmRecorder::Get() > instead to get the UkmRecorder and remove my plumbing? > Also do I need to do anything special to get the UkmRecorder instance created > for this process (as per comment above ukm::UkmRecorder::Get()) ping
https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:332: int32_t RenderWidgetHostLatencyTracker::GetUkmSourceId() { The name of this implies to me that it always gets the same id, but it looks like it actually gets a new id each time. Which is correct? https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:357: std::unique_ptr<ukm::UkmEntryBuilder> builder = Does this need to be a unique_ptr, or can it be stack allocated? https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc (right): https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc:150: size_t expected_metrics_index = 0; It isn't clear from the name what this is. https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc:151: for (size_t i = 0; i < ukm_recoder->entries_count(); ++i) { Could this be a range based for? https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc:635: EXPECT_EQ(1U, test_ukm_recorder->entries_count()); Should we be using UkmReportAssert here? https://codereview.chromium.org/2888153002/diff/20001/tools/metrics/ukm/ukm.xml File tools/metrics/ukm/ukm.xml (right): https://codereview.chromium.org/2888153002/diff/20001/tools/metrics/ukm/ukm.x... tools/metrics/ukm/ukm.xml:246: Latency in microseconds representing Touch.TimeToScrollUpdateSwapBegin. This summary is a bit confusing. Do we need a metric specific summary here? Will this event ever have multiple entries? Long term, it would make sense to have an event which shared the top level metric as well as the breakdown metrics. Perhaps we should name things such that this naturally extends to include the breakdowns?
Description was changed from ========== Add TimeToScrollUpdateSwapBegin2 to UKM Report TimeToScrollUpdateSwapBegin2 via UKM and also add the testing mechanisms in render_widget_host_latency_tracker_unittest BUG=723677 ========== to ========== Add TimeToScrollUpdateSwapBegin2 to UKM Report TimeToScrollUpdateSwapBegin2 via UKM and also add the testing mechanisms in render_widget_host_latency_tracker_unittest BUG=723677 ==========
rkaplow@chromium.org changed reviewers: + oysteine@chromium.org
You also need a Launch bug to associate any new UKMs with. If you don't have one for this already you can use a generic UKM m60 for example: https://bugs.chromium.org/p/chromium/issues/detail?id=717692&q=ukm%20m60&cols... https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:334: GetContentClient()->browser()->GetUkmRecorder(); On 2017/05/26 15:15:43, Navid Zolghadr wrote: > I recently added this plumbing in this CL: > https://codereview.chromium.org/2877673003/ > > But at the same time there was another change exposing a static function > ukm::UkmRecorder::Get() > https://cs.chromium.org/chromium/src/components/ukm/public/ukm_recorder.cc?ty... > > to get the UkmRecorder. So the question I have is which one to use? Is the > plumbing I added is alright or is it better to just use ukm::UkmRecorder::Get() > instead to get the UkmRecorder and remove my plumbing? > Also do I need to do anything special to get the UkmRecorder instance created > for this process (as per comment above ukm::UkmRecorder::Get()) +oystein - can you answer https://codereview.chromium.org/2888153002/diff/20001/tools/metrics/ukm/ukm.xml File tools/metrics/ukm/ukm.xml (right): https://codereview.chromium.org/2888153002/diff/20001/tools/metrics/ukm/ukm.x... tools/metrics/ukm/ukm.xml:246: Latency in microseconds representing Touch.TimeToScrollUpdateSwapBegin. On 2017/05/30 16:52:07, tdresser wrote: > This summary is a bit confusing. Do we need a metric specific summary here? Will > this event ever have multiple entries? > > Long term, it would make sense to have an event which shared the top level > metric as well as the breakdown metrics. Perhaps we should name things such that > this naturally extends to include the breakdowns? I agree - does it make more sense for the Event to be a more generic Scroll Touch? Is there other metrics related to it that may want to eb added and associated with it?
On 2017/05/26 at 15:15:43, nzolghadr wrote: > https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... > File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): > > https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:334: GetContentClient()->browser()->GetUkmRecorder(); > I recently added this plumbing in this CL: > https://codereview.chromium.org/2877673003/ > > But at the same time there was another change exposing a static function > ukm::UkmRecorder::Get() > https://cs.chromium.org/chromium/src/components/ukm/public/ukm_recorder.cc?ty... > > to get the UkmRecorder. So the question I have is which one to use? Is the plumbing I added is alright or is it better to just use ukm::UkmRecorder::Get() instead to get the UkmRecorder and remove my plumbing? > Also do I need to do anything special to get the UkmRecorder instance created for this process (as per comment above ukm::UkmRecorder::Get()) It seems to me the ukm::UkmRecorder::Get() is what should be used, since it's more generalized (usable from other components, etc). Care still has to be taken that it's just used from the browser process; the Mojo interface landing in https://codereview.chromium.org/2882353002/ as we speak should be used from other processes.
asvitkine@chromium.org changed reviewers: - asvitkine@chromium.org
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
ptal https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:332: int32_t RenderWidgetHostLatencyTracker::GetUkmSourceId() { On 2017/05/30 16:52:07, tdresser wrote: > The name of this implies to me that it always gets the same id, but it looks > like it actually gets a new id each time. > > Which is correct? I wasn't sure if I had to reuse the id. But now that I look again at other samples they do use the same id. https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:334: GetContentClient()->browser()->GetUkmRecorder(); On 2017/05/30 20:03:32, rkaplow wrote: > On 2017/05/26 15:15:43, Navid Zolghadr wrote: > > I recently added this plumbing in this CL: > > https://codereview.chromium.org/2877673003/ > > > > But at the same time there was another change exposing a static function > > ukm::UkmRecorder::Get() > > > https://cs.chromium.org/chromium/src/components/ukm/public/ukm_recorder.cc?ty... > > > > to get the UkmRecorder. So the question I have is which one to use? Is the > > plumbing I added is alright or is it better to just use > ukm::UkmRecorder::Get() > > instead to get the UkmRecorder and remove my plumbing? > > Also do I need to do anything special to get the UkmRecorder instance created > > for this process (as per comment above ukm::UkmRecorder::Get()) > > +oystein - can you answer I started using ukm::UkmRecorder::Get() and removed my own plumbing. https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:357: std::unique_ptr<ukm::UkmEntryBuilder> builder = On 2017/05/30 16:52:07, tdresser wrote: > Does this need to be a unique_ptr, or can it be stack allocated? You mean the normal object with no pointer? But the GetEntryBuilder returns a pointer. The usage instructions are here: https://cs.chromium.org/chromium/src/components/ukm/public/mojo_ukm_recorder.... https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc (right): https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc:150: size_t expected_metrics_index = 0; On 2017/05/30 16:52:07, tdresser wrote: > It isn't clear from the name what this is. > renamed. https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc:151: for (size_t i = 0; i < ukm_recoder->entries_count(); ++i) { On 2017/05/30 16:52:07, tdresser wrote: > Could this be a range based for? It seems that ukm_recorder is not (and does not return) an iterable object. https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc:635: EXPECT_EQ(1U, test_ukm_recorder->entries_count()); On 2017/05/30 16:52:07, tdresser wrote: > Should we be using UkmReportAssert here? It is used earlier in the function. I got rid of this to make it more clear as I'm going to add the test of the metrics right after this and they will verify anything that will be reported anyway. https://codereview.chromium.org/2888153002/diff/20001/tools/metrics/ukm/ukm.xml File tools/metrics/ukm/ukm.xml (right): https://codereview.chromium.org/2888153002/diff/20001/tools/metrics/ukm/ukm.x... tools/metrics/ukm/ukm.xml:246: Latency in microseconds representing Touch.TimeToScrollUpdateSwapBegin. On 2017/05/30 20:03:32, rkaplow wrote: > On 2017/05/30 16:52:07, tdresser wrote: > > This summary is a bit confusing. Do we need a metric specific summary here? > Will > > this event ever have multiple entries? > > > > Long term, it would make sense to have an event which shared the top level > > metric as well as the breakdown metrics. Perhaps we should name things such > that > > this naturally extends to include the breakdowns? > > I agree - does it make more sense for the Event to be a more generic Scroll > Touch? Is there other metrics related to it that may want to eb added and > associated with it? I did some renaming here. Let me know if this looks better now.
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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
LGTM, thanks. https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:357: std::unique_ptr<ukm::UkmEntryBuilder> builder = On 2017/05/31 20:37:15, Navid Zolghadr wrote: > On 2017/05/30 16:52:07, tdresser wrote: > > Does this need to be a unique_ptr, or can it be stack allocated? > > You mean the normal object with no pointer? But the GetEntryBuilder returns a > pointer. The usage instructions are here: > https://cs.chromium.org/chromium/src/components/ukm/public/mojo_ukm_recorder.... Acknowledged. https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc (right): https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc:151: for (size_t i = 0; i < ukm_recoder->entries_count(); ++i) { On 2017/05/31 20:37:15, Navid Zolghadr wrote: > On 2017/05/30 16:52:07, tdresser wrote: > > Could this be a range based for? > > It seems that ukm_recorder is not (and does not return) an iterable object. Acknowledged. https://codereview.chromium.org/2888153002/diff/40001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc (right): https://codereview.chromium.org/2888153002/diff/40001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc:145: ::testing::AssertionResult UkmReportAssert(const char* event_name, Maybe AssertUkmReported?
Description was changed from ========== Add TimeToScrollUpdateSwapBegin2 to UKM Report TimeToScrollUpdateSwapBegin2 via UKM and also add the testing mechanisms in render_widget_host_latency_tracker_unittest BUG=723677 ========== to ========== Add TimeToScrollUpdateSwapBegin2 to UKM Report TimeToScrollUpdateSwapBegin2 via UKM and also add the testing mechanisms in render_widget_host_latency_tracker_unittest BUG=723677, 717692 ==========
On 2017/06/01 12:44:45, tdresser wrote: > LGTM, thanks. > > https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... > File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc > (right): > > https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:357: > std::unique_ptr<ukm::UkmEntryBuilder> builder = > On 2017/05/31 20:37:15, Navid Zolghadr wrote: > > On 2017/05/30 16:52:07, tdresser wrote: > > > Does this need to be a unique_ptr, or can it be stack allocated? > > > > You mean the normal object with no pointer? But the GetEntryBuilder returns a > > pointer. The usage instructions are here: > > > https://cs.chromium.org/chromium/src/components/ukm/public/mojo_ukm_recorder.... > > Acknowledged. > > https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... > File > content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc > (right): > > https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc:151: > for (size_t i = 0; i < ukm_recoder->entries_count(); ++i) { > On 2017/05/31 20:37:15, Navid Zolghadr wrote: > > On 2017/05/30 16:52:07, tdresser wrote: > > > Could this be a range based for? > > > > It seems that ukm_recorder is not (and does not return) an iterable object. > > Acknowledged. > > https://codereview.chromium.org/2888153002/diff/40001/content/browser/rendere... > File > content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc > (right): > > https://codereview.chromium.org/2888153002/diff/40001/content/browser/rendere... > content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc:145: > ::testing::AssertionResult UkmReportAssert(const char* event_name, > Maybe AssertUkmReported? you might have missed this in my previous message, but You also need a Launch bug to associate any new UKMs with. If you don't have one for this already you can use a generic UKM m60 for example: https://bugs.chromium.org/p/chromium/issues/detail?id=717692&q=ukm%20m60&cols...
On 2017/06/01 15:14:50, rkaplow wrote: > On 2017/06/01 12:44:45, tdresser wrote: > > LGTM, thanks. > > > > > https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... > > File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc > > (right): > > > > > https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... > > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:357: > > std::unique_ptr<ukm::UkmEntryBuilder> builder = > > On 2017/05/31 20:37:15, Navid Zolghadr wrote: > > > On 2017/05/30 16:52:07, tdresser wrote: > > > > Does this need to be a unique_ptr, or can it be stack allocated? > > > > > > You mean the normal object with no pointer? But the GetEntryBuilder returns > a > > > pointer. The usage instructions are here: > > > > > > https://cs.chromium.org/chromium/src/components/ukm/public/mojo_ukm_recorder.... > > > > Acknowledged. > > > > > https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... > > File > > > content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc > > (right): > > > > > https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... > > > content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc:151: > > for (size_t i = 0; i < ukm_recoder->entries_count(); ++i) { > > On 2017/05/31 20:37:15, Navid Zolghadr wrote: > > > On 2017/05/30 16:52:07, tdresser wrote: > > > > Could this be a range based for? > > > > > > It seems that ukm_recorder is not (and does not return) an iterable object. > > > > Acknowledged. > > > > > https://codereview.chromium.org/2888153002/diff/40001/content/browser/rendere... > > File > > > content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc > > (right): > > > > > https://codereview.chromium.org/2888153002/diff/40001/content/browser/rendere... > > > content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc:145: > > ::testing::AssertionResult UkmReportAssert(const char* event_name, > > Maybe AssertUkmReported? > > you might have missed this in my previous message, but > You also need a Launch bug to associate any new UKMs with. If you don't have one > for this already you can use a generic UKM m60 for example: > https://bugs.chromium.org/p/chromium/issues/detail?id=717692&q=ukm%20m60&cols... I already added it into the bug description. Not sure what else is needed.
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...
Description was changed from ========== Add TimeToScrollUpdateSwapBegin2 to UKM Report TimeToScrollUpdateSwapBegin2 via UKM and also add the testing mechanisms in render_widget_host_latency_tracker_unittest BUG=723677, 717692 ========== to ========== Add TimeToScrollUpdateSwapBegin2 to UKM Report TimeToScrollUpdateSwapBegin2 via UKM and also add the testing mechanisms in render_widget_host_latency_tracker_unittest BUG=723677, 728707 ==========
On 2017/06/01 15:22:07, Navid Zolghadr wrote: > On 2017/06/01 15:14:50, rkaplow wrote: > > On 2017/06/01 12:44:45, tdresser wrote: > > > LGTM, thanks. > > > > > > > > > https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... > > > File > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... > > > > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:357: > > > std::unique_ptr<ukm::UkmEntryBuilder> builder = > > > On 2017/05/31 20:37:15, Navid Zolghadr wrote: > > > > On 2017/05/30 16:52:07, tdresser wrote: > > > > > Does this need to be a unique_ptr, or can it be stack allocated? > > > > > > > > You mean the normal object with no pointer? But the GetEntryBuilder > returns > > a > > > > pointer. The usage instructions are here: > > > > > > > > > > https://cs.chromium.org/chromium/src/components/ukm/public/mojo_ukm_recorder.... > > > > > > Acknowledged. > > > > > > > > > https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... > > > File > > > > > > content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/2888153002/diff/20001/content/browser/rendere... > > > > > > content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc:151: > > > for (size_t i = 0; i < ukm_recoder->entries_count(); ++i) { > > > On 2017/05/31 20:37:15, Navid Zolghadr wrote: > > > > On 2017/05/30 16:52:07, tdresser wrote: > > > > > Could this be a range based for? > > > > > > > > It seems that ukm_recorder is not (and does not return) an iterable > object. > > > > > > Acknowledged. > > > > > > > > > https://codereview.chromium.org/2888153002/diff/40001/content/browser/rendere... > > > File > > > > > > content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/2888153002/diff/40001/content/browser/rendere... > > > > > > content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc:145: > > > ::testing::AssertionResult UkmReportAssert(const char* event_name, > > > Maybe AssertUkmReported? > > > > you might have missed this in my previous message, but > > You also need a Launch bug to associate any new UKMs with. If you don't have > one > > for this already you can use a generic UKM m60 for example: > > > https://bugs.chromium.org/p/chromium/issues/detail?id=717692&q=ukm%20m60&cols... > > I already added it into the bug description. Not sure what else is needed. I used the one for M61.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nzolghadr@chromium.org changed reviewers: + creis@chromium.org
creis@chromium.org: Please review changes in content/public/*
LGTM
rkaplow@google.com changed reviewers: + rkaplow@google.com
lgtm
The CQ bit was checked by nzolghadr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2888153002/#ps60001 (title: "Rename the test function")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/06/02 14:39:17, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Robert, can you l-g-t-m with your chromium account?
lgtm
The CQ bit was checked by nzolghadr@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": 60001, "attempt_start_ts": 1496417575881100,
"parent_rev": "3dbde7154ecc2d2e87760e67547d7a76ac9e3739", "commit_rev":
"04ce77fc2727d5100b89719d7bd0256797b6dfa4"}
Message was sent while issue was closed.
Description was changed from ========== Add TimeToScrollUpdateSwapBegin2 to UKM Report TimeToScrollUpdateSwapBegin2 via UKM and also add the testing mechanisms in render_widget_host_latency_tracker_unittest BUG=723677, 728707 ========== to ========== Add TimeToScrollUpdateSwapBegin2 to UKM Report TimeToScrollUpdateSwapBegin2 via UKM and also add the testing mechanisms in render_widget_host_latency_tracker_unittest BUG=723677, 728707 Review-Url: https://codereview.chromium.org/2888153002 Cr-Commit-Position: refs/heads/master@{#476670} Committed: https://chromium.googlesource.com/chromium/src/+/04ce77fc2727d5100b89719d7bd0... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/04ce77fc2727d5100b89719d7bd0... |
