Timeline AddTraceProvider API

Timeline AddTraceProvider API

1. Add constructors for Timeline.
2. Add method addTraceProvider
3. Add listener onRecordingStarted and fire when Timeline recording starts.
4. Add listener onRecordingStopped and fire when Timeline recording
stops.

TraceProvider API

BUG=620066

[fgao, 2016-07-06 23:17:51]

Description was changed from

==========
Timeline AddTraceProvider API

1. Add constructors for Timeline.
2. Add method addTraceProvider
3. Add listener onRecordingStarted and fire when Timeline recording starts.
4. Add listener onRecordingStopped and fire when Timeline recording
stops.

TraceProvider API

BUG=
==========

to

==========
Timeline AddTraceProvider API

1. Add constructors for Timeline.
2. Add method addTraceProvider
3. Add listener onRecordingStarted and fire when Timeline recording starts.
4. Add listener onRecordingStopped and fire when Timeline recording
stops.

TraceProvider API

BUG=620066
==========

[fgao, 2016-07-06 23:17:51]

fgao@google.com changed reviewers:
+ caseq@chromium.org

[caseq, 2016-07-08 00:57:49]

https://codereview.chromium.org/2128133002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionTraceProvider.js
(right):

https://codereview.chromium.org/2128133002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionTraceProvider.js:17:
get id()
It doesn't look like we have to expose id externally.

https://codereview.chromium.org/2128133002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionTraceProvider.js:20:
},
add blank lines after all methods.

https://codereview.chromium.org/2128133002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionTraceProvider.js:21:
get categoryName()
please add @return annotations. Also, no getters in new code please, just make
it a function.

https://codereview.chromium.org/2128133002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionTraceProvider.js:29:
run: function()
s/run/start/

https://codereview.chromium.org/2128133002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionTraceProvider.js:34:
* @param {!Array.<!WebInspector.NetworkRequest>} requests
nit: Array<!WebInspector.NetworkRequest>

https://codereview.chromium.org/2128133002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js
(right):

https://codereview.chromium.org/2128133002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js:71:
for (var traceProvider of this._extensionTraceProviders) {
nit: drop redundant {}

https://codereview.chromium.org/2128133002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js:87:
var requests = target.networkLog.requests().slice();
Why are we doing this? This does not look good.

[fgao, 2016-07-08 03:33:34]

On 2016/07/08 at 00:57:49, caseq wrote:
>
https://codereview.chromium.org/2128133002/diff/1/third_party/WebKit/Source/d...
> File
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionTraceProvider.js
(right):
> 
>
https://codereview.chromium.org/2128133002/diff/1/third_party/WebKit/Source/d...
>
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionTraceProvider.js:17:
get id()
> It doesn't look like we have to expose id externally.

Removed get id()

>
https://codereview.chromium.org/2128133002/diff/1/third_party/WebKit/Source/d...
>
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionTraceProvider.js:20:
},
> add blank lines after all methods.

Done.
 
>
https://codereview.chromium.org/2128133002/diff/1/third_party/WebKit/Source/d...
>
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionTraceProvider.js:21:
get categoryName()
> please add @return annotations. Also, no getters in new code please, just make
it a function.

Done.

>
https://codereview.chromium.org/2128133002/diff/1/third_party/WebKit/Source/d...
>
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionTraceProvider.js:29:
run: function()
> s/run/start/

Sorry. I didn't get this comment. Do you mean change the function name to
"start"?

>
https://codereview.chromium.org/2128133002/diff/1/third_party/WebKit/Source/d...
>
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionTraceProvider.js:34:
* @param {!Array.<!WebInspector.NetworkRequest>} requests
> nit: Array<!WebInspector.NetworkRequest>

dot after Array is removed.

>
https://codereview.chromium.org/2128133002/diff/1/third_party/WebKit/Source/d...
> File
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js
(right):
> 
>
https://codereview.chromium.org/2128133002/diff/1/third_party/WebKit/Source/d...
>
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js:71:
for (var traceProvider of this._extensionTraceProviders) {
> nit: drop redundant {}

Done.

>
https://codereview.chromium.org/2128133002/diff/1/third_party/WebKit/Source/d...
>
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js:87:
var requests = target.networkLog.requests().slice();
> Why are we doing this? This does not look good.
This was used when I inject extension provided trace to tracing model. Since
that feature is not implemented as for now, I will remove this part.

[caseq, 2016-07-11 18:42:42]

So how does this relate to https://codereview.chromium.org/2073343002/? Looks
like you need to either merge these or make them non-overlap.

https://codereview.chromium.org/2128133002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js
(right):

https://codereview.chromium.org/2128133002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js:21:
this._extensionTraceProviders = WebInspector.extensionServer.traceProviders();
let's move this to startRecording() and make a copy of the array (i.e. just call
slice() at the end), so that we have a snapshot of trace provides at the
beginning of the trace recording.

https://codereview.chromium.org/2128133002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js:86:
//var requests = target.networkLog.requests().slice();
drop this.

[fgao, 2016-07-11 20:39:24]

