Remove promiseWindowWillBeClosed and use BrowserTestUtils.domWindowClosed

RESOLVED FIXED in Firefox 63

Status

()

P5
normal
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: marco, Assigned: sandeepb)

Tracking

({good-first-bug})

Trunk
Firefox 63
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 months ago
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.
Priority: -- → P5
Comment hidden (mozreview-request)
(Assignee)

Comment 2

7 months ago
(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
(Reporter)

Comment 3

7 months ago
mozreview-review
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 hidden (mozreview-request)
(Reporter)

Comment 5

7 months ago
mozreview-review
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
(Assignee)

Comment 6

7 months ago
(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 hidden (mozreview-request)
(Assignee)

Comment 8

7 months ago
mozreview-review-reply
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.
(Reporter)

Comment 9

7 months ago
mozreview-review
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!
Attachment #8993232 - Flags: review+

Comment 10

7 months ago
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9fcf0dece71d
Remove promiseWindowWillBeClosed and use BrowserTestUtils.domWindowClosed r=marco

Comment 11

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9fcf0dece71d
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox63: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Assignee: nobody → kumarsandeep2357
You need to log in before you can comment on or make changes to this bug.