Closed Bug 1449170 Opened 3 years ago Closed 3 years ago

Remove old-event-emitter usage from responsive.html

Categories

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

enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: nchevobbe, Assigned: nchevobbe)

References

Details

Attachments

(1 file)

No description provided.
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
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.
Attachment #8962712 - Flags: review?(jryans) → review+
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
Attachment #8962712 - Flags: review+
Attachment #8962712 - Flags: review?(jryans)
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
Attachment #8962712 - Flags: review?(jryans) → review-
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.
Depends on: 1449635
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 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! :)
Attachment #8962712 - Flags: review?(jryans) → review+
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3f37287132bf
Remove old-event-emitter usage from responsive.html; r=jryans.
https://hg.mozilla.org/mozilla-central/rev/3f37287132bf
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.