Failing tests: devtools/client/shared/components/test/mochitest/test_searchbox.html devtools/client/shared/test/browser_cubic-bezier-06.js devtools/client/shared/test/browser_key_shortcuts.js devtools/client/shared/test/browser_options-view-01.js devtools/client/shared/test/browser_spectrum.js devtools/client/shared/test/browser_tableWidget_basic.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 type event: 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, see: https://github.com/devtools-html/snippets-for-removing-the-sdk/#events

Comment on attachment 8955136 [details] Bug 1382605 - Fix 6 tests failures on devtools/client/shared due the EventEmitter refactoring https://reviewboard.mozilla.org/r/224296/#review230272 That's great Yulia, thanks a lot ! I only have a handful of nits, and one question, but this is good to go with a green try ::: devtools/client/inspector/rules/test/browser_rules_colorpicker-release-outside-frame.js:36 (Diff revision 1) > - spectrum.once("changed", (event, newValue) => { > + spectrum.once("changed", (newValue) => { > is(newValue, value, "Value changed on mousemove without a button pressed."); nit: no need to wrap single argument into parens ::: devtools/client/shared/components/SearchBox.js:53 (Diff revision 1) > } > > this.shortcuts = new KeyShortcuts({ > window > }); > - this.shortcuts.on(this.props.keyShortcut, (name, event) => { > + this.shortcuts.on(this.props.keyShortcut, (event) => { nit: no need to wrap single params in parens ::: devtools/client/shared/test/browser_key_shortcuts.js:37 (Diff revision 1) > // Test helper to listen to the next key press for a given key, > // returning a promise to help using Tasks. > function once(shortcuts, key, listener) { > let called = false; > return new Promise(done => { > - let onShortcut = (key2, event) => { > + let onShortcut = (event) => { nit: no need to wrap single params in parens (same for every occurence in this file) ::: devtools/client/shared/test/browser_options-view-01.js:44 (Diff revision 1) > let options = createOptionsView(win); > yield options.initialize(); > > let $ = win.document.querySelector.bind(win.document); > > - options.on("pref-changed", (_, pref) => events.push(pref)); > + options.on("pref-changed", (pref) => events.push(pref)); nit: no need to wrap single params in parens ::: devtools/client/shared/test/browser_toolbar_webconsole_errors_count.js:249 (Diff revision 1) > > if (!check()) { > info("wait for: " + options.name); > - toolbar.on("errors-counter-updated", function onUpdate(event) { > + toolbar.on("errors-counter-updated", function onUpdate() { > if (check()) { > - toolbar.off(event, onUpdate); > + toolbar.off(null, onUpdate); shouldn't this be "error-counter-updated" instead of `null` ? ::: devtools/client/shared/widgets/TableWidget.js:109 (Diff revision 1) > this.setColumns(initialColumns, uniqueId); > } else if (this.emptyText) { > this.setPlaceholderText(this.emptyText); > } > > - this.bindSelectedRow = (event, id) => { > + this.bindSelectedRow = (id) => { nit: no need to wrap single params in parens ::: devtools/client/shared/widgets/TableWidget.js:316 (Diff revision 1) > // select the appropriate row and move the textbox on to the next cell. > if (editor.changePending) { > // We need to apply a change, which can mean that the position of cells > // within the table can change. Because of this we need to wait for > // EVENTS.ROW_EDIT and then move the textbox. > - this.once(EVENTS.ROW_EDIT, (e, uniqueId) => { > + this.once(EVENTS.ROW_EDIT, (uniqueId) => { nit: no need to wrap single params in parens

Comment on attachment 8955136 [details] Bug 1382605 - Fix 6 tests failures on devtools/client/shared due the EventEmitter refactoring https://reviewboard.mozilla.org/r/224296/#review231356 ::: npm-shrinkwrap.json:1 (Diff revision 3) > { this shouldnt be here

Comment on attachment 8955136 [details] Bug 1382605 - Fix 6 tests failures on devtools/client/shared due the EventEmitter refactoring resetting flag for tomorrow-self to think about looking into this :)

Comment on attachment 8955136 [details] Bug 1382605 - Fix 6 tests failures on devtools/client/shared due the EventEmitter refactoring https://reviewboard.mozilla.org/r/224296/#review231530 This is a huge work \o/, thanks a ton for doing that Yulia ! This is good to go once the comment is addressed and the shrinkwrap file is reverted :) ::: devtools/client/inspector/breadcrumbs.js:391 (Diff revision 3) > - this.shortcuts.on("Right", this.handleShortcut); > - this.shortcuts.on("Left", this.handleShortcut); > + this.shortcuts.on("Right", event => this.handleShortcut("Right", event)); > + this.shortcuts.on("Left", event => this.handleShortcut("Left", event)); here maybe we can keep the same thing as before, and in handleShortcut check for `event.code === "ArrowRight"` ? Not madatory ::: devtools/client/inspector/rules/rules.js:700 (Diff revision 3) > - this._prefObserver.off(PREF_UA_STYLES, this._handlePrefChange); > - this._prefObserver.off(PREF_DEFAULT_COLOR_UNIT, this._handlePrefChange); > + this._prefObserver.off(PREF_UA_STYLES, () => this._handlePrefChange(PREF_UA_STYLES)); > + this._prefObserver.off( > + PREF_DEFAULT_COLOR_UNIT, > + () => this._handlePrefChange(PREF_DEFAULT_COLOR_UNIT) > + ); i am not sure this works. If I recall correctly, we need to pass the same reference passed in `on`. Here we create a new inline function. Could we split handlePrefChange in 2 functions (handlePUaStylesPrefChange, handleDefaultColorUnitPrefChange) , which then call handlePrefChange with the name of the pref. That way we have a proper reference that we can pass to `off`. ::: devtools/client/performance/views/details-abstract-subview.js:168 (Diff revision 3) > }, > > /** > * Fired when a preference in `devtools.performance.ui.` is changed. > */ > - _onPrefChanged: function (_, prefName) { > + _onPrefChanged: function (_, prefName, prefValue) { is the first, `_`, argument still passed here ?

Comment on attachment 8955136 [details] Bug 1382605 - Fix 6 tests failures on devtools/client/shared due the EventEmitter refactoring https://reviewboard.mozilla.org/r/224296/#review231530 > i am not sure this works. If I recall correctly, we need to pass the same reference passed in `on`. Here we create a new inline function. > > Could we split handlePrefChange in 2 functions (handlePUaStylesPrefChange, handleDefaultColorUnitPrefChange) , which then call handlePrefChange with the name of the pref. > > That way we have a proper reference that we can pass to `off`. oh nice catch, thanks! > is the first, `_`, argument still passed here ? yes, it took quite some time to figure that out, because perf wraps pref changes with its own event, and these are listening to that instead. the prefValue should be removed though!

Comment on attachment 8956749 [details] Bug 1382605 - Fix 6 tests failures on devtools/client/shared due the EventEmitter refactoring https://reviewboard.mozilla.org/r/225712/#review231548 Thanks for addressing the comments :) Let's land this

Pushed by ystartsev@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7fc308540c3d Fix 6 tests failures on devtools/client/shared due the EventEmitter refactoring r=nchevobbe