1449170 - Remove old-event-emitter usage from responsive.html

Status: RESOLVED FIXED

(DevTools :: Responsive Design Mode, enhancement, P2)

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1449170 - Remove old-event-emitter usage from responsive.html; .

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

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8962712 [details]
Bug 1449170 - Remove old-event-emitter usage from responsive.html; .

https://reviewboard.mozilla.org/r/231576/#review237148

Thanks for working on this! :) It looks correct to me.

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8962712 [details]
Bug 1449170 - Remove old-event-emitter usage from responsive.html; .

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

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8962712 [details]
Bug 1449170 - Remove old-event-emitter usage from responsive.html; .

Clearing flag since there were some test failure that required some changes in tests

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8962712 [details]
Bug 1449170 - Remove old-event-emitter usage from responsive.html; .

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

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8962712 [details]
Bug 1449170 - Remove old-event-emitter usage from responsive.html; .

https://reviewboard.mozilla.org/r/231576/#review237560

Thanks for working on this! :) I would like to better understand why we can't use the `once` helper here, so marking r- for now.

::: devtools/client/responsive.html/test/browser/browser_exit_button.js:67
(Diff revision 3)
>    ok(manager.isActiveForTab(ui.tab),
>      "Responsive Design Mode active for the tab");
>  
>    exitButton.click();
>  
> -  await once(manager, "off");
> +  await manager.once("off");

Hmm, I really prefer to use the `once` helper in tests because logs "Waiting for event" / "Got event" which is quite helpful with intermittent failures.

Why was this change needed?

::: devtools/client/responsive.html/test/browser/browser_window_close.js:22
(Diff revision 3)
>    // Close the window on a tab with an active responsive design UI and
>    // wait for the UI to gracefully shutdown.  This has leaked the window
>    // in the past.
>    ok(ResponsiveUIManager.isActiveForTab(tab),
>       "ResponsiveUI should be active for tab when the window is closed");
> -  let offPromise = once(ResponsiveUIManager, "off");
> +  let offPromise = ResponsiveUIManager.once("off");

Same here, can we go back to the `once` helper?

::: devtools/client/styleeditor/test/browser_styleeditor_media_sidebar_links.js:104
(Diff revision 3)
>  }
>  
>  /* Helpers */
>  function waitForResizeTo(rdmUI, type, value) {
>    return new Promise(resolve => {
> -    let onResize = (_, data) => {
> +    let onResize = (data) => {

Sorry for missing this...!  I even tried a quick search over the style editor with the first reivew. :S

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8962712 [details]
Bug 1449170 - Remove old-event-emitter usage from responsive.html; .

https://reviewboard.mozilla.org/r/231576/#review237560

> Hmm, I really prefer to use the `once` helper in tests because logs "Waiting for event" / "Got event" which is quite helpful with intermittent failures.
> 
> Why was this change needed?

The change was made because the test was leaking in debug mode.
This is because the way `once` and the new emitter work, i.e. see https://searchfox.org/mozilla-central/rev/028cd8342899357d80fafba2d528a0fd5819e316/devtools/client/shared/test/shared-head.js#275

Before, the `onEvent` function would receive as arguments: the event name (here "off"), and then whatever was passed to the "emit" call (in the case of "off", it's an object containing the browser tab as a tab property).

So aArgs was ["off", {tab}], and calling `deferred.resolve.apply(deferred, aArgs);` would only resolve the deferred promise with the first argument, i.e. "off".

Now aArgs is [{tab}], and so the deferred promise resolves with {tab}, which causes the leak.

Switching to native promise in `waitForNEvents` resolves the issue. I feel like this should be done in a separate bug since it's risky and might make other tests fail. If it's okay for you, I'll file a bug to do that, and block this one on it, and rollback the `once` changes here.

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8962712 [details]
Bug 1449170 - Remove old-event-emitter usage from responsive.html; .

https://reviewboard.mozilla.org/r/231576/#review237560

> The change was made because the test was leaking in debug mode.
> This is because the way `once` and the new emitter work, i.e. see https://searchfox.org/mozilla-central/rev/028cd8342899357d80fafba2d528a0fd5819e316/devtools/client/shared/test/shared-head.js#275
> 
> Before, the `onEvent` function would receive as arguments: the event name (here "off"), and then whatever was passed to the "emit" call (in the case of "off", it's an object containing the browser tab as a tab property).
> 
> So aArgs was ["off", {tab}], and calling `deferred.resolve.apply(deferred, aArgs);` would only resolve the deferred promise with the first argument, i.e. "off".
> 
> Now aArgs is [{tab}], and so the deferred promise resolves with {tab}, which causes the leak.
> 
> Switching to native promise in `waitForNEvents` resolves the issue. I feel like this should be done in a separate bug since it's risky and might make other tests fail. If it's okay for you, I'll file a bug to do that, and block this one on it, and rollback the `once` changes here.

Ah, thanks for the explanation!  Yes, cleaning up that test helper first so it can still be used sounds like the right thing to do.  (Not totally sure I follow yet why native promises avoid the leak, but it seems like you've got it all figured out!) :)

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8962712 [details]
Bug 1449170 - Remove old-event-emitter usage from responsive.html; .

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

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8962712 [details]
Bug 1449170 - Remove old-event-emitter usage from responsive.html; .

https://reviewboard.mozilla.org/r/231576/#review237966

Great!  Thanks for working on it! :)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f37287132bf
Remove old-event-emitter usage from responsive.html; r=jryans.

Comment 12

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/3f37287132bf