1050384 - [timeline] build an actor to forward gecko operations

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Comment 1

[Paul Rouget [:paul]]

Attachment: v0.0001

Comment 2

[Paul Rouget [:paul]]

Attachment: v0.0002

Comment 3

[Paul Rouget [:paul]]

Attachment: v0.0003

Comment 4

[Paul Rouget [:paul]]

Attachment: v1

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8475867 [details] [diff] [review]
v1

Review of attachment 8475867 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

::: toolkit/devtools/server/actors/timeline.js
@@ +19,5 @@
> +  handle.removeGlobalActor(TimelineActor);
> +  handle.removeTabActor(TimelineActor);
> +};
> +
> +let TimelineActor = protocol.ActorClass({

Please add a jsdoc comment block before this line.
It helps so much when, opening a file for the first time, you see that the author has taken the time to explain what this file contains, what the classes in it do, and also gave usage examples.
I think a quick example usage showing that you first need to call start, then listen for events, and then call stop would be useful for future maintainers.

@@ +68,5 @@
> +    let markers = this.docshell.popProfileTimelineMarkers();
> +    if (markers.length > 0) {
> +      events.emit(this, "markers", markers);
> +    }
> +    this._timeout = setTimeout(() => this._pullTimelineData(), 300);

Add a 'const TIMELINE_DATA_PULL_TIMEOUT = 300;' at the top of the file.
Also rename 'this._timeout' into something more explicit like 'this._dataPullTimeout'

::: toolkit/devtools/server/tests/browser/browser_timeline.js
@@ +4,5 @@
> +"use strict";
> +
> +waitForExplicitFinish();
> +let test = asyncTest(function*() {
> +  let {Task} = require("resource://gre/modules/Task.jsm");

This isn't needed, head.js already uses Task, it must be imported by the test harness.
I think it's also worth checking if requiring Promise.jsm is needed in head.js too. It might already be available.

::: toolkit/devtools/server/tests/browser/head.js
@@ -17,3 @@
>   */
> -function addTab(aURL, aCallback) {
> -  waitForExplicitFinish();

In most (all?) of the other browser mochitests in the devtools, we call waitForExplicitFinish(); in head.js, near the top of the file.
I think we should do this here too. I haven't yet seen a browser test that isn't async.

@@ +69,5 @@
> +
> +/**
> + * Define an async test based on a generator function
> + */
> +function asyncTest(generator) {

I think this new function belongs to the top of head.js.
I know it doesn't make any difference, but I think it does for people who don't know the code much and look in head.js for simple helper functions they can use.

Comment 6

[Paul Rouget [:paul]]

Attachment: v1.1

Comment 7

[Paul Rouget [:paul]]

Attachment: v1.2

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8480385 [details] [diff] [review]
v1.2

Review of attachment 8480385 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/timeline.js
@@ +40,5 @@
> +  handle.removeTabActor(TimelineActor);
> +};
> +
> +/**
> + * The timeline actor pop and forward timeline markers registered in

nit: s/pop/pops and s/forward/forwards

Comment 9

[Paul Rouget [:paul]]

Attachment: to land

Comment 10

[Paul Rouget [:paul]]

Attachment: to land (r=pbrosset)

Marker names have changed. Updating the patch.

https://tbpl.mozilla.org/?tree=Try&rev=42daabc3a0e5

Comment 11

[Paul Rouget [:paul]]

implement `.disconnect`: https://tbpl.mozilla.org/?tree=Try&rev=57db3c6c1211

Comment 12

[Paul Rouget [:paul]]

Attachment: adding disconnect (r=pbrosset)

Comment 13

[Patrick Brosset <:pbro>]

In fx-team now: https://hg.mozilla.org/integration/fx-team/rev/75bad4fe63af

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/75bad4fe63af

Comment 15

[Andrei Vaida [:avaida]]

Setting qe-verify- since this patch is covered by automated testing. If there's something manual QA can look at here, please flip the flag.

Comment 16

[David Lawrence [:dkl]]

Sorry for the spam. Moving bugs to Firefox :: Developer Tools: Performance Tools (Profiler/Timeline).

dkl