Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
965953002:
[Telemetry] Output all timeline based metrics on all timeline interaction records (Closed)
Description[Telemetry] Output all timeline based metrics on all timeline interaction records
BUG=444697
Committed: https://crrev.com/946ce9402129467d952b9a89349b7a346f041ef4
Cr-Commit-Position: refs/heads/master@{#318918}
Patch Set 1 : #
Total comments: 8
Patch Set 2 : Address Ethan's reviews #
Total comments: 2
Patch Set 3 : Address Sami's comment #
Messages
Total messages: 22 (6 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
nednguyen@google.com changed reviewers: + eakuefner@chromium.org, nduca@chromium.org, skyostil@google.com
https://codereview.chromium.org/965953002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/965953002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:35: # This cannot be done until crbug.com/460208 is fixed. Is it okay to return a tuple instead of a list here? I ask only because _GetMetricsFromFlags returns list rather than tuple.
https://codereview.chromium.org/965953002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/965953002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:35: # This cannot be done until crbug.com/460208 is fixed. On 2015/03/02 21:55:47, eakuefner wrote: > Is it okay to return a tuple instead of a list here? I ask only because > _GetMetricsFromFlags returns list rather than tuple. Yes, it is since callsite will not change this. I usually prefer immutability if it's possible.
lgtm https://codereview.chromium.org/965953002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/965953002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:35: # This cannot be done until crbug.com/460208 is fixed. On 2015/03/02 at 23:29:09, nednguyen wrote: > On 2015/03/02 21:55:47, eakuefner wrote: > > Is it okay to return a tuple instead of a list here? I ask only because > > _GetMetricsFromFlags returns list rather than tuple. > > Yes, it is since callsite will not change this. I usually prefer immutability if it's possible. sgtm, I agree immutability is preferable wherever possible. https://codereview.chromium.org/965953002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:230: model, renderer_thread, interaction_records) nit: styleguide likes 4 spaces instead of 2 here https://codereview.chromium.org/965953002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_based_measurement_unittest.py (right): https://codereview.chromium.org/965953002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement_unittest.py:106: self.actual_get_all_tmb_metrics = tbm_module._GetAllTimelineBasedMetrics self.actual_get_all_tbm_metrics
https://codereview.chromium.org/965953002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_based_measurement_unittest.py (right): https://codereview.chromium.org/965953002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement_unittest.py:106: self.actual_get_all_tmb_metrics = tbm_module._GetAllTimelineBasedMetrics On 2015/03/02 at 23:38:10, eakuefner wrote: > self.actual_get_all_tbm_metrics or maybe "tbms" since otherwise this is like atm machine, but really, either is fine with me.
On 2015/03/02 at 23:44:45, eakuefner wrote: > https://codereview.chromium.org/965953002/diff/40001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/web_perf/timeline_based_measurement_unittest.py (right): > > https://codereview.chromium.org/965953002/diff/40001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/timeline_based_measurement_unittest.py:106: self.actual_get_all_tmb_metrics = tbm_module._GetAllTimelineBasedMetrics > On 2015/03/02 at 23:38:10, eakuefner wrote: > > self.actual_get_all_tbm_metrics > > or maybe "tbms" since otherwise this is like atm machine, but really, either is fine with me.
On 2015/03/03 at 00:01:10, eakuefner wrote: > On 2015/03/02 at 23:44:45, eakuefner wrote: > > https://codereview.chromium.org/965953002/diff/40001/tools/telemetry/telemetr... > > File tools/telemetry/telemetry/web_perf/timeline_based_measurement_unittest.py (right): > > > > https://codereview.chromium.org/965953002/diff/40001/tools/telemetry/telemetr... > > tools/telemetry/telemetry/web_perf/timeline_based_measurement_unittest.py:106: self.actual_get_all_tmb_metrics = tbm_module._GetAllTimelineBasedMetrics > > On 2015/03/02 at 23:38:10, eakuefner wrote: > > > self.actual_get_all_tbm_metrics > > > > or maybe "tbms" since otherwise this is like atm machine, but really, either is fine with me. * Although a case could be made that "m" maybe stands for measurement rather than metric, so perhaps it isn't redundant after all :) ahh sorry for the review noise
https://codereview.chromium.org/965953002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/965953002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:230: model, renderer_thread, interaction_records) On 2015/03/02 23:38:10, eakuefner wrote: > nit: styleguide likes 4 spaces instead of 2 here Done. https://codereview.chromium.org/965953002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_based_measurement_unittest.py (right): https://codereview.chromium.org/965953002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement_unittest.py:106: self.actual_get_all_tmb_metrics = tbm_module._GetAllTimelineBasedMetrics On 2015/03/02 23:38:10, eakuefner wrote: > self.actual_get_all_tbm_metrics Done.
lgtm
skyostil@chromium.org changed reviewers: + skyostil@chromium.org
https://codereview.chromium.org/965953002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/965953002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:33: # TODO(nednguyen): uses discovery pattern to return all the instances of s/uses/use/?
https://codereview.chromium.org/965953002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/965953002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:33: # TODO(nednguyen): uses discovery pattern to return all the instances of On 2015/03/03 17:49:49, Sami wrote: > s/uses/use/? Done.
The CQ bit was checked by nednguyen@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org, nduca@chromium.org Link to the patchset: https://codereview.chromium.org/965953002/#ps80001 (title: "Address Sami's comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/965953002/80001
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/946ce9402129467d952b9a89349b7a346f041ef4 Cr-Commit-Position: refs/heads/master@{#318918} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
