Closed Bug 1382605 Opened 2 years ago Closed 2 years ago

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

Categories

(DevTools :: General, enhancement, P2)

enhancement

Tracking

(firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: zer0, Assigned: yulia)

References

Details

Attachments

(1 file, 1 obsolete file)

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
Flags: qe-verify-
Priority: -- → P2
No longer blocks: 1381542
Blocks: 1384546
Whiteboard: [nosdk]
Severity: normal → enhancement
Assignee: nobody → ystartsev
Attachment #8955136 - Flags: review?(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
Attachment #8955136 - Flags: review?(nchevobbe) → review+
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 :)
Attachment #8955136 - Flags: review+ → review?(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 ?
Attachment #8955136 - Flags: review?(nchevobbe) → review+
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!
Attachment #8955136 - Attachment is obsolete: true
Attachment #8956749 - Flags: review?(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
Attachment #8956749 - Flags: review?(nchevobbe) → review+
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
https://hg.mozilla.org/mozilla-central/rev/7fc308540c3d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.