1486824 - Make BrowserTestUtils.openNewBrowserWindow() delegate to window.OpenBrowserWindow

Status: RESOLVED FIXED

Description

[:Gijs (he/him)]

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.

Comment 1

[:Gijs (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd78d10442e9d114ae93c9c356248dd7b73f56b3

Comment 2

[:Gijs (he/him)]

Created 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

Comment 3

[Nihanth Subramanya [:nhnt11]]

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.

Comment 4

[:Gijs (he/him)]

Looks like browser/base/content/test/plugins/browser_private_clicktoplay.js needs fixing, too.

Comment 5

[:Gijs (he/him)]

(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 6

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/4e195f7b2d6b

Comment 9

[Narcis Beleuzu [:NarcisB]]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

[Raul Gurzau (:RaulGurzau)]

https://hg.mozilla.org/mozilla-central/rev/96b5a88ac004

Comment 12

[Eliza Balazs [:ebalazs_]]

Backed out changeset 96b5a88ac004 (Bug 1486824) for causing frequent failures e.g: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_about.js a=backout

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=96b5a88ac0041a1bd382a731749e1fca9aa911ed&filter-searchStr=Linux%20x64%20debug%20Mochitests%20with%20e10s%20test-linux64%2Fdebug-mochitest-browser-chrome-e10s-11%20M-e10s(bc11)&selectedJob=196787659

Failure logs:
https://treeherder.mozilla.org/logviewer.html#?job_id=196787659&repo=autoland&lineNumber=4027

https://treeherder.mozilla.org/logviewer.html#?job_id=196840060&repo=autoland&lineNumber=3676

Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=63d2134f95878cabfb89c7849a1baad2ef139fd0&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c995c3a15034
review comment follow-up: change params for openNewBrowserWindow to explicitly check for URL, r=nhnt11

Comment 15

[:Gijs (he/him)]

(In reply to Pulsebot from comment #14)
> Pushed by [e-mail]:
> 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...

Comment 16

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/c4b770468984
https://hg.mozilla.org/mozilla-central/rev/c995c3a15034