Last Comment Bug 727637 - nsBrowserGlue does unnecessary work when there are no new add-ons installed
: nsBrowserGlue does unnecessary work when there are no new add-ons installed
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 14
Assigned To: Blair McBride [:Unfocused] (UNAVAILABLE)
Depends on: 736542
  Show dependency treegraph
Reported: 2012-02-15 15:41 PST by Blair McBride [:Unfocused] (UNAVAILABLE)
Modified: 2012-03-21 15:55 PDT (History)
4 users (show)
blair: in‑testsuite-
blair: in‑litmus-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch v1 (2.31 KB, patch)
2012-02-15 15:55 PST, Blair McBride [:Unfocused] (UNAVAILABLE)
dtownsend: review+
Details | Diff | Splinter Review
Patch v1.2 (3.65 KB, patch)
2012-03-21 03:11 PDT, Blair McBride [:Unfocused] (UNAVAILABLE)
no flags Details | Diff | Splinter Review
Patch v1.2.000001 (3.72 KB, patch)
2012-03-21 03:42 PDT, Blair McBride [:Unfocused] (UNAVAILABLE)
mak77: review+
Details | Diff | Splinter Review

Description Blair McBride [:Unfocused] (UNAVAILABLE) 2012-02-15 15:41:14 PST
In nsBrowserGlue, _onWindowsRestored checks for newly installed addons - if there are any then it opens tabs for about:newaddon. However, if there are *not* any new addons, then it still unnecessary uses the Add-ons Manager API to fetch data for an empty list of addons.
Comment 1 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-02-15 15:55:12 PST
Created attachment 597593 [details] [diff] [review]
Patch v1

Think we should optimize those APIs directly too, but this code should at least not needlessly call it.

(Found after adding additional logging and timing measurements for optimizing on mobile. Yes really.)
Comment 2 Dave Townsend [:mossop] 2012-02-16 12:56:24 PST
Really? Calling AddonManager.getAddonsByIDs with an empty array is largely a no-op. If that is costing us then we have real issues :(
Comment 3 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-02-16 17:16:17 PST
Oh, I doubt it's costing us a lot - I just saw the function call in my logs. It's quick of course, but it's still doing a bunch of stuff for no reason.
Comment 4 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-03-07 00:20:21 PST
Comment 5 Rob Campbell [:rc] (:robcee) 2012-03-08 06:45:25 PST
Comment 6 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-03-20 21:51:36 PDT
Bug 736542 backed this out, as it broke:
Comment 7 Marco Bonardo [::mak] 2012-03-21 02:59:13 PDT
since it's related, next time you land this could you also please change the module import to a lazy import? That's really useless work we do at app-startup :)
Comment 8 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-03-21 03:11:55 PDT
Created attachment 607890 [details] [diff] [review]
Patch v1.2

I so totally did what mak suggested 2 min before he suggested it. Really truely!

Also converted the nearby usages of defineLazyGetters into defineLazyModuleGetters while I was there, cos that's the new hotness.
Comment 9 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-03-21 03:42:08 PDT
Created attachment 607896 [details] [diff] [review]
Patch v1.2.000001

This one is ever betterer! (Just a little code polish after comments from mak)
Comment 10 Marco Bonardo [::mak] 2012-03-21 03:44:46 PDT
Comment on attachment 607890 [details] [diff] [review]
Patch v1.2

Review of attachment 607890 [details] [diff] [review]:

oh well, since this doesn't touch add-ons manager code, and it's bitrotting with bug 482911, I'll r it in the hope it lands soon and we can unbitrot against it.

::: browser/components/nsBrowserGlue.js
@@ +430,1 @@
>      var changedIDs = AddonManager.getStartupChanges(AddonManager.STARTUP_CHANGE_INSTALLED);

nit: old code uses var, but newer code (see keywordURLUserSet) uses let, should probably use let.

@@ +430,4 @@
>      var changedIDs = AddonManager.getStartupChanges(AddonManager.STARTUP_CHANGE_INSTALLED);
> +    if (changedIDs.length > 0) {
> +      let win = this.getMostRecentBrowserWindow();
> +      let browser = win.gBrowser;

nit: win is used only here and once, may coalesce to oneline
Comment 11 Marco Bonardo [::mak] 2012-03-21 03:45:41 PDT
Comment on attachment 607896 [details] [diff] [review]
Patch v1.2.000001

Review of attachment 607896 [details] [diff] [review]:

yes what I meant
Comment 12 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-03-21 03:50:12 PDT
Comment 13 Marco Bonardo [::mak] 2012-03-21 15:55:35 PDT

Note You need to log in before you can comment on or make changes to this bug.