Closed Bug 1159480 Opened 6 years ago Closed 6 years ago

Pull out actor-specific logic from the PerformanceConnection

Categories

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

37 Branch
defect

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(1 file, 2 obsolete files)

We have a lot of actor-specific logic in the perf connection, where we deal with polling for allocations, massaging the timeline events into one event, a whole bunch of profiler work-around magic, making all RDP calls directly with a _request method, determining if we need to use a mock actor, etc.

These should all be handled elsewhere, with the performance connection just piecing together these different sources.

The impetus for this was wanting an event for the profiler for the buffer state, and so I cleaned things up a bit.
Assignee: nobody → jsantell
Blocks: 1145187, 1159389
Status: NEW → ASSIGNED
Made the actor APIs consistent as well -- each have start/stop. Polling for allocation sites is now an event listened to on the connection. From here we can also do compatibility data massaging for older geckos.

This is very nice. I've had a good day.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4457be0d40f2
Attachment #8598998 - Flags: review?(vporof)
Had to do a good rebase.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b271e6be8443
Attachment #8598998 - Attachment is obsolete: true
Attachment #8598998 - Flags: review?(vporof)
Attachment #8599659 - Flags: review?(vporof)
Priority: -- → P1
This is basically r+, but I'm waiting for bug 1160516 because I can't build again. Stand by, investigating, shouldn't take long.
Comment on attachment 8599659 [details] [diff] [review]
1159480-perf-front-refactor.patch

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

::: browser/devtools/performance/modules/front.js
@@ +22,2 @@
>  loader.lazyImporter(this, "Promise",
>    "resource://gre/modules/Promise.jsm");

I'm just noticing this now, why is Promise imported twice??!

::: browser/devtools/performance/test/browser_perf-shared-connection-02.js
@@ -18,5 @@
>  
>    ok(sharedConnection,
>      "A shared profiler connection for the current toolbox was retrieved.");
> -  is(sharedConnection._request, panel.panelWin.gFront._request,
> -    "The same shared profiler connection is used by the panel's front.");

We still need some way to test this. Need to make sure it's the same shared connection everytime. The way we checked this was pretty horrible, granted, but this has to be replaced with something else.
Attachment #8599659 - Flags: review?(vporof) → review+
Comment on attachment 8599659 [details] [diff] [review]
1159480-perf-front-refactor.patch

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

::: browser/devtools/performance/modules/front.js
@@ +8,5 @@
>  const { extend } = require("sdk/util/object");
>  const { RecordingModel } = require("devtools/performance/recording-model");
>  
>  loader.lazyRequireGetter(this, "Services");
>  loader.lazyRequireGetter(this, "promise");

Removing this. I hate lowercase `promise`, not to mention no indication of which "promise" this is

::: browser/devtools/performance/test/browser_perf-shared-connection-02.js
@@ -18,5 @@
>  
>    ok(sharedConnection,
>      "A shared profiler connection for the current toolbox was retrieved.");
> -  is(sharedConnection._request, panel.panelWin.gFront._request,
> -    "The same shared profiler connection is used by the panel's front.");

Adding a test, this is a better way:

is(panel.panelWin.gFront._connection, sharedConnection,
  "The same shared profiler connection is used by the panel's front.");
Attachment #8599659 - Attachment is obsolete: true
Attachment #8600463 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d58edbe9f922
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.