Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
399943002:
Telemetry: make InspectorTimeline backward compatible with upcoming changes in DevTools protocol. (Closed)
|
Created:
6 years, 5 months ago by loislo Modified:
6 years, 5 months ago CC:
chromium-reviews, telemetry+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionTelemetry: make InspectorTimeline backward compatible with upcoming changes in DevTools protocol.
I'm moving events parameter from the results of Timeline.stop command to Timeline.stopped event.
BUG=394780
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284035
Patch Set 1 #Patch Set 2 : a change in comment #
Total comments: 8
Patch Set 3 : comments addressed #Messages
Total messages: 11 (0 generated)
lgtm https://codereview.chromium.org/399943002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/inspector_timeline.py (right): https://codereview.chromium.org/399943002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/inspector_timeline.py:72: raw_events = result['events'] Can you add a TODO to clean this up once this is no longer necessary for the stable channel? https://codereview.chromium.org/399943002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/inspector_timeline.py:74: raw_events = self._raw_events Reset self._raw_events to None after this?
https://codereview.chromium.org/399943002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/inspector_timeline.py (right): https://codereview.chromium.org/399943002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/inspector_timeline.py:74: raw_events = self._raw_events Is it true that _OnNotification is always called before you hit this code? It isn't clear to me from reading the code. In fact, it looks like once we finished sending Timeline.stop request, we stop listening to notification from Timeline domain (UnregisterDomain('Timeline') call above).
On 2014/07/18 00:16:05, chrishenry wrote: > https://codereview.chromium.org/399943002/diff/20001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/core/backends/chrome/inspector_timeline.py > (right): > > https://codereview.chromium.org/399943002/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/core/backends/chrome/inspector_timeline.py:74: > raw_events = self._raw_events > Is it true that _OnNotification is always called before you hit this code? > > It isn't clear to me from reading the code. In fact, it looks like once we > finished sending Timeline.stop request, we stop listening to notification from > Timeline domain (UnregisterDomain('Timeline') call above). I believe inspector_backend.SyncRequest method is blocked until _OnNotification() is invoked.
https://codereview.chromium.org/399943002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/inspector_timeline.py (right): https://codereview.chromium.org/399943002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/inspector_timeline.py:103: self._raw_events = msg['params']['events'] Actually do we actually need to touch this? Can we modify _SendSyncRequest to return raw_events instead? If so, I prefer we use a same mechanism for getting these data.
On 2014/07/18 02:12:04, nednguyen wrote: > On 2014/07/18 00:16:05, chrishenry wrote: > > > https://codereview.chromium.org/399943002/diff/20001/tools/telemetry/telemetr... > > File tools/telemetry/telemetry/core/backends/chrome/inspector_timeline.py > > (right): > > > > > https://codereview.chromium.org/399943002/diff/20001/tools/telemetry/telemetr... > > tools/telemetry/telemetry/core/backends/chrome/inspector_timeline.py:74: > > raw_events = self._raw_events > > Is it true that _OnNotification is always called before you hit this code? > > > > It isn't clear to me from reading the code. In fact, it looks like once we > > finished sending Timeline.stop request, we stop listening to notification from > > Timeline domain (UnregisterDomain('Timeline') call above). > > I believe inspector_backend.SyncRequest method is blocked until > _OnNotification() is invoked. Well, but this isn't the response to the SyncRequest, but an additional event fired by DevTools: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... I don't think SendSyncRequest will wait for additional notification.
https://codereview.chromium.org/399943002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/inspector_timeline.py (right): https://codereview.chromium.org/399943002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/inspector_timeline.py:72: raw_events = result['events'] On 2014/07/17 21:35:41, nednguyen wrote: > Can you add a TODO to clean this up once this is no longer necessary for the > stable channel? Done. https://codereview.chromium.org/399943002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/inspector_timeline.py:74: raw_events = self._raw_events On 2014/07/18 00:16:05, chrishenry wrote: > Is it true that _OnNotification is always called before you hit this code? > > It isn't clear to me from reading the code. In fact, it looks like once we > finished sending Timeline.stop request, we stop listening to notification from > Timeline domain (UnregisterDomain('Timeline') call above). Timeline.stopped event arrives just before the response to Timeline.stop command. So after calling self._SendSyncRequest we definitely got the event and the response and it is safe to unregister the domain. BTW We have the event in the protocol because timeline might be started and finished by console.timeline console.timelineEnd commands from within the inspected page. https://codereview.chromium.org/399943002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/inspector_timeline.py:74: raw_events = self._raw_events On 2014/07/17 21:35:41, nednguyen wrote: > Reset self._raw_events to None after this? Done. https://codereview.chromium.org/399943002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/inspector_timeline.py:103: self._raw_events = msg['params']['events'] On 2014/07/18 02:17:35, nednguyen wrote: > Actually do we actually need to touch this? Can we modify _SendSyncRequest to > return raw_events instead? If so, I prefer we use a same mechanism for getting > these data. The method looks generic. It would be strange that it knows about the result content.
The CQ bit was checked by loislo@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/loislo@chromium.org/399943002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
Message was sent while issue was closed.
Change committed as 284035 |
