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

RESOLVED FIXED in Firefox 57

Status

()

Firefox
Developer Tools
P1
normal
RESOLVED FIXED
8 months ago
6 months ago

People

(Reporter: sole, Assigned: pbro)

Tracking

unspecified
Firefox 57
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [nosdk])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

8 months ago
Used in: devtools/client/webaudioeditor/includes.js

More details to follow as we triage.
(Reporter)

Updated

7 months ago
Assignee: nobody → sole
(Reporter)

Comment 1

7 months ago
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.
(Reporter)

Updated

7 months ago
Assignee: sole → nobody
(Reporter)

Updated

7 months ago
Depends on: 1378894

Updated

7 months ago
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [nosdk]
Extending EventTarget is what messes everything up. If the classes don't extend it, then their methods become defined again.
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.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Updated

6 months ago
Priority: P2 → P1
Target Milestone: --- → Firefox 57
Attachment #8898350 - Flags: review?(spenades)
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.
Depends on: 1137935
Comment hidden (mozreview-request)
(Reporter)

Comment 8

6 months ago
mozreview-review
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?
Attachment #8898350 - Flags: review?(spenades) → review+
(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

6 months ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/12fecb86b637
Stop using add-on SDK APIs in the webaudio editor; r=sole

Comment 11

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/12fecb86b637
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.