506111 - consolidate getService calls in browser.js

Status: RESOLVED DUPLICATE of bug 512784

(Firefox :: General, defect)

Description

[Dietrich Ayala (:dietrich)]

right now there are a bunch of redundant instantiations of observer service, io service, and others. we should instead load them each only once, and lazily at that.

eg, something like this: http://hg.mozilla.org/labs/weave/file/42d1239d8262/source/modules/util.js#l274

Comment 1

[(no longer active)]

Attachment: Patch (v1)

I tried to catch all services instantiated more than three times.

Comment 2

[Dão Gottwald [:dao]]

How about:

> [
>   ["gPrefService",       "preferences-service;1", "nsIPrefBranch2"],
>   ["gPermissionManager", "permissionmanager;1",   "nsIPermissionManager"],
>   ...
> ].forEach(function (service) {
>   var [name, cid, iface] = service;
>   window.__defineGetter__(name, function () {
>     delete window[name];
>     return window[name] = Cc["@mozilla.org/" + cid].getService(Ci[iface]);
>   });
> });

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 390747 [details] [diff] [review]
Patch (v1)

>+      if (gPermissionManager.testPermission(uri, "popup") == this._kIPM.ALLOW_ACTION) {

remove _kIPM from gPopupBlockerObserver, use gPermissionManager instead

>+      var rv = gPromptService.confirmEx(window, promptTitle, promptMessage,
>+                                        Ci.nsIPromptService.STD_YES_NO_BUTTONS,
>+                                        null, null, null, checkboxLabel, checkEveryTime);

gPromptService instead of Ci.nsIPromptService

>     } else {
>-      var ss = Cc["@mozilla.org/browser/search-service;1"].
>-               getService(Ci.nsIBrowserSearchService);
>-      var searchForm = ss.defaultEngine.searchForm;
>+      var searchForm = gSearchService.defaultEngine.searchForm;
>       loadURI(searchForm, null, null, false);
>     }
>   },

use let instead of var

