Closed Bug 1378856 Opened 7 years ago Closed 7 years ago

Stop using sdk/core/heritage in DevTools performance server

Categories

(DevTools :: General, enhancement, P1)

Product:
DevTools
Component:
General
Type:
enhancement

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: sole, Assigned: Honza)

References

Details

(Whiteboard: [nosdk])

Attachments

(1 file)

Bug 1378856 - Stop using sdk/core/heritage in DevTools performance server;
59 bytes, text/x-review-board-request
zer0
: review+
Details 
Used in:

devtools/server/performance/framerate.js
devtools/server/performance/memory.js
devtools/server/performance/profiler.js
devtools/server/performance/timeline.js

More details to follow as we triage. Perhaps we need to split into four more bugs.
Used (lazy) also in:

devtools/server/performance/recorder.js
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [nosdk]
Some of the files also use `extends: EventTarget`, so this bug should wait until the EventEmitter situation has been sorted out in bug 952653 or bug Bug 1381542 ... possibly the second one.
Depends on: 1381542
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Priority: P2 → P1
Target Milestone: --- → Firefox 57
Comment on attachment 8896977 [details]
Bug 1378856 - Stop using sdk/core/heritage in DevTools performance server;

https://reviewboard.mozilla.org/r/168288/#review173528

thanks for the patch! I think it needs a bit more work. Once the sdk is gone this code will not work anymore as you're still including `sdk/*` stuff. Also you'll need to use an instance of EventEmitter-I left a comment with a link, you'll see.

::: devtools/server/performance/memory.js:12
(Diff revision 1)
>  const { reportException } = require("devtools/shared/DevToolsUtils");
> -const { Class } = require("sdk/core/heritage");
>  const { expectState } = require("devtools/server/actors/common");
> +const EventEmitter = require("devtools/shared/event-emitter");
> +
>  loader.lazyRequireGetter(this, "events", "sdk/event/core");

Would you not need to stop including this one?

::: devtools/server/performance/memory.js:13
(Diff revision 1)
> -const { Class } = require("sdk/core/heritage");
>  const { expectState } = require("devtools/server/actors/common");
> +const EventEmitter = require("devtools/shared/event-emitter");
> +
>  loader.lazyRequireGetter(this, "events", "sdk/event/core");
>  loader.lazyRequireGetter(this, "EventTarget", "sdk/event/target", true);

And not include this one either...

::: devtools/server/performance/memory.js:50
(Diff revision 1)
>  
> -    this._onGarbageCollection = this._onGarbageCollection.bind(this);
> +  this._onGarbageCollection = this._onGarbageCollection.bind(this);
> -    this._emitAllocations = this._emitAllocations.bind(this);
> +  this._emitAllocations = this._emitAllocations.bind(this);
> -    this._onWindowReady = this._onWindowReady.bind(this);
> +  this._onWindowReady = this._onWindowReady.bind(this);
>  
> -    events.on(this.parent, "window-ready", this._onWindowReady);
> +  events.on(this.parent, "window-ready", this._onWindowReady);

I think you might need to create an instance of EventEmitter and use that instance to send/get events instead of the events object, as described here: https://github.com/devtools-html/snippets-for-removing-the-sdk#differences-with-the-old-eventemitter

