Closed
Bug 1050384
Opened 10 years ago
Closed 10 years ago
[timeline] build an actor to forward gecko operations
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect)
Tracking
(firefox35 fixed)
RESOLVED
FIXED
Firefox 35
Tracking | Status | |
---|---|---|
firefox35 | --- | fixed |
People
(Reporter: paul, Assigned: paul)
References
Details
Attachments
(1 file, 8 obsolete files)
(deleted),
patch
|
paul
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Blocks: timeline-v1
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8469401 -
Attachment is obsolete: true
Attachment #8470484 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Component: Developer Tools → Developer Tools: Timeline
Assignee | ||
Comment 4•10 years ago
|
||
Assignee: nobody → paul
Attachment #8470540 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8475867 -
Flags: feedback?(pbrosset)
Comment 5•10 years ago
|
||
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.
Attachment #8475867 -
Flags: feedback?(pbrosset) → feedback+
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8475867 -
Attachment is obsolete: true
Attachment #8476480 -
Attachment is obsolete: true
Attachment #8480385 -
Flags: review?(pbrosset)
Comment 8•10 years ago
|
||
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
Attachment #8480385 -
Flags: review?(pbrosset) → review+
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8480385 -
Attachment is obsolete: true
Attachment #8480405 -
Flags: review+
Assignee | ||
Comment 10•10 years ago
|
||
Marker names have changed. Updating the patch.
https://tbpl.mozilla.org/?tree=Try&rev=42daabc3a0e5
Attachment #8480405 -
Attachment is obsolete: true
Attachment #8486276 -
Flags: review+
Assignee | ||
Comment 11•10 years ago
|
||
implement `.disconnect`: https://tbpl.mozilla.org/?tree=Try&rev=57db3c6c1211
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8486276 -
Attachment is obsolete: true
Attachment #8486362 -
Flags: review+
Comment 13•10 years ago
|
||
In fx-team now: https://hg.mozilla.org/integration/fx-team/rev/75bad4fe63af
Comment 14•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Comment 15•10 years ago
|
||
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.
status-firefox35:
--- → fixed
Flags: qe-verify-
Comment 16•10 years ago
|
||
Sorry for the spam. Moving bugs to Firefox :: Developer Tools: Performance Tools (Profiler/Timeline).
dkl
Component: Developer Tools: Timeline → Developer Tools: Performance Tools (Profiler/Timeline)
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•