>     try {
>-      var ios = Cc["@mozilla.org/network/io-service;1"].
>-                getService(Ci.nsIIOService);
>-
>-      var contentURI = ios.newURI(aWindow.location.href, null, null);
>-      return ios.newURI(attr, aWindow.document.characterSet, contentURI);
>+      var contentURI = gIOService.newURI(aWindow.location.href, null, null);
>+      return gIOService.newURI(attr, aWindow.document.characterSet, contentURI);

ditto

> function undoCloseWindow(aIndex) {
>-  let ss = Cc["@mozilla.org/browser/sessionstore;1"].
>-           getService(Ci.nsISessionStore);
>   let window = null;
>-  if (ss.getClosedWindowCount() > (aIndex || 0))
>-    window = ss.undoCloseWindow(aIndex || 0);
>+  if (gSessionStoreService.getClosedWindowCount() > (aIndex || 0))
>+    window = gSessionStoreService.undoCloseWindow(aIndex || 0);
> 
>   return window;
> }

please rename the "window" variable to something sane

You're using Components.classes and Components.interfaces a bunch of times. Use Cc and Ci instead.

Did you take browser-places.js, browser-tabPreviews.js and browser-textZoom.js into account?

Comment 4

[Dão Gottwald [:dao]]

I'm also not sure that gFooService is the best naming scheme for this. Maybe FooService without the g is more appropriate, except for gPrefService.

Comment 5

[(no longer active)]

Attachment: Patch (v2)

Addressed all review comments.

Comment 6

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

(In reply to comment #4)
> I'm also not sure that gFooService is the best naming scheme for this. Maybe
> FooService without the g is more appropriate, except for gPrefService.

I think a common prefix for these global accessors is useful (to make them easy to identify as browser.js-scope globals). It may have been useful to distinguish the element references from the service references, but we already have gPrefService as a constraint, so I think I would favor keeping the "g" prefix here.

Comment 7

[(no longer active)]

(In reply to comment #6)
> (In reply to comment #4)
> > I'm also not sure that gFooService is the best naming scheme for this. Maybe
> > FooService without the g is more appropriate, except for gPrefService.
> 
> I think a common prefix for these global accessors is useful (to make them easy
> to identify as browser.js-scope globals). It may have been useful to
> distinguish the element references from the service references, but we already
> have gPrefService as a constraint, so I think I would favor keeping the "g"
> prefix here.

I'm fine either way, but I tend to lean a bit on the side of having the "g" prefix.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 391033 [details] [diff] [review]
Patch (v2)

Fine by me, let's use the g prefix.

Nit: No need to replace let with var if you're not in a nested block, especially if you'd be introducing inconsistency because var is used a few lines above.

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #8)
> Nit: No need to replace let with var if you're not in a nested block,
> especially if you'd be introducing inconsistency because var is used a few
> lines above.

Quite obviously, I meant "replace var with let"...

Comment 10

[(no longer active)]

Attachment: Patch (v3)

Use the "g" prefix, and addressed the nit.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 391049 [details] [diff] [review]
Patch (v3)

>+  ["gPrefService",         "preferences-service;1",      "nsIPrefBranch2"],
>+  ["gPermissionManager",   "permissionmanager;1",        "nsIPermissionManager"],
>+  ["gObserverService",     "observer-service;1",         "nsIObserverService"],
>+  ["gWindowMediator",      "appshell/window-mediator;1", "nsIWindowMediator"],
>+  ["gPromptService",       "embedcomp/prompt-service;1", "nsIPromptService"],
>+  ["gSearchService",       "browser/search-service;1",   "nsIBrowserSearchService"],
>+  ["gSessionStoreService", "browser/sessionstore;1",     "nsISessionStore"],
>+  ["gIOService",           "network/io-service;1",       "nsIIOService2"],

Calling gSessionStoreService gSessionStore would be more consistent, I think.
Should we use ContentAreaUtils.ioService rather than adding gIOService?

r=me either way, with this nit:

>+    gPermissionManager.add(aDocument.documentURIObject, "offline-app",
>+                           Ci.nsIPermissionManager.DENY_ACTION);

gPermissionManager.DENY_ACTION

Comment 12

[John P Baker]

(In reply to comment #11)
> (From update of attachment 391049 [details] [diff] [review])

> Should we use ContentAreaUtils.ioService rather than adding gIOService?

That is _currently_ nsIIOService - manageOfflineStatus is in nsIIOService2

Comment 13

[(no longer active)]

(In reply to comment #11)
> Calling gSessionStoreService gSessionStore would be more consistent, I think.
> Should we use ContentAreaUtils.ioService rather than adding gIOService?

gIOService is an nsIIOService2.  Is it OK to modify ContentAreaUtils.ioService to be an nsIObserver2?

Comment 14

[Dão Gottwald [:dao]]

There's no reason not to make it nsIObserver2, at least if it's intended for external use, which seems to be the case.

Comment 15

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

You both mean nsIIOService2 rather than nsIObserver2, I'm guessing?

Comment 16

[(no longer active)]

(In reply to comment #15)
> You both mean nsIIOService2 rather than nsIObserver2, I'm guessing?

Yes.

/me wishes bugzilla supported comment edits... :-)

Comment 17

[(no longer active)]

Attachment: For check-in

Comment 18

[Dão Gottwald [:dao]]

Gavin writes:

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

>   // XXX: duplicated in preferences/advanced.js
>   _getOfflineAppUsage: function (host, groups)

We avoided touching this function in the other bug because it is duplicated. We
should get a bug on file on fixing that and avoid making this change too, I
think.

Comment 19

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/27b53ca1b085

Comment 20

[Ed Lee :Mardak]

Comment on attachment 391117 [details] [diff] [review]
For check-in

>+[
>+  ["gPrefService",       "preferences-service;1",      "nsIPrefBranch2"],
>+].forEach(function (service) {
>+  let [name, cid, iface] = service;
You can actually do this instead.. :)

  ["gPrefService",       "preferences-service;1",      "nsIPrefBranch2"],
].forEach(function ([name, cid, iface]) {

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 391117 [details] [diff] [review]
For check-in

>     // don't bother showing UI if the user has already made a decision
>-    if (pm.testExactPermission(currentURI, "offline-app") !=
>+    if (gPermissionManager.testExactPermission(currentURI, "offline-app") !=
>         Ci.nsIPermissionManager.UNKNOWN_ACTION)

>+    gPermissionManager.add(aDocument.documentURIObject, "offline-app",
>+                           Ci.nsIPermissionManager.ALLOW_ACTION);

I replaced Ci.nsIPermissionManager with gPermissionManager.

Comment 22

[(no longer active)]

Attachment: Be a js ninja!

Following Mardak's suggestion...

Comment 23

[John P Baker]

Comment on attachment 391117 [details] [diff] [review]
For check-in

> function getMeOutOfHere() {
>   // Get the start page from the *default* pref branch, not the user's
>-  var prefs = Cc["@mozilla.org/preferences-service;1"]
>-             .getService(Ci.nsIPrefService).getDefaultBranch(null);
>   var url = "about:blank";
>   try {
>-    url = prefs.getComplexValue("browser.startup.homepage",
>-                                Ci.nsIPrefLocalizedString).data;
>+    url = gPrefService.getComplexValue("browser.startup.homepage",
>+                                       Ci.nsIPrefLocalizedString).data;

That doesn't look right to me - wrong branch.

Comment 24

[Dão Gottwald [:dao]]

Backed out, as this caused or triggered bug 507092 somehow.

(In reply to comment #23)
Which branch?

Comment 25

[Dão Gottwald [:dao]]

Attachment: Patch (v4)

This is what got landed and backed out.

Comment 26

[John P Baker]

(In reply to comment #24)
> (In reply to comment #23)
> Which branch?

No longer the default branch:

>-             .getService(Ci.nsIPrefService).getDefaultBranch(null);

Comment 27

[Dão Gottwald [:dao]]

> nsIPrefBranch nsIPrefService::getDefaultBranch(
>	in string aPrefRoot)
> [...] aPrefRoot: [...] nsnull or "" may be used to access to the entire
> preference "tree".

This is what gPrefService does by default.

Comment 28

[John P Baker]

(In reply to comment #27)
> > nsIPrefBranch nsIPrefService::getDefaultBranch(
> >	in string aPrefRoot)
> > [...] aPrefRoot: [...] nsnull or "" may be used to access to the entire
> > preference "tree".
> 
> This is what gPrefService does by default.

Sorry I seem to be getting the terminology wrong! Presumably I meant wrong *tree* - this code wants the value from the default preferences not from the users preferences.

> When using a Get method a default value will always be returned.

Comment 29

[Dão Gottwald [:dao]]

I see. So gPrefService can't be used there.

Comment 30

[John P Baker]

(In reply to comment #29)
> I see. So gPrefService can't be used there.

+    url = gPrefService.getDefaultBranch(null).getComplexValue("browser.startup.homepage",

should be OK.

Comment 31

[John P Baker]

(In reply to comment #30)
> +    url =
> gPrefService.getDefaultBranch(null).getComplexValue("browser.startup.homepage",
> 
> should be OK.

Now I need an edit a previous comment. gPrefService is nsIPrefBranch2 not the nsIPrefService that its name suggests.

Comment 32

[Dão Gottwald [:dao]]

gPrefService.getDefaultBranch(null) would depend on other consumers, but gPrefService.QueryInterface(Ci.nsIPrefService).getDefaultBranch(null) should work reliably.

Comment 33

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

gPrefService should just also be an nsIPrefService, IMO.

Comment 34

[Dão Gottwald [:dao]]

Ehsan, any idea what was going on with bug 507092? Should we try to re-land this piece by piece?

Comment 35

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

This patch makes us keep longer-living references to these services... perhaps we should clear them onunload? I suppose that might break stuff, though.

Comment 36

[Dão Gottwald [:dao]]

(In reply to comment #35)
> I suppose that might break stuff, though.

We could re-define the getters. ;)

Comment 37

[Dão Gottwald [:dao]]

Attachment: like this

Comment 38

[Dão Gottwald [:dao]]

Attachment: like this... on top of the previous patches

Comment 39

[(no longer active)]

(In reply to comment #34)
> Ehsan, any idea what was going on with bug 507092? Should we try to re-land
> this piece by piece?

I've read this patch over and over, and I couldn't think of anything which might potentially go wrong.  And since this is OS X only, I can't even debug it to figure out which service is causing the assertion.

Regarding your getter patch, I think this may solve the problem, and I doubt if that would break other things, as nothing should be run in the context of a browser window after BrowserShutdown, right?

Have you tested this patch on the try server?  I'm going to do that now...

Comment 40

[(no longer active)]

I forgot to mention that if the new patch doesn't help, I'm in favor of landing this piece by piece and see which part is the culprit.

Comment 41

[Dão Gottwald [:dao]]

(In reply to comment #39)
> Regarding your getter patch, I think this may solve the problem, and I doubt if
> that would break other things, as nothing should be run in the context of a
> browser window after BrowserShutdown, right?

There could be more unload handlers for instance. But they would still be supported with my patch, as it re-defines the getters rather than just deleting the properties.

Comment 42

[(no longer active)]

(In reply to comment #39)
> Have you tested this patch on the try server?  I'm going to do that now...

Ah, forget it.  I just remembered that try server does not run leak tests (which was failing after this check-in.)

Can you test it locally please?

Comment 43

[Dão Gottwald [:dao]]

I don't run OS X.

Comment 44

[(no longer active)]

In that case, let's wait for Gavin's review and try to land the new getters alongside the old patch and see if the problem persists.

Comment 45

[Bryan]

PING!

Comment 46

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

Comment on attachment 391667 [details] [diff] [review]
like this... on top of the previous patches

Hmm, I forgot about this bug before starting work on bug 512784 :(

The patch in that bug largely overlaps with the patch in this one, I think, which is unfortunate. I think I prefer that bug's patch because it is slightly more generic, though.

This followup makes sense, and we can apply it to the work in that bug as well (by calling Services.release() in BrowserShutdown()).