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)

enhancement

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.
Assignee: nobody → sole
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.
Assignee: sole → nobody
Depends on: 1378894
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
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 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.
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: