Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
390143002:
Cleanup Timeline Interaction Record (Closed)
DescriptionMinor cleanups for the Timeline Interaction Record:
* Update to use a single regex with named groups
* Add flags attr to the TIR __init__ function
* Remove references to 'logical name' (use 'label' instead)
* Use @property for attributes
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283462
Patch Set 1 : #
Total comments: 9
Patch Set 2 : address comments #
Total comments: 2
Patch Set 3 : #Patch Set 4 : add is_fast #Patch Set 5 : #Messages
Total messages: 17 (0 generated)
https://codereview.chromium.org/390143002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_interaction_record.py (right): https://codereview.chromium.org/390143002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_interaction_record.py:103: * repeatable: Allows other interactions to use the same logical name Also, can you change 'logical name' to label? https://codereview.chromium.org/390143002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_interaction_record.py:110: self.end = end It would be nice cleanup if you can make these attribute immutable with @property. https://codereview.chromium.org/390143002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_interaction_record.py:116: setattr(self, flag, flag in flags) I really don't like the setattr magic and would want flags to be static attribute. +Nat for opinion.
https://codereview.chromium.org/390143002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_interaction_record.py (right): https://codereview.chromium.org/390143002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_interaction_record.py:116: setattr(self, flag, flag in flags) On 2014/07/14 20:19:15, nednguyen wrote: > I really don't like the setattr magic and would want flags to be static > attribute. > +Nat for opinion. Agreed. Maybe turn them into properties? self._flags = flags if flags is not None else [] @property def is_smooth(self): return IS_SMOOTH in self._flags etc. https://codereview.chromium.org/390143002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_interaction_record.py:234: flags.append(flag) Then here you can just do ','.join(self._flags)
https://codereview.chromium.org/390143002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_interaction_record.py (right): https://codereview.chromium.org/390143002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_interaction_record.py:103: * repeatable: Allows other interactions to use the same logical name On 2014/07/14 20:19:15, nednguyen wrote: > Also, can you change 'logical name' to label? Done. https://codereview.chromium.org/390143002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_interaction_record.py:110: self.end = end On 2014/07/14 20:19:15, nednguyen wrote: > It would be nice cleanup if you can make these attribute immutable with > @property. Done. https://codereview.chromium.org/390143002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_interaction_record.py:116: setattr(self, flag, flag in flags) On 2014/07/14 21:06:39, chrishenry wrote: > On 2014/07/14 20:19:15, nednguyen wrote: > > I really don't like the setattr magic and would want flags to be static > > attribute. > > +Nat for opinion. > > Agreed. Maybe turn them into properties? > > self._flags = flags if flags is not None else [] > > @property > def is_smooth(self): > return IS_SMOOTH in self._flags > > etc. Good suggestions. Done. https://codereview.chromium.org/390143002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_interaction_record.py:234: flags.append(flag) On 2014/07/14 21:06:39, chrishenry wrote: > Then here you can just do ','.join(self._flags) Done.
lgtm https://codereview.chromium.org/390143002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_interaction_record.py (right): https://codereview.chromium.org/390143002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_interaction_record.py:171: return getattr(self, metric_type) return metric_type in self._flags? The less of setattr, getattr magic we use, the better.
https://codereview.chromium.org/390143002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_interaction_record.py (right): https://codereview.chromium.org/390143002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_interaction_record.py:171: return getattr(self, metric_type) On 2014/07/14 22:09:59, nednguyen wrote: > return metric_type in self._flags? > > The less of setattr, getattr magic we use, the better. Done.
lgtm
The CQ bit was checked by ariblue@google.com
The CQ bit was unchecked by ariblue@google.com
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/390143002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
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/390143002/120001
Message was sent while issue was closed.
Change committed as 283462 |
