1378849 - Stop using sdk/core/heritage in DevTools Web Audio Editor

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P1)

Description

[Soledad Penades [:sole] [:spenades]]

Used in: devtools/client/webaudioeditor/includes.js

More details to follow as we triage.

Comment 1

[Soledad Penades [:sole] [:spenades]]

OK I optimistically tried to tackle this... without much success so far.

I commented out the require importing sdk/core/heritage and then modified the models.js file to use ES6 classes, but when it comes to actually running the Web Audio Editor panel, it looks as if the models are losing their own methods - they look as undefined.

The error is: 

```
TypeError: gAudioNodes.get is not a function: _onNodeSet@chrome://devtools/content/webaudioeditor/views/properties.js:123:24
emit@resource://devtools/shared/event-emitter.js:194:13
InspectorView.setCurrentAudioNode<@chrome://devtools/content/webaudioeditor/views/inspector.js:87:7
_run@resource://devtools/shared/task.js:311:39
TaskImpl@resource://devtools/shared/task.js:273:3
asyncFunction@resource://devtools/shared/task.js:247:14
resetUI@chrome://devtools/content/webaudioeditor/views/inspector.js:110:5
reset@chrome://devtools/content/webaudioeditor/controller.js:147:5
WebAudioEditorController._onTabNavigated<@chrome://devtools/content/webaudioeditor/controller.js:188:9
_run@resource://devtools/shared/task.js:311:39
TaskImpl@resource://devtools/shared/task.js:273:3
asyncFunction@resource://devtools/shared/task.js:247:14
emit@resource://devtools/shared/event-emitter.js:194:13
_setupRemoteListeners/this._onTabNavigated@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/target.js:553:9
eventSource/proto.emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:130:9
onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:1018:7
send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:570:13
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
```

gAudioNodes is an instance of AudioNodesCollection, so it *should* have the methods it declares. But no, they're undefined (as you can check with `dump(gAudioNodes.get)`.

Strangely, the instance *does* have the inherited EventTarget methods.

We discussed it might be better to leave this bug for later as we need to work on the EventEmitter stuff as well, so I'm leaving the changes I did here: https://github.com/mozilla/gecko/compare/central...sole:bug1378849

and deassigning myself from this bug for now.

Comment 2

[Patrick Brosset <:pbro>]

Extending EventTarget is what messes everything up. If the classes don't extend it, then their methods become defined again.

Comment 3

[Patrick Brosset <:pbro>]

I think I've got a fix. I've had to convert the models.js file away from EventTarget and use Event-Emitter instead, but that itself required other fixes because events aren't sent the same way.
Things look good locally, I'll push to try.

Comment 4

[Patrick Brosset <:pbro>]

Attachment: Bug 1378849 - Stop using add-on SDK APIs in the webaudio editor;

Review commit: https://reviewboard.mozilla.org/r/169718/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169718/

Comment 5

[Patrick Brosset <:pbro>]

Actually, let's hold off, I just discussed with Julian and his work on bug 1137935 is conflicting.
He's changing all event-related things, and I've had to do that in this bug too, but just for the webaudio editor. The goal of this bug is only to remove the Heritage usage, but to do that I've had to change how events are handled. So let's block on Julian's bug, and when it's done, I can simply remove the Heritage usage.

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8898350 [details]
Bug 1378849 - Stop using add-on SDK APIs in the webaudio editor;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169718/diff/1-2/

Comment 7

[Patrick Brosset <:pbro>]

Try push for this change: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2281c3a32d37822dc7a035b2fc7c83c8deb4d00&group_state=expanded

Comment 8

[Soledad Penades [:sole] [:spenades]]

Comment on attachment 8898350 [details]
Bug 1378849 - Stop using add-on SDK APIs in the webaudio editor;

https://reviewboard.mozilla.org/r/169718/#review177320

Apart from the question about the OldEventEmitter being included, this looks good to me - it's what I was trying to do but with an EventEmitter that doesn't break the world when it's extended!

Even if the try build has green in our tests, did you try with a Web Audio page? You could try this http://sole.github.io/test_cases/web_audio/thx_cutting_out/ and see if the graph is rendered (press any of the buttons to create the context and start the sound)

::: devtools/client/webaudioeditor/includes.js:11
(Diff revision 2)
>  
>  const { loader, require } = Cu.import("resource://devtools/shared/Loader.jsm", {});
>  const { XPCOMUtils } = require("resource://gre/modules/XPCOMUtils.jsm");
>  const { Task } = require("devtools/shared/task");
> -const { Class } = require("sdk/core/heritage");
>  const OldEventEmitter = require("devtools/shared/old-event-emitter");

Is the old event emitter still used?

Comment 9

[Patrick Brosset <:pbro>]

(In reply to Soledad Penades [:sole] [:spenades] from comment #8)
> it's what I was trying to do but with an EventEmitter that
> doesn't break the world when it's extended!
Thanks Julian for having migrated all the events! It made this bug so much easier to fix.

> Even if the try build has green in our tests, did you try with a Web Audio
> page? You could try this
> http://sole.github.io/test_cases/web_audio/thx_cutting_out/ and see if the
> graph is rendered (press any of the buttons to create the context and start
> the sound)
Just tested, this seems to be working well. I can see nodes coming and going. And I can click on them to inspect them.

> ::: devtools/client/webaudioeditor/includes.js:11
> (Diff revision 2)
> >  
> >  const { loader, require } = Cu.import("resource://devtools/shared/Loader.jsm", {});
> >  const { XPCOMUtils } = require("resource://gre/modules/XPCOMUtils.jsm");
> >  const { Task } = require("devtools/shared/task");
> > -const { Class } = require("sdk/core/heritage");
> >  const OldEventEmitter = require("devtools/shared/old-event-emitter");
> 
> Is the old event emitter still used?
Huh, I didn't even realize this was here, I was merely focusing on removing the heritage dependency. The goal is to get rid of it of course, but I think we're still in the transition period.
I think we should fix this as part of bug 1384546.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12fecb86b637
Stop using add-on SDK APIs in the webaudio editor; r=sole

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/12fecb86b637