Closed
Bug 1378849
Opened 7 years ago
Closed 7 years ago
Stop using sdk/core/heritage in DevTools Web Audio Editor
Categories
(DevTools :: General, enhancement, P1)
DevTools
General
Tracking
(firefox57 fixed)
RESOLVED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: sole, Assigned: pbro)
References
Details
(Whiteboard: [nosdk])
Attachments
(1 file)
Used in: devtools/client/webaudioeditor/includes.js
More details to follow as we triage.
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → sole
Reporter | ||
Comment 1•7 years 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 years ago
|
Assignee: sole → nobody
Updated•7 years ago
|
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [nosdk]
Assignee | ||
Comment 2•7 years ago
|
||
Extending EventTarget is what messes everything up. If the classes don't extend it, then their methods become defined again.
Assignee | ||
Comment 3•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Priority: P2 → P1
Target Milestone: --- → Firefox 57
Assignee | ||
Updated•7 years ago
|
Attachment #8898350 -
Flags: review?(spenades)
Assignee | ||
Comment 5•7 years ago
|
||
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) |
Assignee | ||
Comment 7•7 years ago
|
||
Reporter | ||
Comment 8•7 years 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+
Assignee | ||
Comment 9•7 years ago
|
||
(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•7 years 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•7 years ago
|
||
bugherder |
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•