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 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.

(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.

(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.

(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.

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.

Couldn't they use window.wrappedJSObject?

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

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.

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

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

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