Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
476363002:
NotEnoughFramesError no longer raises an exception. (Closed)
DescriptionNotEnoughFramesError no longer raises an exception.
Instead, it causes smoothness metrics to have None values. Additionaly,
the smoothness metric in smoothness.py has been refactored to more
easily support (the newly added) unit tests.
BUG=
Committed: https://crrev.com/fb47659a3c5c43f42600c6bde8c1e371dac38ffb
Cr-Commit-Position: refs/heads/master@{#292201}
Patch Set 1 #Patch Set 2 : #
Total comments: 17
Patch Set 3 : address comments #
Total comments: 17
Patch Set 4 : address more comments #
Total comments: 14
Patch Set 5 : address further comments #
Total comments: 1
Messages
Total messages: 15 (0 generated)
Split from https://codereview.chromium.org/475503004/
https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/metrics/smoothness.py (right): https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:31: +1. Can you make sure that these description are included in corresponding values. https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:47: def PopulateResultsFromStats(self, results, stats): Makes this private also. https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:50: self.ComputeLatencyMetric(page, stats, 'input_event_latency'), Maybe GetLatencyValuesFromStats. Also these methods doesn't need to be public. https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:58: for v in FlattenList(values): FlattenList support flattening deep structure. We can be more explicit in this case by using operator + to merge these values lists. https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:61: @decorators.Cache This has bit me once and made a test flaky, and it raises some question of how this works when input type is mutable. If this code path is not performance critical, I would remove this. https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:63: """Returns whether we have collected at least two frames in every range. In every timestamp list. https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:66: have caused this in the past include: Can you move this message to NOT_ENOUGH_FRAMES_MESSAGE? It would be most helpful to people who run telemetry test to see this in the output. https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:76: """Adds the mean and discrepancy for given latency stats to results.""" This docstring is incorrect. https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:81: latency_list = FlattenList(getattr(stats, name)) Please avoid this getattr pattern if possible. Maybe use latency_lists as argument.
https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/metrics/smoothness.py (right): https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:50: self.ComputeLatencyMetric(page, stats, 'input_event_latency'), On 2014/08/18 13:32:42, nednguyen wrote: > Maybe GetLatencyValuesFromStats. Also these methods doesn't need to be public. Is there any way to call these from unittests directly if they are private? I believe this will trigger the "Access to a protected member _Foo of a client class" pylint error. https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:58: for v in FlattenList(values): On 2014/08/18 13:32:42, nednguyen wrote: > FlattenList support flattening deep structure. We can be more explicit in this > case by using operator + to merge these values lists. Done. https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:61: @decorators.Cache On 2014/08/18 13:32:42, nednguyen wrote: > This has bit me once and made a test flaky, and it raises some question of how > this works when input type is mutable. If this code path is not performance > critical, I would remove this. Sounds good. Removed. https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:63: """Returns whether we have collected at least two frames in every range. On 2014/08/18 13:32:42, nednguyen wrote: > In every timestamp list. Done. https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:66: have caused this in the past include: On 2014/08/18 13:32:42, nednguyen wrote: > Can you move this message to NOT_ENOUGH_FRAMES_MESSAGE? It would be most helpful > to people who run telemetry test to see this in the output. Done. https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:76: """Adds the mean and discrepancy for given latency stats to results.""" On 2014/08/18 13:32:42, nednguyen wrote: > This docstring is incorrect. Ah, good catch. Updating all the ComputeX docstrings below. https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:81: latency_list = FlattenList(getattr(stats, name)) On 2014/08/18 13:32:42, nednguyen wrote: > Please avoid this getattr pattern if possible. Maybe use latency_lists as > argument. Done.
On 2014/08/18 21:59:53, ariblue wrote: > https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/web_perf/metrics/smoothness.py (right): > > https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/metrics/smoothness.py:50: > self.ComputeLatencyMetric(page, stats, 'input_event_latency'), > On 2014/08/18 13:32:42, nednguyen wrote: > > Maybe GetLatencyValuesFromStats. Also these methods doesn't need to be public. > > Is there any way to call these from unittests directly if they are private? I > believe this will trigger the "Access to a protected member > _Foo of a client class" pylint error. You can suppress pylint with # pylint: disable=W0212 https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/metrics/smoothness.py:58: for v in > FlattenList(values): > On 2014/08/18 13:32:42, nednguyen wrote: > > FlattenList support flattening deep structure. We can be more explicit in this > > case by using operator + to merge these values lists. > > Done. > > https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/metrics/smoothness.py:61: @decorators.Cache > On 2014/08/18 13:32:42, nednguyen wrote: > > This has bit me once and made a test flaky, and it raises some question of how > > this works when input type is mutable. If this code path is not performance > > critical, I would remove this. > > Sounds good. Removed. > > https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/metrics/smoothness.py:63: """Returns whether > we have collected at least two frames in every range. > On 2014/08/18 13:32:42, nednguyen wrote: > > In every timestamp list. > > Done. > > https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/metrics/smoothness.py:66: have caused this in > the past include: > On 2014/08/18 13:32:42, nednguyen wrote: > > Can you move this message to NOT_ENOUGH_FRAMES_MESSAGE? It would be most > helpful > > to people who run telemetry test to see this in the output. > > Done. > > https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/metrics/smoothness.py:76: """Adds the mean > and discrepancy for given latency stats to results.""" > On 2014/08/18 13:32:42, nednguyen wrote: > > This docstring is incorrect. > > Ah, good catch. Updating all the ComputeX docstrings below. > > https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/metrics/smoothness.py:81: latency_list = > FlattenList(getattr(stats, name)) > On 2014/08/18 13:32:42, nednguyen wrote: > > Please avoid this getattr pattern if possible. Maybe use latency_lists as > > argument. > > Done.
https://codereview.chromium.org/476363002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/metrics/rendering_stats_unittest.py (left): https://codereview.chromium.org/476363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/rendering_stats_unittest.py:262: def testRangeWithoutFrames(self): Can you keep this test and assert that the output is as you expected (and that no exception is thrown). https://codereview.chromium.org/476363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/rendering_stats_unittest.py:262: def testRangeWithoutFrames(self): Do we also have a test for where queueing delay isn't supported? https://codereview.chromium.org/476363002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/metrics/smoothness.py (right): https://codereview.chromium.org/476363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:27: 60 frames per second and uniformly distrubted over the sequence. To determine frames per second (fps) https://codereview.chromium.org/476363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:27: 60 frames per second and uniformly distrubted over the sequence. To determine distributed https://codereview.chromium.org/476363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:31: frame_times - A list of raw frame times Let's use : instead of - https://codereview.chromium.org/476363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:39: than 2 frames, we will return None values for each of the smoothness metrics. Can you also document the same thing for queueing_durations https://codereview.chromium.org/476363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:88: return [ You can return a tuple instead. https://codereview.chromium.org/476363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:106: none_value_reason = 'Gesture scroll update latency is unsupported.' This is a little funny. How does smoothness.py knows that gesture scroll update latency is unsupported? It seems to break layering a bit. Are you sure it's not better for us to add failure reasons in rendering_stats instead? https://codereview.chromium.org/476363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:126: none_value_reason=none_value_reason) Can we add descriptions for these too (and a few more results above that are missing descriptions).
https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/metrics/smoothness.py (right): https://codereview.chromium.org/476363002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:47: def PopulateResultsFromStats(self, results, stats): On 2014/08/18 13:32:42, nednguyen wrote: > Makes this private also. Done. These and the ComputeX functions are now private. https://codereview.chromium.org/476363002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/metrics/rendering_stats_unittest.py (left): https://codereview.chromium.org/476363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/rendering_stats_unittest.py:262: def testRangeWithoutFrames(self): On 2014/08/19 17:57:43, chrishenry wrote: > Can you keep this test and assert that the output is as you expected (and that > no exception is thrown). Done. https://codereview.chromium.org/476363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/rendering_stats_unittest.py:262: def testRangeWithoutFrames(self): On 2014/08/19 17:57:43, chrishenry wrote: > Do we also have a test for where queueing delay isn't supported? Technically all of these don't have queueing delay supported (since it isn't added in addImplThreadRenderingStats). In smoothness unittests we check both the case of when there are and aren't queueing delay values. https://codereview.chromium.org/476363002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/metrics/smoothness.py (right): https://codereview.chromium.org/476363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:27: 60 frames per second and uniformly distrubted over the sequence. To determine > frames per second (fps) Done. > distributed Done. https://codereview.chromium.org/476363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:31: frame_times - A list of raw frame times On 2014/08/19 17:57:44, chrishenry wrote: > Let's use : instead of - Done. https://codereview.chromium.org/476363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:39: than 2 frames, we will return None values for each of the smoothness metrics. On 2014/08/19 17:57:44, chrishenry wrote: > Can you also document the same thing for queueing_durations Done. https://codereview.chromium.org/476363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:88: return [ On 2014/08/19 17:57:44, chrishenry wrote: > You can return a tuple instead. Done. https://codereview.chromium.org/476363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:106: none_value_reason = 'Gesture scroll update latency is unsupported.' On 2014/08/19 17:57:44, chrishenry wrote: > This is a little funny. How does smoothness.py knows that gesture scroll update > latency is unsupported? It seems to break layering a bit. Are you sure it's not > better for us to add failure reasons in rendering_stats instead? I was thinking of it as rendering_stats just provides data and a lack of data implies that the functionality is unsupported. Maybe the reason here should be more reflective of this actual situation without any assumptions. I've changed the error to 'No gesture scroll update latency values were recorded'. Similarly for the latency values above and the queueing delay below. https://codereview.chromium.org/476363002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:126: none_value_reason=none_value_reason) On 2014/08/19 17:57:44, chrishenry wrote: > Can we add descriptions for these too (and a few more results above that are > missing descriptions). Done.
lgtm Please wait for Chris' approval also. https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py (right): https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py:254: pass Instead of swallowing exception and have "No frame queueing durations recorded" as the none value reason, can we get more descriptive reason? Maybe we can have a field errors in rendering_stats which is a list of raised exceptions. Then smoothness can pull out the exception's message to report none value reason in this case. https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/metrics/smoothness.py (right): https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:71: def HasEnoughFrames(self, list_of_frame_timestamp_lists): This should also be private. https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:101: def _ComputeGestureScrollUpdateLatency(self, page, stats): _ComputeFirstGestureScrollUpdateLatency https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py (right): https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py:12: class MockRenderingStats(object): Can you make this private? https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py:39: setattr(stats, stat, [[10, 20], [30, 40, 50]]) This loop isn't sane to me because 1) it will overwrite frame_timestamps value you set above and 2) it assign frame times to stats like painted_pixel_counts and approximated_pixel_percentages. https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py:64: self.assertIsNone(mean_value.none_value_reason) maybe there is no need to check this since I remember you have the check in ValidateNoneValueReason already. https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py:98: stats) strange indentation
https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py (right): https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py:254: pass On 2014/08/22 15:46:37, nednguyen wrote: > Instead of swallowing exception and have "No frame queueing durations recorded" > as the none value reason, can we get more descriptive reason? > > Maybe we can have a field errors in rendering_stats which is a list of raised > exceptions. Then smoothness can pull out the exception's message to report none > value reason in this case. I've added a self.errors lookup. Seem reasonable? https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/metrics/smoothness.py (right): https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:71: def HasEnoughFrames(self, list_of_frame_timestamp_lists): On 2014/08/22 15:46:37, nednguyen wrote: > This should also be private. Done. https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness.py:101: def _ComputeGestureScrollUpdateLatency(self, page, stats): On 2014/08/22 15:46:37, nednguyen wrote: > _ComputeFirstGestureScrollUpdateLatency Done. https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py (right): https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py:12: class MockRenderingStats(object): On 2014/08/22 15:46:37, nednguyen wrote: > Can you make this private? Done. https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py:39: setattr(stats, stat, [[10, 20], [30, 40, 50]]) On 2014/08/22 15:46:38, nednguyen wrote: > This loop isn't sane to me because 1) it will overwrite frame_timestamps value > you set above and 2) it assign frame times to stats like painted_pixel_counts > and approximated_pixel_percentages. That's fine. Regarding (2), this test doesn't really care what the numbers actually are. I guess I could add lots of different arrays of more appropriate fake data, but I figured that is unnecessary. I've added a comment accordingly. For (1), I've removed the frame_timestamps=self.good_timestamps to remove some of the confusion. https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py:64: self.assertIsNone(mean_value.none_value_reason) On 2014/08/22 15:46:37, nednguyen wrote: > maybe there is no need to check this since I remember you have the check in > ValidateNoneValueReason already. Done. https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py:98: stats) On 2014/08/22 15:46:38, nednguyen wrote: > strange indentation Done.
Patchset #5 (id:80001) has been deleted
On 2014/08/27 00:15:20, ariblue wrote: > https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py (right): > > https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py:254: pass > On 2014/08/22 15:46:37, nednguyen wrote: > > Instead of swallowing exception and have "No frame queueing durations > recorded" > > as the none value reason, can we get more descriptive reason? > > > > Maybe we can have a field errors in rendering_stats which is a list of raised > > exceptions. Then smoothness can pull out the exception's message to report > none > > value reason in this case. > > I've added a self.errors lookup. Seem reasonable? That's great :-) > > https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/web_perf/metrics/smoothness.py (right): > > https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/metrics/smoothness.py:71: def > HasEnoughFrames(self, list_of_frame_timestamp_lists): > On 2014/08/22 15:46:37, nednguyen wrote: > > This should also be private. > > Done. > > https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/metrics/smoothness.py:101: def > _ComputeGestureScrollUpdateLatency(self, page, stats): > On 2014/08/22 15:46:37, nednguyen wrote: > > _ComputeFirstGestureScrollUpdateLatency > > Done. > > https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py (right): > > https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py:12: class > MockRenderingStats(object): > On 2014/08/22 15:46:37, nednguyen wrote: > > Can you make this private? > > Done. > > https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py:39: > setattr(stats, stat, [[10, 20], [30, 40, 50]]) > On 2014/08/22 15:46:38, nednguyen wrote: > > This loop isn't sane to me because 1) it will overwrite frame_timestamps value > > you set above and 2) it assign frame times to stats like painted_pixel_counts > > and approximated_pixel_percentages. > > That's fine. Regarding (2), this test doesn't really care what the numbers > actually are. I guess I could add lots of different arrays of more appropriate > fake data, but I figured that is unnecessary. I've added a comment accordingly. > For (1), I've removed the frame_timestamps=self.good_timestamps to remove some > of the confusion. > > https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py:64: > self.assertIsNone(mean_value.none_value_reason) > On 2014/08/22 15:46:37, nednguyen wrote: > > maybe there is no need to check this since I remember you have the check in > > ValidateNoneValueReason already. > > Done. > > https://codereview.chromium.org/476363002/diff/60001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py:98: stats) > On 2014/08/22 15:46:38, nednguyen wrote: > > strange indentation > > Done.
lgtm https://codereview.chromium.org/476363002/diff/100001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py (right): https://codereview.chromium.org/476363002/diff/100001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py:131: self.errors = {} +1
The CQ bit was checked by ariblue@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ariblue@google.com/476363002/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as 53c5630ada9b5616d7123075de48bbb9fd65c847
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/fb47659a3c5c43f42600c6bde8c1e371dac38ffb Cr-Commit-Position: refs/heads/master@{#292201} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