::: devtools/server/performance/memory.js:57
(Diff revision 1)
> +
> +Memory.prototype = {
> +  /**
> +   * Requires a root actor and a StackFrameCache.
> +   */
> +  initialize: function (parent, frameCache = new StackFrameCache()) {

do we still need this method? if you're doing the things in the new constructor, and this function is empty, perhaps it needs to be removed?

::: devtools/server/performance/profiler.js:10
(Diff revision 1)
>  
>  const { Cc, Ci, Cu } = require("chrome");
>  const Services = require("Services");
> -const { Class } = require("sdk/core/heritage");
> +const EventEmitter = require("devtools/shared/event-emitter");
> +
>  loader.lazyRequireGetter(this, "events", "sdk/event/core");

remove this?

::: devtools/server/performance/profiler.js:11
(Diff revision 1)
>  const { Cc, Ci, Cu } = require("chrome");
>  const Services = require("Services");
> -const { Class } = require("sdk/core/heritage");
> +const EventEmitter = require("devtools/shared/event-emitter");
> +
>  loader.lazyRequireGetter(this, "events", "sdk/event/core");
>  loader.lazyRequireGetter(this, "EventTarget", "sdk/event/target", true);

and remove this?

::: devtools/server/performance/recorder.js:12
(Diff revision 1)
>  loader.lazyRequireGetter(this, "promise");
> -loader.lazyRequireGetter(this, "Class",
> -  "sdk/core/heritage", true);
> -loader.lazyRequireGetter(this, "EventTarget",
> -  "sdk/event/target", true);
>  loader.lazyRequireGetter(this, "events",

Remove this?

::: devtools/server/performance/timeline.js:28
(Diff revision 1)
>  const { Ci, Cu } = require("chrome");
> -const { Class } = require("sdk/core/heritage");
> +const EventEmitter = require("devtools/shared/event-emitter");
> +
>  // Be aggressive about lazy loading, as this will run on every
>  // toolbox startup
>  loader.lazyRequireGetter(this, "events", "sdk/event/core");

Remove?
Attachment #8896977 - Flags: review-
Thanks for the review Sole!

I am attaching new version, all review comments resolved.

My question is whether using static on, off, emit methods is ok.
It looks like it does what we want, but I am not sure.

Like as follows:

const EventEmitter = require("devtools/shared/event-emitter");
const events = EventEmitter;
events.on(...)

Using static version of the methods allows specifying the `target`, while
using:

const events = new EventEmitter();

... always uses the 'EventEmitter' instance as the target.


There are still test failures, so the patch is still WIP.

Honza
I would advise to wait until Bug 1137935 is resolved before moving forward here. 

In Bug 1137935 I am trying to remove all usage of sdk/event/* in devtools at once. See https://bugzilla.mozilla.org/show_bug.cgi?id=1137935#c3 for an explanation why I think it's preferable to do it in one shot rather than in a file by file manner.

(In reply to Jan Honza Odvarko [:Honza] from comment #6)
> Thanks for the review Sole!
> 
> I am attaching new version, all review comments resolved.
> 
> My question is whether using static on, off, emit methods is ok.
> It looks like it does what we want, but I am not sure.
> 
> Like as follows:
> 
> const EventEmitter = require("devtools/shared/event-emitter");
> const events = EventEmitter;
> events.on(...)
> 

FWIW, I think using the static version works fine and is the easiest way to migrate existing code that relied on the sdk event-emitter.
Comment on attachment 8896977 [details]
Bug 1378856 - Stop using sdk/core/heritage in DevTools performance server;

https://reviewboard.mozilla.org/r/168288/#review177410

Looks good to me, if all tests are passing let's try to land this. However, I'd like to understand why in some cases we have ES6 classes and in others we don't. It might be worth to add comments if there are specific reason for that, and/or file a follow up bug to have more consistency (as we are doing for other similar bugs)

::: devtools/server/performance/memory.js:34
(Diff revision 3)
>   *
>   * To be consumed by actor's, like MemoryActor using this module to
>   * send information over RDP, and TimelineActor for using more light-weight
>   * utilities like GC events and measuring memory consumption.
>   */
> -exports.Memory = Class({
> +function Memory(parent, frameCache = new StackFrameCache()) {

Just out of curiosity, is there a reason why this is not a class?

::: devtools/server/performance/recorder.js:50
(Diff revision 3)
>   * shared by all tools in a target.
>   *
>   * @param Target target
>   *        The target owning this connection.
>   */
> -exports.PerformanceRecorder = Class({
> +function PerformanceRecorder(conn, tabActor) {

Same curiosity as above (maybe there are some decorators that are not visible in the review?).

::: devtools/server/performance/timeline.js:41
(Diff revision 3)
>  const DEFAULT_TIMELINE_DATA_PULL_TIMEOUT = 200;
>  
>  /**
>   * The timeline actor pops and forwards timeline markers registered in docshells.
>   */
> -exports.Timeline = Class({
> +function Timeline(tabActor) {

Ditto.
Attachment #8896977 - Flags: review?(zer0) → review+
Thanks for the review Matteo!

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #9)
> ::: devtools/server/performance/memory.js:34
> (Diff revision 3)
> >   *
> >   * To be consumed by actor's, like MemoryActor using this module to
> >   * send information over RDP, and TimelineActor for using more light-weight
> >   * utilities like GC events and measuring memory consumption.
> >   */
> > -exports.Memory = Class({
> > +function Memory(parent, frameCache = new StackFrameCache()) {
> 
> Just out of curiosity, is there a reason why this is not a class?
The Memory object users method decorators (`expectState`).
So, I am following:
https://github.com/devtools-html/snippets-for-removing-the-sdk#how-to-deal-with-decorators

> ::: devtools/server/performance/recorder.js:50
> (Diff revision 3)
> >   * shared by all tools in a target.
> >   *
> >   * @param Target target
> >   *        The target owning this connection.
> >   */
> > -exports.PerformanceRecorder = Class({
> > +function PerformanceRecorder(conn, tabActor) {
> 
> Same curiosity as above (maybe there are some decorators that are not
> visible in the review?).
Yep, precisely. Same as above.

> ::: devtools/server/performance/timeline.js:41
> (Diff revision 3)
> >  const DEFAULT_TIMELINE_DATA_PULL_TIMEOUT = 200;
> >  
> >  /**
> >   * The timeline actor pops and forwards timeline markers registered in docshells.
> >   */
> > -exports.Timeline = Class({
> > +function Timeline(tabActor) {
> 
> Ditto.
Again, decorators.

I am seeing some Try failures. I am working on fixing them.

Honza


Honza
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf2a7774f7b9
Stop using sdk/core/heritage in DevTools performance server; r=zer0
https://hg.mozilla.org/mozilla-central/rev/bf2a7774f7b9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
See Also: → 1395903
(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #1)
> Used (lazy) also in:
> 
> devtools/server/performance/recorder.js

This part was missed in the current patch, Sole filed a follow up bug to fix this: Bug 1378856
(Ah, clipboard didn't work: the bug was of course bug 1395903)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: