1382605 - Fix 6 tests failures on devtools/client/shared due the EventEmitter refactoring

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P2)

Description

[Matteo Ferretti [:zer0] [:matteo]]

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.

Comment 1

[Matteo Ferretti [:zer0] [:matteo]]

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 2

[Yulia Startsev [:yulia]]

Attachment: Bug 1382605 - Fix 6 tests failures on devtools/client/shared due the EventEmitter refactoring

Review commit: https://reviewboard.mozilla.org/r/224296/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224296/

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

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 4

[Yulia Startsev [:yulia]]

Comment on attachment 8955136 [details]
Bug 1382605 - Fix 6 tests failures on devtools/client/shared due the EventEmitter refactoring

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224296/diff/1-2/

Comment 5

[Yulia Startsev [:yulia]]

Comment on attachment 8955136 [details]
Bug 1382605 - Fix 6 tests failures on devtools/client/shared due the EventEmitter refactoring

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224296/diff/2-3/

Comment 6

[Yulia Startsev [:yulia]]

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 7

[Nicolas Chevobbe [:nchevobbe]]

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 8

[Nicolas Chevobbe [:nchevobbe]]

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 9

[Yulia Startsev [:yulia]]

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 10

[Yulia Startsev [:yulia]]

Attachment: Bug 1382605 - Fix 6 tests failures on devtools/client/shared due the EventEmitter refactoring

Review commit: https://reviewboard.mozilla.org/r/225712/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225712/

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 12

[Yulia Startsev [:yulia]]

Comment on attachment 8956749 [details]
Bug 1382605 - Fix 6 tests failures on devtools/client/shared due the EventEmitter refactoring

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225712/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7fc308540c3d
Fix 6 tests failures on devtools/client/shared due the EventEmitter refactoring r=nchevobbe

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/7fc308540c3d