Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Refactor init/shutdown functions in browser.js

RESOLVED FIXED in Firefox 16

Status

()

Firefox
General
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

(Blocks: 2 bugs, {addon-compat})

unspecified
Firefox 16
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 11 obsolete attachments)

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
(Assignee)

Description

6 years ago
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!
(Assignee)

Comment 1

6 years ago
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.
Assignee: nobody → dolske
Attachment #601884 - Attachment is obsolete: true
Attachment #602803 - Flags: review?(gavin.sharp)
(Assignee)

Comment 2

6 years ago
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.
(Assignee)

Comment 3

6 years ago
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.]
Attachment #602806 - Flags: review?(gavin.sharp)
(Assignee)

Comment 4

6 years ago
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
Attachment #602807 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
Attachment #602804 - Flags: review?(gavin.sharp)
(Assignee)

Comment 5

6 years ago
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()
(Assignee)

Comment 6

6 years ago
(In reply to Justin Dolske [:Dolske] from comment #5)
 
> Oops: this.initializeSanitizer()

Sigh. gBrowserInit.initializeSanitizer()

Test first, comment later.
(Assignee)

Updated

6 years ago
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];

Updated

5 years ago
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+
(Assignee)

Comment 12

5 years ago
(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).
(Assignee)

Updated

5 years ago
Blocks: 740279
(Assignee)

Comment 13

5 years ago
Created attachment 611124 [details] [diff] [review]
Patch v.2, Part A
Attachment #602803 - Attachment is obsolete: true
Attachment #611124 - Flags: review?(gavin.sharp)
(Assignee)

Comment 14

5 years ago
Created attachment 611125 [details] [diff] [review]
Patch v.2, Part B
Attachment #602804 - Attachment is obsolete: true
Attachment #611125 - Flags: review?(gavin.sharp)
(Assignee)

Comment 15

5 years ago
Created attachment 611126 [details] [diff] [review]
Patch v.2, Part C
Attachment #602806 - Attachment is obsolete: true
(Assignee)

Comment 16

5 years ago
Created attachment 611127 [details] [diff] [review]
Patch v.2, Part D
Attachment #602807 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #611125 - Attachment is patch: true
(Assignee)

Updated

5 years ago
Attachment #611126 - Attachment is patch: true
(Assignee)

Comment 17

5 years ago
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
(Assignee)

Comment 18

5 years ago
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().
Attachment #611124 - Attachment is obsolete: true
Attachment #615193 - Flags: review?(gavin.sharp)
Attachment #611124 - Flags: review?(gavin.sharp)
(Assignee)

Comment 19

5 years ago
Created attachment 615195 [details] [diff] [review]
Patch v.2, Part B (de-bitrot)
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...
(Assignee)

Updated

5 years ago
Attachment #615193 - Flags: review?(nobody) → review?(gavin.sharp)
(Assignee)

Comment 21

5 years ago
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...)
(Assignee)

Comment 22

5 years ago
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).
(Assignee)

Comment 23

5 years ago
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
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)
(Assignee)

Comment 24

5 years ago
Created attachment 632497 [details] [diff] [review]
Patch v.3, Part A-More
(Assignee)

Comment 25

5 years ago
Created attachment 632498 [details] [diff] [review]
Patch v.3, Part B
(Assignee)

Comment 26

5 years ago
Created attachment 632500 [details] [diff] [review]
Patch v.3, Part B-More
(Assignee)

Comment 27

5 years ago
Created attachment 632501 [details] [diff] [review]
Patch v.3, Part C
(Assignee)

Comment 28

5 years ago
Created attachment 632502 [details] [diff] [review]
Patch v.3, Part D
(Assignee)

Comment 29

5 years ago
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. :/
Attachment #632495 - Flags: review?(gavin.sharp) → review+
Attachment #632497 - Flags: 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+
Attachment #632500 - Flags: review+
(Assignee)

Comment 31

5 years ago
Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec2383f5e4b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/055b05c2a56f
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bbf0325b940
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed5376e06fc5
https://hg.mozilla.org/integration/mozilla-inbound/rev/f86f39aa9ee8
https://hg.mozilla.org/integration/mozilla-inbound/rev/4320698fc291
Target Milestone: --- → Firefox 16
https://hg.mozilla.org/mozilla-central/rev/ec2383f5e4b3
https://hg.mozilla.org/mozilla-central/rev/055b05c2a56f
https://hg.mozilla.org/mozilla-central/rev/3bbf0325b940
https://hg.mozilla.org/mozilla-central/rev/ed5376e06fc5
https://hg.mozilla.org/mozilla-central/rev/f86f39aa9ee8
https://hg.mozilla.org/mozilla-central/rev/4320698fc291
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Keywords: addon-compat
You need to log in before you can comment on or make changes to this bug.