Closed Bug 731926 Opened 8 years ago Closed 8 years ago

Refactor init/shutdown functions in browser.js

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 16

People

(Reporter: Dolske, Assigned: Dolske)

References

(Blocks 2 open bugs)

Details

(Keywords: addon-compat)

Attachments

(7 files, 11 obsolete files)

48.52 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
61.70 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
11.01 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
10.95 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
5.92 KB, patch
Details | Diff | Splinter Review
6.92 KB, patch
Details | Diff | Splinter Review
9.20 KB, patch
Details | Diff | Splinter Review
Attached patch Patch v.0 (WIP) (obsolete) — Splinter Review
Ala bug 720294. Shift existing code into a gBrowserInit. Patch v.0 is just a change in intention and addition of legacy compat functions. Also, completely untested!
Attached patch Patch v.1, Part A (obsolete) — Splinter Review
Part A just moves HandleAppCommandEvent() further down in the file, and wraps the startup/shutdown functions into gBrowserInit.
Assignee: nobody → dolske
Attachment #601884 - Attachment is obsolete: true
Attachment #602803 - Flags: review?(gavin.sharp)
Attached patch Patch v.1, Part B (obsolete) — Splinter Review
Part B renames a couple functions, and inlines prepareForStartup() since its division seems highly arbitrary. In a future bug I'd like to tackle splitting this up in a more sensible way, but that's more risk prone so I don't want to do it here.
Attached patch Patch v.1, Part C (obsolete) — Splinter Review
Part C eliminates getWebNavigation() usage. This function dates back to the original browser.js 1.1 in CVS from Blake Ross. All the try/catch stuff seems unneeded now. [I kept the function just to avoid thinking about addon compat.]
Attachment #602806 - Flags: review?(gavin.sharp)
Attached patch Patch v.1, Part D (obsolete) — Splinter Review
Part D is just come cleanup I couldn't stop myself from doing.

  * Prefer usage of Cc/Ci/Cu
  * remove try/catch from |FullZoom| init/destroy, these function don't seem
    likely to throw (but if you'd rather be paranoid, I can just move the
    try/catch into the init/destroy functions themselves.)
  * minor whitespace / comment fixup
Attachment #602807 - Flags: review?(gavin.sharp)
Attachment #602804 - Flags: review?(gavin.sharp)
Hmm, I lost a couple trivial fixes somewhere along the way...

Part A:

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
...
>+    initializeSanitizer();

Oops: this.initializeSanitizer()


Part B:

>+++ b/browser/base/content/browser.xul
...
>+        onload="gBrowerInit.onLoad()" onunload="gBrowserInit.onUnload()" onclose="return WindowIsClosing();"