On 2016/07/11 at 18:42:42, caseq wrote:
> So how does this relate to https://codereview.chromium.org/2073343002/? Looks
like you need to either merge these or make them non-overlap.
The feature in https://codereview.chromium.org/2073343002/ will not be
implemented for now. I will close that issue at this time and if we insert
extension provided traces into tracingModel, we can reopen that CL.

 
>
https://codereview.chromium.org/2128133002/diff/20001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js
(right):
> 
>
https://codereview.chromium.org/2128133002/diff/20001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js:21:
this._extensionTraceProviders = WebInspector.extensionServer.traceProviders();
> let's move this to startRecording() and make a copy of the array (i.e. just
call slice() at the end), so that we have a snapshot of trace provides at the
beginning of the trace recording.>

Done

>
https://codereview.chromium.org/2128133002/diff/20001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js:86:
//var requests = target.networkLog.requests().slice();
> drop this.
Done

[fgao, 2016-07-18 17:59:33]

Hi Andrey,

Layout tests are added. PTAL.

Thanks!

On 2016/07/11 at 20:39:24, fgao wrote:
> On 2016/07/11 at 18:42:42, caseq wrote:
> > So how does this relate to https://codereview.chromium.org/2073343002/?
Looks like you need to either merge these or make them non-overlap.
> The feature in https://codereview.chromium.org/2073343002/ will not be
implemented for now. I will close that issue at this time and if we insert
extension provided traces into tracingModel, we can reopen that CL.
> 
>  
> >
https://codereview.chromium.org/2128133002/diff/20001/third_party/WebKit/Sour...
> > File
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js
(right):
> > 
> >
https://codereview.chromium.org/2128133002/diff/20001/third_party/WebKit/Sour...
> >
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js:21:
this._extensionTraceProviders = WebInspector.extensionServer.traceProviders();
> > let's move this to startRecording() and make a copy of the array (i.e. just
call slice() at the end), so that we have a snapshot of trace provides at the
beginning of the trace recording.>
> 
> Done
> 
> >
https://codereview.chromium.org/2128133002/diff/20001/third_party/WebKit/Sour...
> >
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js:86:
//var requests = target.networkLog.requests().slice();
> > drop this.
> Done

[caseq, 2016-07-19 22:01:17]

https://codereview.chromium.org/2128133002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/extensions/extensions-timeline-api.html
(right):

https://codereview.chromium.org/2128133002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/extensions/extensions-timeline-api.html:19:
traceProvider.onRecordingStarted.addListener(onRecordingStarted);
how about onRecordingStopped?

https://codereview.chromium.org/2128133002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js
(right):

https://codereview.chromium.org/2128133002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:186:
stopTraceRecording: function(traceProviderId)
nit: please group together with startTraceRecording()

https://codereview.chromium.org/2128133002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:609:
this.dispatchEventToListeners(WebInspector.ExtensionServer.Events.TraceProviderAdded,
provider);
is this used?

https://codereview.chromium.org/2128133002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionTraceProvider.js
(right):

https://codereview.chromium.org/2128133002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionTraceProvider.js:28:
categoryTooltip: function()
this one and the one above look unused

https://codereview.chromium.org/2128133002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionTraceProvider.js:33:
run: function()
rename to start

https://codereview.chromium.org/2128133002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js
(right):

https://codereview.chromium.org/2128133002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js:73:
traceProvider.run();
rename from run() to start()?

https://codereview.chromium.org/2128133002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js:86:
var target = WebInspector.targetManager.mainTarget();
unused?

[fgao, 2016-07-20 01:14:22]

Hi Andrey, 

thanks for reviewing. I have removed all dead code from previous CL. Now tests
have covered both onStartedRecording and onStoppedRecording. Thanks for your
help!

On 2016/07/19 at 22:01:17, caseq wrote:
>
https://codereview.chromium.org/2128133002/diff/140001/third_party/WebKit/Lay...
> File
third_party/WebKit/LayoutTests/inspector/extensions/extensions-timeline-api.html
(right):
> 
>
https://codereview.chromium.org/2128133002/diff/140001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/inspector/extensions/extensions-timeline-api.html:19:
traceProvider.onRecordingStarted.addListener(onRecordingStarted);
> how about onRecordingStopped?

test added.

>
https://codereview.chromium.org/2128133002/diff/140001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js
(right):
> 
>
https://codereview.chromium.org/2128133002/diff/140001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:186:
stopTraceRecording: function(traceProviderId)
> nit: please group together with startTraceRecording()
Done.
 
>
https://codereview.chromium.org/2128133002/diff/140001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:609:
this.dispatchEventToListeners(WebInspector.ExtensionServer.Events.TraceProviderAdded,
provider);
> is this used?
Removed.

>
https://codereview.chromium.org/2128133002/diff/140001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionTraceProvider.js
(right):
> 
>
https://codereview.chromium.org/2128133002/diff/140001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionTraceProvider.js:28:
categoryTooltip: function()
> this one and the one above look unused
Removed.

>
https://codereview.chromium.org/2128133002/diff/140001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionTraceProvider.js:33:
run: function()
> rename to start
Done.

