If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

consolidate getService calls in browser.js

RESOLVED DUPLICATE of bug 512784

Status

()

Firefox
General
RESOLVED DUPLICATE of bug 512784
8 years ago
4 years ago

People

(Reporter: dietrich, Assigned: Ehsan)

Tracking

(Blocks: 1 bug, {perf})

Trunk
Firefox 3.6a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ts][tsnap])

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

8 years ago
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
(Reporter)

Updated

8 years ago
Whiteboard: [ts][tsnap]
Created attachment 390747 [details] [diff] [review]
Patch (v1)

I tried to catch all services instantiated more than three times.
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Attachment #390747 - Flags: review?(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 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?
Attachment #390747 - Flags: review?(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.
Created attachment 391033 [details] [diff] [review]
Patch (v2)

Addressed all review comments.
Attachment #390747 - Attachment is obsolete: true
Attachment #391033 - Flags: review?(dao)
Keywords: perf
(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.

Updated

8 years ago
Depends on: 506116
(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 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.
Attachment #391033 - Flags: review?(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"...
Created attachment 391049 [details] [diff] [review]
Patch (v3)

Use the "g" prefix, and addressed the nit.
Attachment #391033 - Attachment is obsolete: true
Attachment #391049 - Flags: review?(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
Attachment #391049 - Flags: review?(dao) → review+

Comment 12

8 years ago
(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
(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?
There's no reason not to make it nsIObserver2, at least if it's intended for external use, which seems to be the case.
You both mean nsIIOService2 rather than nsIObserver2, I'm guessing?
(In reply to comment #15)
> You both mean nsIIOService2 rather than nsIObserver2, I'm guessing?

Yes.

/me wishes bugzilla supported comment edits... :-)
Created attachment 391117 [details] [diff] [review]
For check-in
Attachment #391049 - Attachment is obsolete: true
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.
http://hg.mozilla.org/mozilla-central/rev/27b53ca1b085
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1

Comment 20

8 years ago
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 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.
Created attachment 391286 [details] [diff] [review]
Be a js ninja!

Following Mardak's suggestion...
Attachment #391286 - Flags: review?(dao)

Updated

8 years ago
Attachment #391286 - Flags: review?(dao) → review+

Comment 23

8 years ago
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.

Updated

8 years ago
Depends on: 507092
Backed out, as this caused or triggered bug 507092 somehow.

(In reply to comment #23)
Which branch?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 391307 [details] [diff] [review]
Patch (v4)

This is what got landed and backed out.
Attachment #391117 - Attachment is obsolete: true

Updated

8 years ago
Blocks: 354894

Comment 26

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

No longer the default branch:

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

Updated

8 years ago
Blocks: 467530
> 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

8 years ago
(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.
I see. So gPrefService can't be used there.

Comment 30

8 years ago
(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

8 years ago
(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.
gPrefService.getDefaultBranch(null) would depend on other consumers, but gPrefService.QueryInterface(Ci.nsIPrefService).getDefaultBranch(null) should work reliably.
gPrefService should just also be an nsIPrefService, IMO.

Updated

8 years ago
No longer blocks: 354894
Depends on: 354894
Ehsan, any idea what was going on with bug 507092? Should we try to re-land this piece by piece?
This patch makes us keep longer-living references to these services... perhaps we should clear them onunload? I suppose that might break stuff, though.
(In reply to comment #35)
> I suppose that might break stuff, though.

We could re-define the getters. ;)
Created attachment 391664 [details] [diff] [review]
like this
Attachment #391664 - Flags: review?(gavin.sharp)
Created attachment 391667 [details] [diff] [review]
like this... on top of the previous patches
Attachment #391664 - Attachment is obsolete: true
Attachment #391667 - Flags: review?(gavin.sharp)
Attachment #391664 - Flags: review?(gavin.sharp)

Updated

8 years ago
Blocks: 507623
(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...
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.
(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.
(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?
I don't run OS X.
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

8 years ago
PING!
(Reporter)

Updated

8 years ago
Blocks: 448669
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()).
Attachment #391667 - Flags: review?(gavin.sharp)

Updated

8 years ago
Blocks: 538899

Updated

8 years ago
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 512784
No longer blocks: 538899

Updated

4 years ago
No longer blocks: 467530
You need to log in before you can comment on or make changes to this bug.