449010 - In browser.js, don't prototype single-instance objects

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch

Comment 3

[Dão Gottwald [:dao]]

Attachment: updated to trunk

Comment 4

[Dão Gottwald [:dao]]

Attachment: updated to trunk

Comment 5

[Dão Gottwald [:dao]]

Attachment: updated to trunk

Comment 6

[Dão Gottwald [:dao]]

Attachment: updated to trunk

Comment 7

[Asaf Romano (gone)]

Comment on attachment 404823 [details] [diff] [review]
updated to trunk

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -70,23 +70,18 @@ Cu.import("resource://gre/modules/XPCOMU
> const nsIWebNavigation = Ci.nsIWebNavigation;
> 
> var gCharsetMenu = null;
> var gLastBrowserCharset = null;
> var gPrevCharset = null;
> var gProxyFavIcon = null;
> var gLastValidURLStr = "";
> var gInPrintPreviewMode = false;
>-let gDownloadMgr = null;
>-
>-// Global variable that holds the nsContextMenu instance.
>-var gContextMenu = null;
>-
>-var gAutoHideTabbarPrefListener = null;
>-var gBookmarkAllTabsHandler = null;
>+var gDownloadMgr = null;

Why did you change let back to var? I heard it's preferable even in the global scope. IIRC sdwilsh knows why.
 
>+var gContextMenu = null; // nsContextMenu instance
> 
> #ifndef XP_MACOSX
> var gEditUIVisible = true;
> #endif
> 
> [
>   ["gBrowser",            "content"],
>   ["gNavToolbox",         "navigator-toolbox"],
>@@ -1117,18 +1112,17 @@ function prepareForStartup() {
>   // and give C++ access to gBrowser
>   XULBrowserWindow.init();
>   window.QueryInterface(Ci.nsIInterfaceRequestor)
>         .getInterface(nsIWebNavigation)
>         .QueryInterface(Ci.nsIDocShellTreeItem).treeOwner
>         .QueryInterface(Ci.nsIInterfaceRequestor)
>         .getInterface(Ci.nsIXULWindow)
>         .XULBrowserWindow = window.XULBrowserWindow;
>-  window.QueryInterface(Ci.nsIDOMChromeWindow).browserDOMWindow =
>-    new nsBrowserAccess();
>+  window.QueryInterface(Ci.nsIDOMChromeWindow);
> 

comment above the browserDOMWindow object, that it implements the property from nsIDOMChromeWindow, and reference that comment here.

r=mano otherwise.

Comment 8

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

Comment on attachment 404823 [details] [diff] [review]
updated to trunk

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

>-function nsBrowserAccess()

>+/* nsIDOMChromeWindow */
>+var browserDOMWindow = {

I think this will have a negative compatibility impact - several addons attempt to modify nsBrowserAccess.prototype directly (particularly openURI), per http://mxr-test.konigsberg.mozilla.org/addons/search?string=nsBrowserAccess . I think you should omit this change.

>+var gMissingPluginInstaller = {

Seems like you should probably fix the indentation here.

r=me with those addressed.

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #7)
> >-let gDownloadMgr = null;
> >-
> >-// Global variable that holds the nsContextMenu instance.
> >-var gContextMenu = null;
> >-
> >-var gAutoHideTabbarPrefListener = null;
> >-var gBookmarkAllTabsHandler = null;
> >+var gDownloadMgr = null;
> 
> Why did you change let back to var? I heard it's preferable even in the global
> scope. IIRC sdwilsh knows why.

It used to make a difference regarding window.gDownloadMgr, but it doesn't anymore.

Comment 10

[Dão Gottwald [:dao]]

(In reply to comment #8)
> (From update of attachment 404823 [details] [diff] [review])
> >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
> 
> >-function nsBrowserAccess()
> 
> >+/* nsIDOMChromeWindow */
> >+var browserDOMWindow = {
> 
> I think this will have a negative compatibility impact - several addons attempt
> to modify nsBrowserAccess.prototype directly (particularly openURI), per
> http://mxr-test.konigsberg.mozilla.org/addons/search?string=nsBrowserAccess . I
> think you should omit this change.

Most of these extensions do nsBrowserAccess.prototype.openURI.toString().replace( ... They'll generally break whenever we touch that function (which we have recently).

> >+var gMissingPluginInstaller = {
> 
> Seems like you should probably fix the indentation here.

Well, I didn't want to make blame completely useless.

Comment 11

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

(In reply to comment #10)
> Most of these extensions do
> nsBrowserAccess.prototype.openURI.toString().replace( ... They'll generally
> break whenever we touch that function (which we have recently).

Not all do that, and they won't always break when we modify it. There's very little cost to keeping it as is, and there is very little gain from changing it.

> Well, I didn't want to make blame completely useless.

I don't think whitespace fixes "make blame useless" - at worst they introduce an additional step in a process that usually already takes several steps. Not worth avoiding it at the cost of dealing with the code being incorrectly indented perpetually.

Comment 12

[Dão Gottwald [:dao]]

I just found out that browserDOMWindow.openURI isn't writable and toString() returns "function openURI() { [native code] }", so the prototyping is needed as an extension hook.

Comment 13

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/c2e54fc14cd7

Comment 14

[Asaf Romano (gone)]

Couldn't they use window.wrappedJSObject?

Comment 15

[Asaf Romano (gone)]

And, FWIW, I don't think this is .replace(...) is something we should encourage or pretend to support, at all.

Comment 16

[Dão Gottwald [:dao]]

Yeah, toString().replace() is ugly, but a cleaner way to inject custom code wouldn't have worked either without the property being writable. wrappedJSObject doesn't seem to help.

Comment 17

[Benjamin Smedberg]

I backported only the gMissingPluginInstaller portions of this patch to Lorentz to make transplanting the rest of the crashed-plugin UI less awful:

http://hg.mozilla.org/projects/firefox-lorentz/rev/9ae87295ef3b

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 19

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430