Last Comment Bug 731926 - Refactor init/shutdown functions in browser.js
: Refactor init/shutdown functions in browser.js
Status: RESOLVED FIXED
: addon-compat
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 16
Assigned To: Justin Dolske [:Dolske]
:
Mentors:
Depends on:
Blocks: 732896 740279
  Show dependency treegraph
 
Reported: 2012-02-29 23:05 PST by Justin Dolske [:Dolske]
Modified: 2012-06-18 00:59 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v.0 (WIP) (63.18 KB, patch)
2012-02-29 23:05 PST, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.1, Part A (62.14 KB, patch)
2012-03-04 21:06 PST, Justin Dolske [:Dolske]
dao+bmo: review-
Details | Diff | Splinter Review
Patch v.1, Part B (12.42 KB, patch)
2012-03-04 21:09 PST, Justin Dolske [:Dolske]
gavin.sharp: feedback+
Details | Diff | Splinter Review
Patch v.1, Part C (5.89 KB, patch)
2012-03-04 21:13 PST, Justin Dolske [:Dolske]
gavin.sharp: review+
Details | Diff | Splinter Review
Patch v.1, Part D (7.07 KB, patch)
2012-03-04 21:17 PST, Justin Dolske [:Dolske]
gavin.sharp: review+
Details | Diff | Splinter Review
Patch v.2, Part A (58.51 KB, patch)
2012-03-30 21:36 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.2, Part B (19.72 KB, patch)
2012-03-30 21:37 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.2, Part C (5.92 KB, patch)
2012-03-30 21:37 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.2, Part D (7.25 KB, patch)
2012-03-30 21:38 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.2, Part A (de-bitrot) (58.66 KB, patch)
2012-04-15 15:19 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.2, Part B (de-bitrot) (19.86 KB, patch)
2012-04-15 15:20 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.3, Part A (48.52 KB, patch)
2012-06-12 18:42 PDT, Justin Dolske [:Dolske]
gavin.sharp: review+
Details | Diff | Splinter Review
Patch v.3, Part A-More (61.70 KB, patch)
2012-06-12 18:42 PDT, Justin Dolske [:Dolske]
gavin.sharp: review+
Details | Diff | Splinter Review
Patch v.3, Part B (11.01 KB, patch)
2012-06-12 18:43 PDT, Justin Dolske [:Dolske]
gavin.sharp: review+
Details | Diff | Splinter Review
Patch v.3, Part B-More (10.95 KB, patch)
2012-06-12 18:43 PDT, Justin Dolske [:Dolske]
gavin.sharp: review+
Details | Diff | Splinter Review
Patch v.3, Part C (5.92 KB, patch)
2012-06-12 18:44 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.3, Part D (6.92 KB, patch)
2012-06-12 18:44 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Sane view of Part A and Part A-More (9.20 KB, patch)
2012-06-12 18:55 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review

Description Justin Dolske [:Dolske] 2012-02-29 23:05:35 PST
Created attachment 601884 [details] [diff] [review]
Patch v.0 (WIP)

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!
Comment 1 Justin Dolske [:Dolske] 2012-03-04 21:06:31 PST
Created attachment 602803 [details] [diff] [review]
Patch v.1, Part A