Oops: gBrowserInit.onLoad()
(In reply to Justin Dolske [:Dolske] from comment #5)
 
> Oops: this.initializeSanitizer()

Sigh. gBrowserInit.initializeSanitizer()

Test first, comment later.
Blocks: 732896
Comment on attachment 602803 [details] [diff] [review]
Patch v.1, Part A

This has the same flaw that bug 720294 had (bug 733339)
Attachment #602803 - Flags: review?(gavin.sharp) → review-
Comment on attachment 602806 [details] [diff] [review]
Patch v.1, Part C

> function URLBarSetURI(aURI) {
>   var value = gBrowser.userTypedValue;
>   var valid = false;
> 
>   if (value == null) {
>-    let uri = aURI || getWebNavigation().currentURI;
>+    let uri = aURI || gBrowser.webNavigation.currentURI;

gBrowser.currentURI

> function BrowserSetForcedCharacterSet(aCharset)
> {
>   gBrowser.docShell.charset = aCharset;
>   // Save the forced character-set
>-  PlacesUtils.history.setCharsetForURI(getWebNavigation().currentURI, aCharset);
>+  PlacesUtils.history.setCharsetForURI(gBrowser.webNavigation.currentURI, aCharset);

ditto
Comment on attachment 602807 [details] [diff] [review]
Patch v.1, Part D

> var gBrowserInit = {
>   onLoad: function () {
>-    var uriToLoad = null;
>-
>     // window.arguments[0]: URI to load (string), or an nsISupportsArray of
>     //                      nsISupportsStrings to load, or a xul:tab of
>     //                      a tabbrowser, which will be replaced by this
>     //                      window (for this case, all other arguments are
>     //                      ignored).
>     //                 [1]: character set (string)
>     //                 [2]: referrer (nsIURI)
>     //                 [3]: postData (nsIInputStream)
>     //                 [4]: allowThirdPartyFixup (bool)
>+    var uriToLoad = null;
>     if ("arguments" in window && window.arguments[0])
>       uriToLoad = window.arguments[0];

you could actually change this to:

    if ("arguments" in window && window.arguments[0])
      var uriToLoad = window.arguments[0];
OS: Mac OS X → All
Hardware: x86 → All
Comment on attachment 602804 [details] [diff] [review]
Patch v.1, Part B

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

>-        onload="BrowserStartup()" onunload="BrowserShutdown()" onclose="return WindowIsClosing();"
>+        onload="gBrowerInit.onLoad()" onunload="gBrowserInit.onUnload()" onclose="return WindowIsClosing();"

This will break people who monkeypatch these functions. Hope no one does that! Probably unlikely enough not to worry about.

inlining of prepareForStartup sounds fine, but it looks like this patch needs to be rebased anyways.
Attachment #602804 - Flags: review?(gavin.sharp) → feedback+
Comment on attachment 602806 [details] [diff] [review]
Patch v.1, Part C

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

> function BrowserStop()
> {

Can you fix this bracing while you're at it?

>+  const stopFlags = nsIWebNavigation.STOP_ALL;

Ci.nsIWebNavigation? Ideally the nsIWebNavigation const wouldn't exist, but maybe removing it now is more trouble than it's worth.

> function BrowserSetForcedCharacterSet(aCharset)

>+  PlacesUtils.history.setCharsetForURI(gBrowser.webNavigation.currentURI, aCharset);

gBrowser.currentURI
Attachment #602806 - Flags: review?(gavin.sharp) → review+
Attachment #602807 - Flags: review?(gavin.sharp) → review+
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #11)

> >+  const stopFlags = nsIWebNavigation.STOP_ALL;
> 
> Ci.nsIWebNavigation? Ideally the nsIWebNavigation const wouldn't exist, but
> maybe removing it now is more trouble than it's worth.

Blech. I'm going to leave this one as-is, the weird |nsIWebNavigation| is used all over, so we should just fix them all at once (in some other bug, natch).
Blocks: 740279
Attached patch Patch v.2, Part A (obsolete) — Splinter Review
Attachment #602803 - Attachment is obsolete: true
Attachment #611124 - Flags: review?(gavin.sharp)
Attached patch Patch v.2, Part B (obsolete) — Splinter Review
Attachment #602804 - Attachment is obsolete: true
Attachment #611125 - Flags: review?(gavin.sharp)
Attached patch Patch v.2, Part C (obsolete) — Splinter Review
Attachment #602806 - Attachment is obsolete: true
Attached patch Patch v.2, Part D (obsolete) — Splinter Review
Attachment #602807 - Attachment is obsolete: true
Attachment #611125 - Attachment is patch: true
Attachment #611126 - Attachment is patch: true
Comments on v.2 patches:

Part A is 99% just a move-and-indent change.
Part B is inlining prepareForStartup and renaming a couple things, along with some fixup to make |this| be as expected in the refactored code.
Part C and D are the same as last time plus review fixes.

Did a try push for paranoia's sake: https://tbpl.mozilla.org/?tree=Try&rev=9ffdc6665786
Attached patch Patch v.2, Part A (de-bitrot) (obsolete) — Splinter Review
Debitrotted. Verified with a before/after diff -w that this is just objectifying the code, adding the legacy global functions, and moving HandleAppCommandEvent().
Attachment #611124 - Attachment is obsolete: true
Attachment #615193 - Flags: review?(gavin.sharp)
Attachment #611124 - Flags: review?(gavin.sharp)
Attached patch Patch v.2, Part B (de-bitrot) (obsolete) — Splinter Review
Attachment #611125 - Attachment is obsolete: true
Attachment #615195 - Flags: review?(gavin.sharp)
Attachment #611125 - Flags: review?(gavin.sharp)
Attachment #615193 - Flags: review?(gavin.sharp) → review?(nobody)
There are add-ons that "patch" BrowserStartup and delayedStartup (I didn't check BrowserShutdown) using toString, which is going to fail with the gBrowserInit.foo.bind(gBrowserInit) pattern. All-in-One Sidebar even depends on prepareForStartup since it re-implements all of BrowserStartup...
Attachment #615193 - Flags: review?(nobody) → review?(gavin.sharp)
Talked with gavin about this a bit yesterday, I'm going to look at some of those addons (addon MXR) and see about doing some outreach, and see if there's a better way we can provide to help them do whatever they're trying to do. Since it's the end of the cycle and we're in approval-only mode anyway, will try to wrap up early in Fx15 to make sure things can adapt.

(*shudder* there are some awful hacks in those addons...)
I just finished doing an MXR search for addons that look possible to break, and sent the authors an email (1) notifying them of this change and (2) asking for suggestions on how to help them do things more robustly on our side.

The plan is to update this patch (de-bitrot, add any useful suggestions) and land at the beginning of the Firefox 16 cycle (shortly after June 5th).
Ok, new release cycle, here we go. De-bitrotted, and I split up a couple of them to make trivial code movements not cluttered up with other changes.

Part A -- Trivial move of HandleAppCommandEvent(), no other change
Part A-More -- Methodification of functions [func foo() --> foo: func()],
               indent everything as a result, add legacy globals. No code
               changes otherwise.
Part B -- Rename some functions (to onLoad, onUnload)
Part B-More -- Trivial move (inlining) of prepareForStartup(). No code changes.
Part C -- Eliminate getWebNavigation() usage
Part D -- Minor leanup (see comment 4)

Pushed to try, seems good so far...

https://tbpl.mozilla.org/?tree=Try&rev=1b3e7e9f46a5
Attachment #611126 - Attachment is obsolete: true
Attachment #611127 - Attachment is obsolete: true
Attachment #615193 - Attachment is obsolete: true
Attachment #615195 - Attachment is obsolete: true
Attachment #615193 - Flags: review?(gavin.sharp)
Attachment #615195 - Flags: review?(gavin.sharp)
Attachment #632495 - Flags: review?(gavin.sharp)
I don't know why "hg diff" produces such dumb output for Part A when it's actually just a small change. This is the output of "diff -U 8 oldfile newfile", which is much less scary looking. Also appended a diff -w version of Part B-More, showing that it's not much of a change other than whitespace.

Apologies for the Sergeish patch naming. :/
Attachment #632495 - Flags: review?(gavin.sharp) → review+
Attachment #632503 - Attachment description: Sane view of Part A and Part B-More → Sane view of Part A and Part A-More
Comment on attachment 632498 [details] [diff] [review]
Patch v.3, Part B

You could also rename nonBrowserWindowStartup -> nonBrowserWindowLoad and nonBrowserWindowShutdown -> nonBrowserWindowUnload
Attachment #632498 - Flags: review+
Keywords: addon-compat
You need to log in before you can comment on or make changes to this bug.