Failing tests: devtools/client/framework/test/browser_devtools_api.js devtools/client/framework/test/browser_devtools_api_destroy.js devtools/client/framework/test/browser_devtools_shim.js devtools/client/framework/test/browser_keybindings_01.js devtools/client/framework/test/browser_source_map-01.js devtools/client/framework/test/browser_source_map-absolute.js devtools/client/framework/test/browser_source_map-inline.js devtools/client/framework/test/browser_source_map-no-race.js devtools/client/framework/test/browser_source_map-reload.js devtools/client/framework/test/browser_toolbox_dynamic_registration.js devtools/client/framework/test/browser_toolbox_getpanelwhenready.js devtools/client/framework/test/browser_toolbox_options.js devtools/client/framework/test/browser_toolbox_options_disable_buttons.js devtools/client/framework/test/browser_toolbox_sidebar.js devtools/client/framework/test/browser_toolbox_sidebar_events.js devtools/client/framework/test/browser_toolbox_theme_registration.js The refactoring is currently only on: https://github.com/zer0/gecko/tree/event-emitter-1381542 We need to address the test failures before land this patch in m-c.

Here the original try build with the failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bba13e27a2371fa8aad68b9b227534b31829cb0d Those failures are most likely due the breaking change in how the `EventEmitter` emits event. Previously, the first argument was the event type: myEmitter.on("custom-event", (eventType, message) => { ... }); Now the first argument is the message: myEmitter.on("custom-event", (message) => { ... }); In the majority of the scenario the `eventType` is ignored by our code, so we should just remove it from the function's signature. For more details and edge cases, see: https://github.com/devtools-html/snippets-for-removing-the-sdk/#events

Comment on attachment 8958881 [details] Bug 1382581 - Remove old-event-emitter usage from devtools/client/framework; . https://reviewboard.mozilla.org/r/227752/#review233520 Nice! ::: devtools/client/framework/test/browser_target_events.js:47 (Diff revision 1) > - target.once("navigate", executeSoon.bind(null, onNavigate)); > } > > function onNavigate() { > ok(true, "navigate event received"); > + target.off("navigate", onNavigate); Why not continue to use `once` instead of `on`/`off`?

Comment on attachment 8958887 [details] Bug 1382581 - Adapt webconsole code to the EventEmitter change in devtools/client/framework; . https://reviewboard.mozilla.org/r/227764/#review233532

Comment on attachment 8958882 [details] Bug 1382581 - Adapt extension code to the EventEmitter change in devtools/client/framework; . https://reviewboard.mozilla.org/r/227754/#review233538 Looks great, thanks Nicolas for taking care of it also here (and it also cleaned up a bunch of unused parameters).

Comment on attachment 8958885 [details] Bug 1382581 - Adapt inspector code to the EventEmitter change in devtools/client/framework; . https://reviewboard.mozilla.org/r/227760/#review233804 ::: devtools/client/inspector/test/shared-head.js:50 (Diff revision 1) > -var openInspectorSidebarTab = Task.async(function* (id) { > - let {toolbox, inspector, testActor} = yield openInspector(); > +var openInspectorSidebarTab = async function(id) { > + let {toolbox, inspector, testActor} = await openInspector(); > > info("Selecting the " + id + " sidebar"); > > let onSidebarSelect = inspector.sidebar.once("select"); > if (id === "computedview" || id === "layoutview") { > // The layout and computed views should wait until the box-model widget is ready. > let onBoxModelViewReady = inspector.once("boxmodel-view-updated"); > // The layout view also needs to wait for the grid panel to be fully updated. > let onGridPanelReady = id === "layoutview" ? > inspector.once("grid-panel-updated") : Promise.resolve(); > inspector.sidebar.select(id); > - yield onBoxModelViewReady; > - yield onGridPanelReady; > + await onBoxModelViewReady; > + await onGridPanelReady; > } else { > inspector.sidebar.select(id); > } > - yield onSidebarSelect; > + await onSidebarSelect; > > return { > toolbox, > inspector, > testActor > }; > -}); > +}; Althought nice to switch to async/await, this seems unrelated to your patch. Maybe remove it?