Part A just moves HandleAppCommandEvent() further down in the file, and wraps the startup/shutdown functions into gBrowserInit.
Comment 2 Justin Dolske [:Dolske] 2012-03-04 21:09:19 PST
Created attachment 602804 [details] [diff] [review]
Patch v.1, Part B

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.
Comment 3 Justin Dolske [:Dolske] 2012-03-04 21:13:26 PST
Created attachment 602806 [details] [diff] [review]
Patch v.1, Part C

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.]
Comment 4 Justin Dolske [:Dolske] 2012-03-04 21:17:42 PST
Created attachment 602807 [details] [diff] [review]
Patch v.1, Part D

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
Comment 5 Justin Dolske [:Dolske] 2012-03-04 23:58:26 PST
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()
Comment 6 Justin Dolske [:Dolske] 2012-03-05 00:02:41 PST
(In reply to Justin Dolske [:Dolske] from comment #5)
 
> Oops: this.initializeSanitizer()

Sigh. gBrowserInit.initializeSanitizer()

Test first, comment later.
Comment 7 Dão Gottwald [:dao] 2012-03-06 04:49:30 PST
Comment on attachment 602803 [details] [diff] [review]
Patch v.1, Part A

This has the same flaw that bug 720294 had (bug 733339)
Comment 8 Dão Gottwald [:dao] 2012-03-06 04:50:42 PST
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 9 Dão Gottwald [:dao] 2012-03-06 04:52:09 PST
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];
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-22 17:46:57 PDT
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.
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-22 18:00:04 PDT
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
Comment 12 Justin Dolske [:Dolske] 2012-03-28 23:38:22 PDT
(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).
Comment 13 Justin Dolske [:Dolske] 2012-03-30 21:36:31 PDT
Created attachment 611124 [details] [diff] [review]
Patch v.2, Part A
Comment 14 Justin Dolske [:Dolske] 2012-03-30 21:37:18 PDT
Created attachment 611125 [details] [diff] [review]
Patch v.2, Part B
Comment 15 Justin Dolske [:Dolske] 2012-03-30 21:37:42 PDT
Created attachment 611126 [details] [diff] [review]
Patch v.2, Part C
Comment 16 Justin Dolske [:Dolske] 2012-03-30 21:38:14 PDT
Created attachment 611127 [details] [diff] [review]
Patch v.2, Part D
Comment 17 Justin Dolske [:Dolske] 2012-03-30 21:49:11 PDT
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
Comment 18 Justin Dolske [:Dolske] 2012-04-15 15:19:14 PDT
Created attachment 615193 [details] [diff] [review]
Patch v.2, Part A (de-bitrot)

Debitrotted. Verified with a before/after diff -w that this is just objectifying the code, adding the legacy global functions, and moving HandleAppCommandEvent().
Comment 19 Justin Dolske [:Dolske] 2012-04-15 15:20:03 PDT
Created attachment 615195 [details] [diff] [review]
Patch v.2, Part B (de-bitrot)
Comment 20 Dão Gottwald [:dao] 2012-04-16 00:53:04 PDT
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...
Comment 21 Justin Dolske [:Dolske] 2012-04-18 15:48:23 PDT
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...)
Comment 22 Justin Dolske [:Dolske] 2012-05-16 15:05:36 PDT
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).
Comment 23 Justin Dolske [:Dolske] 2012-06-12 18:42:35 PDT
Created attachment 632495 [details] [diff] [review]
Patch v.3, Part A

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
Comment 24 Justin Dolske [:Dolske] 2012-06-12 18:42:55 PDT
Created attachment 632497 [details] [diff] [review]
Patch v.3, Part A-More
Comment 25 Justin Dolske [:Dolske] 2012-06-12 18:43:31 PDT
Created attachment 632498 [details] [diff] [review]
Patch v.3, Part B
Comment 26 Justin Dolske [:Dolske] 2012-06-12 18:43:54 PDT
Created attachment 632500 [details] [diff] [review]
Patch v.3, Part B-More
Comment 27 Justin Dolske [:Dolske] 2012-06-12 18:44:17 PDT
Created attachment 632501 [details] [diff] [review]
Patch v.3, Part C
Comment 28 Justin Dolske [:Dolske] 2012-06-12 18:44:43 PDT
Created attachment 632502 [details] [diff] [review]
Patch v.3, Part D
Comment 29 Justin Dolske [:Dolske] 2012-06-12 18:55:11 PDT
Created attachment 632503 [details] [diff] [review]
Sane view of Part A and Part A-More

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. :/
Comment 30 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-13 11:25:08 PDT
Comment on attachment 632498 [details] [diff] [review]
Patch v.3, Part B

You could also rename nonBrowserWindowStartup -> nonBrowserWindowLoad and nonBrowserWindowShutdown -> nonBrowserWindowUnload

Note You need to log in before you can comment on or make changes to this bug.