>
https://codereview.chromium.org/2128133002/diff/140001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js
(right):
> 
>
https://codereview.chromium.org/2128133002/diff/140001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js:73:
traceProvider.run();
> rename from run() to start()?
Done.

>
https://codereview.chromium.org/2128133002/diff/140001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js:86:
var target = WebInspector.targetManager.mainTarget();
> unused?
Removed.

[caseq, 2016-07-20 22:51:49]

lgtm

https://codereview.chromium.org/2128133002/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/extensions/extensions-api-expected.txt
(right):

https://codereview.chromium.org/2128133002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/extensions/extensions-api-expected.txt:64:
timeline : {
Please rebaseline this on tip-of-tree, now that
https://codereview.chromium.org/2164763002/ has landed, the methods you're
adding to timeline domain should appear in the test output.

https://codereview.chromium.org/2128133002/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/extensions/extensions-timeline-tests.js
(right):

https://codereview.chromium.org/2128133002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/extensions/extensions-timeline-tests.js:1:
function extension_startTimeline(callback)
nit: let's just put them into the test html file.

https://codereview.chromium.org/2128133002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js
(right):

https://codereview.chromium.org/2128133002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:166:
/**
revert this one.

https://codereview.chromium.org/2128133002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:611:
* @return {!Array.<!WebInspector.ExtensionTraceProvider>}
nit: drop ., just !Array<!WebInspector.ExtensionTraceProvide>

[alph, 2016-07-21 17:06:22]

alph@chromium.org changed reviewers:
+ alph@chromium.org

[alph, 2016-07-21 17:06:23]

https://codereview.chromium.org/2128133002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js
(right):

https://codereview.chromium.org/2128133002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:603:
_onAddTraceProvider: function(message, port)
nit: please annotate

[fgao, 2016-07-21 21:40:50]

On 2016/07/20 at 22:51:49, caseq wrote:
> lgtm
> 
>
https://codereview.chromium.org/2128133002/diff/220001/third_party/WebKit/Lay...
> File
third_party/WebKit/LayoutTests/inspector/extensions/extensions-api-expected.txt
(right):
> 
>
https://codereview.chromium.org/2128133002/diff/220001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/inspector/extensions/extensions-api-expected.txt:64:
timeline : {
> Please rebaseline this on tip-of-tree, now that
https://codereview.chromium.org/2164763002/ has landed, the methods you're
adding to timeline domain should appear in the test output.
Done.

>
https://codereview.chromium.org/2128133002/diff/220001/third_party/WebKit/Lay...
> File
third_party/WebKit/LayoutTests/inspector/extensions/extensions-timeline-tests.js
(right):
> 
>
https://codereview.chromium.org/2128133002/diff/220001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/inspector/extensions/extensions-timeline-tests.js:1:
function extension_startTimeline(callback)
> nit: let's just put them into the test html file.
Done.

>
https://codereview.chromium.org/2128133002/diff/220001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js
(right):
> 
>
https://codereview.chromium.org/2128133002/diff/220001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:166:
/**
> revert this one.
Done.
>
https://codereview.chromium.org/2128133002/diff/220001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:611:
* @return {!Array.<!WebInspector.ExtensionTraceProvider>}
> nit: drop ., just !Array<!WebInspector.ExtensionTraceProvide>
Done.

[fgao, 2016-07-21 21:41:32]

PTAL.

On 2016/07/21 at 17:06:23, alph wrote:
>
https://codereview.chromium.org/2128133002/diff/220001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js
(right):
> 
>
https://codereview.chromium.org/2128133002/diff/220001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:603:
_onAddTraceProvider: function(message, port)
> nit: please annotate

Done.

[caseq, 2016-07-27 00:01:32]

The CQ bit was checked by caseq@chromium.org

[caseq, 2016-07-27 00:01:33]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org
Link to the patchset: https://codereview.chromium.org/2128133002/#ps240001
(title: "Merge branch 'master' of https://chromium.googlesource.com/chromium/src
into record")

[commit-bot: I haz the power, 2016-07-27 00:02:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 01:21:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-27 01:21:10]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[caseq, 2016-07-27 01:21:36]

The CQ bit was checked by caseq@chromium.org

[commit-bot: I haz the power, 2016-07-27 01:22:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 02:35:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-27 02:35:24]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alph, 2016-08-09 01:25:30]

Description was changed from

==========
Timeline AddTraceProvider API

1. Add constructors for Timeline.
2. Add method addTraceProvider
3. Add listener onRecordingStarted and fire when Timeline recording starts.
4. Add listener onRecordingStopped and fire when Timeline recording
stops.

TraceProvider API

BUG=620066
==========

to

==========
Timeline AddTraceProvider API

1. Add constructors for Timeline.
2. Add method addTraceProvider
3. Add listener onRecordingStarted and fire when Timeline recording starts.
4. Add listener onRecordingStopped and fire when Timeline recording
stops.

TraceProvider API

BUG=620066
==========

[alph, 2016-08-09 01:25:30]

alph@chromium.org changed reviewers:
- alph@chromium.org