(In reply to Patrick Brosset <:pbro> from comment #12) > Comment on attachment 8958885 [details] > Bug 1382581 - Adapt inspector code to the EventEmitter change in > devtools/client/framework; . > > https://reviewboard.mozilla.org/r/227760/#review233804 > > ::: devtools/client/inspector/test/shared-head.js:50 > (Diff revision 1) > > -var openInspectorSidebarTab = Task.async(function* (id) { > > - let {toolbox, inspector, testActor} = yield openInspector(); > > +var openInspectorSidebarTab = async function(id) { > > + let {toolbox, inspector, testActor} = await openInspector(); > > > > info("Selecting the " + id + " sidebar"); > > > > let onSidebarSelect = inspector.sidebar.once("select"); > > if (id === "computedview" || id === "layoutview") { > > // The layout and computed views should wait until the box-model widget is ready. > > let onBoxModelViewReady = inspector.once("boxmodel-view-updated"); > > // The layout view also needs to wait for the grid panel to be fully updated. > > let onGridPanelReady = id === "layoutview" ? > > inspector.once("grid-panel-updated") : Promise.resolve(); > > inspector.sidebar.select(id); > > - yield onBoxModelViewReady; > > - yield onGridPanelReady; > > + await onBoxModelViewReady; > > + await onGridPanelReady; > > } else { > > inspector.sidebar.select(id); > > } > > - yield onSidebarSelect; > > + await onSidebarSelect; > > > > return { > > toolbox, > > inspector, > > testActor > > }; > > -}); > > +}; > > Althought nice to switch to async/await, this seems unrelated to your patch. > Maybe remove it? Not sure why this ended up here :) removed

Comment on attachment 8958884 [details] Bug 1382581 - Adapt debugger code to the EventEmitter change in devtools/client/framework; . https://reviewboard.mozilla.org/r/227758/#review233816 Looks good to me, do you also have a patch for the github repo?

(In reply to Brian Grinstead [:bgrins] from comment #9) > Comment on attachment 8958881 [details] > Bug 1382581 - Remove old-event-emitter usage from devtools/client/framework; > . > > https://reviewboard.mozilla.org/r/227752/#review233520 > > Nice! > > ::: devtools/client/framework/test/browser_target_events.js:47 > (Diff revision 1) > > - target.once("navigate", executeSoon.bind(null, onNavigate)); > > } > > > > function onNavigate() { > > ok(true, "navigate event received"); > > + target.off("navigate", onNavigate); > > Why not continue to use `once` instead of `on`/`off`? I had memory leaks with once. Let's see if I can find the root of the issue

(In reply to Yulia Startsev [:yulia] from comment #14) > Comment on attachment 8958884 [details] > Bug 1382581 - Adapt debugger code to the EventEmitter change in > devtools/client/framework; . > > https://reviewboard.mozilla.org/r/227758/#review233816 > > Looks good to me, do you also have a patch for the github repo? Not yet

Comment on attachment 8958883 [details] Bug 1382581 - Adapt canvasdebugger code to the EventEmitter change in devtools/client/framework; . https://reviewboard.mozilla.org/r/227756/#review234132 I'm not sure I'm super qualified to review Canvas debugger code, but it looks like the change is simple enough, and the tests that are failing aren't related to your change so I'm r+ing it! Good work on removing events that aren't used (I guess). ::: devtools/client/canvasdebugger/canvasdebugger.js (Diff revision 1) > // globals. > gFront.setup({ reload: false }); > > - this._onTabNavigated = this._onTabNavigated.bind(this); > - gTarget.on("will-navigate", this._onTabNavigated); > + this._onTabWillNavigate = this._onTabWillNavigate.bind(this); > + gTarget.on("will-navigate", this._onTabWillNavigate); > - gTarget.on("navigate", this._onTabNavigated); I'm curious as to why you removed this, is the event not used at all?

Comment on attachment 8958886 [details] Bug 1382581 - Adapt webaudioeditor code to the EventEmitter change in devtools/client/framework; . https://reviewboard.mozilla.org/r/227762/#review234136 Looks almost good except for that strange indentation issue; r+ing the patch but please fix the whitespace weirdness :) ::: devtools/client/webaudioeditor/controller.js:145 (Diff revision 1) > - this.reset(); > +this.reset(); > > - // When switching to an iframe, ensure displaying the reload button. > +// When switching to an iframe, ensure displaying the reload button. > - // As the document has already been loaded without being hooked. > +// As the document has already been loaded without being hooked. > - if (isFrameSwitching) { > +if (isFrameSwitching) { > - $("#reload-notice").hidden = false; > +$("#reload-notice").hidden = false; > - $("#waiting-notice").hidden = true; > +$("#waiting-notice").hidden = true; > - } else { > +} else { > - // Otherwise, we are loading a new top level document, > +// Otherwise, we are loading a new top level document, > - // so we don't need to reload anymore and should receive > +// so we don't need to reload anymore and should receive > - // new node events. > +// new node events. > - $("#reload-notice").hidden = true; > +$("#reload-notice").hidden = true; > - $("#waiting-notice").hidden = false; > +$("#waiting-notice").hidden = false; > - } > +} > > - // Clear out stored audio nodes > + // Clear out stored audio nodes > - gAudioNodes.reset(); > + gAudioNodes.reset(); > > - window.emit(EVENTS.UI_RESET); > + window.emit(EVENTS.UI_RESET); What happened here with the white space? Mozreview shows me no indentation at all... Would you please fix if it's missing? :-) ::: devtools/client/webaudioeditor/controller.js (Diff revision 1) > - case "navigate": { > - // TODO Case of bfcache, needs investigating > - // bug 994250 > - break; > - } thanks for removing unused code as the bug is WONTFIX'ed!

Comment on attachment 8958886 [details] Bug 1382581 - Adapt webaudioeditor code to the EventEmitter change in devtools/client/framework; . https://reviewboard.mozilla.org/r/227762/#review234136 > What happened here with the white space? Mozreview shows me no indentation at all... Would you please fix if it's missing? :-) oh yes, I think it was something I did during a conflict resolution

Comment on attachment 8958881 [details] Bug 1382581 - Remove old-event-emitter usage from devtools/client/framework; . https://reviewboard.mozilla.org/r/227752/#review233520 > Why not continue to use `once` instead of `on`/`off`? So, it's causing a CPOW error. I'll file a follow p bug to investigate this, since the patch is pretty big and already went through multiple rebase with conflicts :D

Comment on attachment 8958883 [details] Bug 1382581 - Adapt canvasdebugger code to the EventEmitter change in devtools/client/framework; . https://reviewboard.mozilla.org/r/227756/#review234132 > I'm curious as to why you removed this, is the event not used at all? Yes ! Here is what was done: ```js _onTabNavigated: function (event) { if (event != "will-navigate") { return; } ``` So, there wasn't any point having the listener in the first place (I guess it did at some point and then the code changed)

Comment on attachment 8959643 [details] Bug 1382581 - Adapt shadereditor code to the EventEmitter change in devtools/client/framework; . https://reviewboard.mozilla.org/r/228458/#review234318

Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8ef404b01c4a Remove old-event-emitter usage from devtools/client/framework; r=bgrins. https://hg.mozilla.org/integration/autoland/rev/2d11a3944038 Adapt extension code to the EventEmitter change in devtools/client/framework; r=rpl. https://hg.mozilla.org/integration/autoland/rev/072a665b0181 Adapt canvasdebugger code to the EventEmitter change in devtools/client/framework; r=sole. https://hg.mozilla.org/integration/autoland/rev/4815e9bd7de0 Adapt debugger code to the EventEmitter change in devtools/client/framework; r=yulia. https://hg.mozilla.org/integration/autoland/rev/3ccfccd21cc7 Adapt inspector code to the EventEmitter change in devtools/client/framework; r=pbro. https://hg.mozilla.org/integration/autoland/rev/79e040625d0d Adapt webaudioeditor code to the EventEmitter change in devtools/client/framework; r=sole. https://hg.mozilla.org/integration/autoland/rev/0027f4cb4994 Adapt webconsole code to the EventEmitter change in devtools/client/framework; r=bgrins. https://hg.mozilla.org/integration/autoland/rev/3214b1a3f235 Adapt shadereditor code to the EventEmitter change in devtools/client/framework; r=bgrins.

https://hg.mozilla.org/mozilla-central/rev/8ef404b01c4a https://hg.mozilla.org/mozilla-central/rev/2d11a3944038 https://hg.mozilla.org/mozilla-central/rev/072a665b0181 https://hg.mozilla.org/mozilla-central/rev/4815e9bd7de0 https://hg.mozilla.org/mozilla-central/rev/3ccfccd21cc7 https://hg.mozilla.org/mozilla-central/rev/79e040625d0d https://hg.mozilla.org/mozilla-central/rev/0027f4cb4994 https://hg.mozilla.org/mozilla-central/rev/3214b1a3f235