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.
3. Listener onRecordingStopped is not finished yet.

BUG=620066

[caseq, 2016-06-21 07:29:50]

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

[caseq, 2016-06-21 07:29:51]

Great, looks like you're on the right track, but here's a bunch of mostly
stylistic comments.

https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js
(right):

https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js:54:
RecordingStarted: "recording-started-",
nit: "trace-recording-started-"

https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js:585:
function dispatchRecordingEvent(request)
Looks like you don't need the custom dispatch callback, just drop it and the
EventSink will do all the magic on its own.

https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js:589:
this._id = id;
do you need this?

https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js
(right):

https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:180:
startTraceProviderRun: function(traceProviderId)
nit: startTraceRecording

https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:603:
this.dispatchEventToListeners(WebInspector.ExtensionServer.Events.TraceProviderAdded,
message);
let's not leak extension messages outside of ExtensionServer, pass provider
instead of message.

https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:614: 
nit: nuke extra empty lines

https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js
(right):

https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:71:
this._traceProvidersById = {};
We prefer using maps in the new code, i.e.

/**
  * @type {!Map<string, !WebInspector.TraceProvider>}
  *
this._traceProvidersById = new Map();

https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:72:
this._captureExtensionSettings = {};
ditto.

https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:75: var
provider = extensionTraceProviders[i];
nit: for (var provider of extensionTraceProviders) { ...

https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:177: get
traceProvidersById()
do you need to expose this? Also, we avoid getters in the new code, just use
functions.

https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:191:
getTraceProvider: function(id)
not sure you need to expose any of these (also: please drop "get" prefixes on
getters and prefix with underscores for "private" methods)

https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:425: for
(var s in this._captureExtensionSettings) {
I guess you also want to handle TraceProviderAdded to handle the trace providers
added after timeline panel is instantiated.

[fgao, 2016-06-23 20:55:13]

Description was changed from

==========
checkbox prototype


Timeline addTraceProvider API Initialization

1. Add constructors for Timeline.
2. Add skeleton method addTraceProvider.
3. TraceProvider registration will be implemented in future CLs.

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

to

==========
Timeline addTraceProvider API 

1. Add constructors for Timeline.
2. Add method addTraceProvider.
3. Add Listener onRecordingStarted and fire when Timeline recording starts.
3. Listener onRecordingStopped is not finished yet.

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

[fgao, 2016-06-23 21:08:44]

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.
3. Listener onRecordingStopped is not finished yet.

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

to

==========
Timeline addTraceProvider API 

1. Add constructors for Timeline.
2. Add method addTraceProvider.
3. Add Listener onRecordingStarted and fire when Timeline recording starts.
3. Listener onRecordingStopped is not finished yet.

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

[fgao, 2016-06-23 21:24:30]

Hi Andrey,

I have refactored some code and removed the redundant. The extension injected
events are now in extensionTraceProvider's tracing model. Next is to render the
graph in TimelineFlameChart.


On 2016/06/21 at 07:29:51, caseq wrote:
> Great, looks like you're on the right track, but here's a bunch of mostly
stylistic comments.
> 
>
https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js
(right):
> 
>
https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js:54:
RecordingStarted: "recording-started-",
> nit: "trace-recording-started-"
> 
>
https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js:585:
function dispatchRecordingEvent(request)
> Looks like you don't need the custom dispatch callback, just drop it and the
EventSink will do all the magic on its own.
> 
>
https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/extensions/ExtensionAPI.js:589:
this._id = id;
> do you need this?
> 
>
https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js
(right):
> 
>
https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:180:
startTraceProviderRun: function(traceProviderId)
> nit: startTraceRecording
> 
>
https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:603:
this.dispatchEventToListeners(WebInspector.ExtensionServer.Events.TraceProviderAdded,
message);
> let's not leak extension messages outside of ExtensionServer, pass provider
instead of message.
> 
>
https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:614: 
> nit: nuke extra empty lines
> 
>
https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js
(right):
> 
>
https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:71:
this._traceProvidersById = {};
> We prefer using maps in the new code, i.e.
> 
> /**
>   * @type {!Map<string, !WebInspector.TraceProvider>}
>   *
> this._traceProvidersById = new Map();
> 
>
https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:72:
this._captureExtensionSettings = {};
> ditto.
> 
>
https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:75: var
provider = extensionTraceProviders[i];
> nit: for (var provider of extensionTraceProviders) { ...
> 
>
https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:177:
get traceProvidersById()
> do you need to expose this? Also, we avoid getters in the new code, just use
functions.
> 
>
https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:191:
getTraceProvider: function(id)
> not sure you need to expose any of these (also: please drop "get" prefixes on
getters and prefix with underscores for "private" methods)
> 
>
https://codereview.chromium.org/2073343002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:425:
for (var s in this._captureExtensionSettings) {
> I guess you also want to handle TraceProviderAdded to handle the trace
providers added after timeline panel is instantiated.

[caseq, 2016-07-11 18:58:38]

https://codereview.chromium.org/2073343002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/TracingModel.js (right):

https://codereview.chromium.org/2073343002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/TracingModel.js:225: if
(payload.name == WebInspector.TracingModel.ExtensionEvent) {
Let's avoid this. I think the right way would be either to write the trace
meta-data in the extension that would produce the desired effect or handled the
process/thread titles in the TimelineFlameChart.

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

https://codereview.chromium.org/2073343002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js:236:
var p = new Promise(function(resolve, reject) {
Let's piggy-back on the existent allProfilesStoppped prommise:
- rename allProfilesStoppedPromise to allTatrgetsStopped or something other
general enough;
- add one promise per trace provider;
- each promise would resolve either upon associated trace provider returning
data or upon a timeout;

https://codereview.chromium.org/2073343002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChart.js
(right):

https://codereview.chromium.org/2073343002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineFlameChart.js:829:
//console.log(event);
drop this.

https://codereview.chromium.org/2073343002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js
(right):

https://codereview.chromium.org/2073343002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:397:
this._panelToolbar.appendToolbarItem(this._createSettingCheckbox(
let's also have some handling for trace providers that are registered after the
Timeline panel is shown.

https://codereview.chromium.org/2073343002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineModel.js
(right):

https://codereview.chromium.org/2073343002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineModel.js:165:
Extension: "Extension"
This shouldn't be necessary.

https://codereview.chromium.org/2073343002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineModel.js:185:
WebInspector.TimelineModel.ExtensionThreadName = "extension";
ditto.