713713 - Clean up OpenBrowserWindow

Status: RESOLVED FIXED

(Firefox :: General, task, P2)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I was poking at this elsewhere and it's all repeaty so I cleaned it up.

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1

I'm not actually sure this is right (will the empty string be handled the same as no argument?). If not, I guess I could just not instantiate charsetArg so it will be undefined and that should be exactly the same as not passing anything.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 584466 [details] [diff] [review]
Patch v0.1

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

> function OpenBrowserWindow()
> {
>-  var charsetArg = new String();
>+  var charsetArg = "";

might as well also move this next to its first use

>+  return window.openDialog("chrome://browser/content/", "_blank",
>+                           "chrome,all,dialog=no", defaultArgs, charsetArg);

This will change window.arguments.length, but the existing checks for this (which are ancient and kind of ugly) should handle this OK, since they null check arguments[1], and nothing else cares about window.arguments.length == 1 vs. 2.

http://hg.mozilla.org/mozilla-central/annotate/d85920b5691b/browser/base/content/browser.js#l1453

You could write a test to ensure this, though!

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.2

Now with a test, at least I think it's testing what it's supposed to...

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 595445 [details] [diff] [review]
Patch v0.2

BrowserSetForcedCharacterSet causes a page reload, so it's probably best to have this test open a new tab first and close it when it's done, rather than operating on the current tab. It might be useful to also test that OpenBrowserWindow still works when invoked from a non-browser window.

Comment 5

[Dão Gottwald [:dao]]

Attachment: Bug 713713 - Expand BrowserWindowTracker.openWindow to cover the use cases of OpenBrowserWindow, and remove BrowserUIUtils.openNewBrowserWindow. r=mossop

Comment 6

[Dão Gottwald [:dao]]

Resurrecting this bug as OpenBrowserWindow looks like it still could use some cleanup, and we also have too many similar functions with slightly different behavior.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e9251e249c27
Expand BrowserWindowTracker.openWindow to cover the use cases of OpenBrowserWindow, and remove BrowserUIUtils.openNewBrowserWindow. r=mossop

Comment 8

[Cosmin Sabou [:CosminS]]

Backed out for causing leaked windows of different tests.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel&revision=e9251e249c27a9f695341d5ba28a9a04e3ebd6a7&selectedTaskRun=FscOgCoRS9Om9F3hu7fkWQ.0

Failure logs:

• https://treeherder.mozilla.org/logviewer?job_id=421628261&repo=autoland
• https://treeherder.mozilla.org/logviewer?job_id=421625136&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/724b02b57c1a56ec1d643ef69c8a2e6a3f4c4f56

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a09e586f7b7f
Expand BrowserWindowTracker.openWindow to cover the use cases of OpenBrowserWindow, and remove BrowserUIUtils.openNewBrowserWindow. r=mossop

Comment 10

[Sandor Molnar[:smolnar]]

Backed out for causing bc failures in browser/modules/test/browser/browser_BrowserWindowTracker.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/25063720f5a9ddfd911566022a2bde4438d69e2e

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | browser/modules/test/browser/browser_BrowserWindowTracker.js | Uncaught exception in test bound test_pendingWindows

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e72d614ce773
Expand BrowserWindowTracker.openWindow to cover the use cases of OpenBrowserWindow, and remove BrowserUIUtils.openNewBrowserWindow. r=mossop

Comment 12

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/e72d614ce773

Comment 13

[Ryan VanderMeulen [:RyanVM]]

This was backed out for 117.0.1 due to bug 1850828. It remains in effect for 118+.
https://hg.mozilla.org/releases/mozilla-release/rev/1095e192521fc2ae558176d1881a6eee935ccc41

Comment 14

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)

This was backed out for 117.0.1 due to bug 1850828. It remains in effect for 118+.
https://hg.mozilla.org/releases/mozilla-release/rev/1095e192521fc2ae558176d1881a6eee935ccc41

This is hitting test failures in browser_search_history.js. I'm going to re-land this and uplift the patch from bug 1850828 instead.

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/749861239884