Closed
Bug 1159480
Opened 9 years ago
Closed 9 years ago
Pull out actor-specific logic from the PerformanceConnection
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect, P1)
Tracking
(firefox40 fixed)
RESOLVED
FIXED
Firefox 40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: jsantell, Assigned: jsantell)
References
Details
Attachments
(1 file, 2 obsolete files)
40.97 KB,
patch
|
jsantell
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•9 years ago
|
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
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)
Updated•9 years ago
|
Priority: -- → P1
Comment 3•9 years ago
|
||
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 4•9 years ago
|
||
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+
Assignee | ||
Comment 5•9 years ago
|
||
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.");
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8599659 -
Attachment is obsolete: true
Attachment #8600463 -
Flags: review+
Assignee | ||
Comment 7•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/d58edbe9f922
Whiteboard: [fixed-in-fx-team]
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d58edbe9f922
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•