Make BrowserTestUtils.openNewBrowserWindow() delegate to window.OpenBrowserWindow

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P1
normal
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

Trunk
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

10 months ago
This will:

- reduce code duplication (the current implementation does much the same thing as OpenBrowserWindow)
- increase test coverage of new window opening + loading of about:newtab/about:home
- facilitate starting to make decisions about the initial URL passed to new browser windows without having to have special cases for tests (see bug 1362774 comment 150 and further)
- reduce the number of paths we have that manually open browser.xul and that we therefore have to update/audit if we try to change some of the window arguments handling (e.g. bug 1485961)

The main obvious issues were with callsites passing positioning information. I rewrote those successfully already. With those, I have an initial trypush at https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee9de926da36f8c3017386f14e18ac8e404c5f2c&selectedJob=195811711 . There are less than 5 remaining tests with issues, and I hope to have a patch later this week. Nihanth is helping with the captive portal issue.
Priority: -- → P1
Comment on attachment 9004614 [details]
Bug 1486824 - change BrowserTestUtils to just call OpenBrowserWindow so it does the same thing as opening a window normally, r?nhnt11,mconley

Nihanth Subramanya [:nhnt11] has approved the revision.
Attachment #9004614 - Flags: review+
Assignee

Comment 4

10 months ago
Looks like browser/base/content/test/plugins/browser_private_clicktoplay.js needs fixing, too.
Assignee

Comment 5

10 months ago
(In reply to :Gijs (he/him) from comment #4)
> Looks like browser/base/content/test/plugins/browser_private_clicktoplay.js
> needs fixing, too.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d694a6a8ab3c6fe71c4a19cdb4c4170786b1f881 looks green, with the remaining orange apparently caused by bug 1483325 and some other intermittents.
Comment on attachment 9004614 [details]
Bug 1486824 - change BrowserTestUtils to just call OpenBrowserWindow so it does the same thing as opening a window normally, r?nhnt11,mconley

Mike Conley (:mconley) (:⚙️) has approved the revision.
Attachment #9004614 - Flags: review+

Comment 7

10 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4e195f7b2d6b
change BrowserTestUtils to just call OpenBrowserWindow so it does the same thing as opening a window normally, r=mconley,nhnt11

Comment 8

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4e195f7b2d6b
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Assignee

Updated

10 months ago
Depends on: 1396196
Backed out for clipboard failures on OSX browser_editcontrols_update.

Backout link: https://hg.mozilla.org/mozilla-central/rev/1bd0f0baab41d587e900da8797467801d71568a5
Push link: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4e195f7b2d6b8c12aba04ef69401e9af65daa05a&filter-resultStatus=testfailed
Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=196632547&repo=mozilla-central&lineNumber=1604

This push might also affect some bc failures like "browser_privatebrowsing_about.js | Uncaught exception - TypeError: content.document.getElementById(...) is null, can't access property "checked" of it "
Log link : https://treeherder.mozilla.org/logviewer.html#?job_id=196638260&repo=autoland&lineNumber=3769
Flags: needinfo?(gijskruitbosch+bugs)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 63 → ---
Assignee

Updated

10 months ago
No longer depends on: 1396196
Assignee

Updated

10 months ago
Flags: needinfo?(gijskruitbosch+bugs)

Comment 10

10 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/96b5a88ac004
change BrowserTestUtils to just call OpenBrowserWindow so it does the same thing as opening a window normally, r=mconley,nhnt11

Comment 11

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/96b5a88ac004
Status: REOPENED → RESOLVED
Closed: 10 months ago10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63

Comment 13

10 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4b770468984
change BrowserTestUtils to just call OpenBrowserWindow so it does the same thing as opening a window normally, r=mconley,nhnt11

Comment 14

10 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c995c3a15034
review comment follow-up: change params for openNewBrowserWindow to explicitly check for URL, r=nhnt11
Assignee

Comment 15

10 months ago
(In reply to Pulsebot from comment #14)
> Pushed by gijskruitbosch@gmail.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c995c3a15034
> review comment follow-up: change params for openNewBrowserWindow to
> explicitly check for URL, r=nhnt11

Apologies for the separate commits - but in essence, I changed the tests some more here to explicitly wait for about:privatebrowsing as part of waiting for the initial window to open. In my trypush, even after repeated retriggers, I could no longer reproduce the orange that got me backed out last week, so let's hope it sticks this time...
Flags: needinfo?(gijskruitbosch+bugs)

Comment 16

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c4b770468984
https://hg.mozilla.org/mozilla-central/rev/c995c3a15034
Status: REOPENED → RESOLVED
Closed: 10 months ago10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.