1476137 - Remove promiseWindowWillBeClosed and use BrowserTestUtils.domWindowClosed

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P5)

Description

[Marco Castelluccio [:marco]]

browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js, browser/base/content/test/general/browser_bug462673.js and browser/base/content/test/general/browser_save_link_when_window_navigates.js are using a promiseWindowWillBeClosed function (defined in browser/base/content/test/general/head.js), which should be equivalent to the BrowserTestUtils.domWindowClosed function.

Comment 1

[Sandeep Bypina [:sandeepb]]

Attachment: Bug 1476137 - Remove promiseWindowWillBeClosed and use BrowserTestUtils.domWindowClosed

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

Comment 2

[Sandeep Bypina [:sandeepb]]

(In reply to Marco Castelluccio [:marco] from comment #0)
> browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js,
> browser/base/content/test/general/browser_bug462673.js and
> browser/base/content/test/general/browser_save_link_when_window_navigates.js
> are using a promiseWindowWillBeClosed function (defined in
> browser/base/content/test/general/head.js), which should be equivalent to
> the BrowserTestUtils.domWindowClosed function.

Marco, I have submitted the patch. Could you please review it and let me know if anything is needed.

Thanks,
Sandeep

Comment 3

[Marco Castelluccio [:marco]]

Comment on attachment 8993232 [details]
Bug 1476137 - Remove promiseWindowWillBeClosed and use BrowserTestUtils.domWindowClosed

https://reviewboard.mozilla.org/r/258020/#review264960

Could you also remove the function usage and the function itself from browser/base/content/test/general/head.js?

Comment 4

[Sandeep Bypina [:sandeepb]]

Comment on attachment 8993232 [details]
Bug 1476137 - Remove promiseWindowWillBeClosed and use BrowserTestUtils.domWindowClosed

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

Comment 5

[Marco Castelluccio [:marco]]

Comment on attachment 8993232 [details]
Bug 1476137 - Remove promiseWindowWillBeClosed and use BrowserTestUtils.domWindowClosed

https://reviewboard.mozilla.org/r/258020/#review264982

::: browser/base/content/test/general/head.js:209
(Diff revision 2)
> -    }, "domwindowclosed");
> -  });
> -}
> -
>  function promiseWindowClosed(win) {
> -  let promise = promiseWindowWillBeClosed(win);
> +  let promise = BrowserTestUtils.domWindowClosed(win);

You will also need to import BrowserTestUtils in this file in order to use it (similarly to PlacesTestUtils).

Please make sure the tests keep working after your modifications. You can run them with this command:
./mach test browser/base/content/test/general/ --verify

Comment 6

[Sandeep Bypina [:sandeepb]]

(In reply to Marco Castelluccio [:marco] from comment #5)
> Comment on attachment 8993232 [details]
> Bug 1476137 - Remove promiseWindowWillBeClosed and use
> BrowserTestUtils.domWindowClosed
> 
> https://reviewboard.mozilla.org/r/258020/#review264982
> 
> ::: browser/base/content/test/general/head.js:209
> (Diff revision 2)
> > -    }, "domwindowclosed");
> > -  });
> > -}
> > -
> >  function promiseWindowClosed(win) {
> > -  let promise = promiseWindowWillBeClosed(win);
> > +  let promise = BrowserTestUtils.domWindowClosed(win);
> 
> You will also need to import BrowserTestUtils in this file in order to use
> it (similarly to PlacesTestUtils).
> 
> Please make sure the tests keep working after your modifications. You can
> run them with this command:
> ./mach test browser/base/content/test/general/ --verify

Sorry for the mistake. I will upload new patch in an hour.

Comment 7

[Sandeep Bypina [:sandeepb]]

Comment on attachment 8993232 [details]
Bug 1476137 - Remove promiseWindowWillBeClosed and use BrowserTestUtils.domWindowClosed

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

Comment 8

[Sandeep Bypina [:sandeepb]]

Comment on attachment 8993232 [details]
Bug 1476137 - Remove promiseWindowWillBeClosed and use BrowserTestUtils.domWindowClosed

https://reviewboard.mozilla.org/r/258020/#review264982

> You will also need to import BrowserTestUtils in this file in order to use it (similarly to PlacesTestUtils).
> 
> Please make sure the tests keep working after your modifications. You can run them with this command:
> ./mach test browser/base/content/test/general/ --verify

Marco, I imported BrowserTestUtils in head.js and ran the tests as you mentioned. The test results came as OK. Please let me know if i missed any. Please dont mind for the mistakes as this is my contribution to open source and mozilla.

Comment 9

[Marco Castelluccio [:marco]]

Comment on attachment 8993232 [details]
Bug 1476137 - Remove promiseWindowWillBeClosed and use BrowserTestUtils.domWindowClosed

https://reviewboard.mozilla.org/r/258020/#review265406

Looks good! Thanks for your contribution!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9fcf0dece71d
Remove promiseWindowWillBeClosed and use BrowserTestUtils.domWindowClosed r=marco

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/9fcf0dece71d