Closed Bug 727637 Opened 10 years ago Closed 10 years ago

nsBrowserGlue does unnecessary work when there are no new add-ons installed


(Firefox :: General, defect)

Not set



Firefox 14


(Reporter: Unfocused, Assigned: Unfocused)




(1 file, 2 obsolete files)

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.
Attached patch Patch v1 (obsolete) — Splinter Review
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.)
Attachment #597593 - Flags: review?(dtownsend+bugmail)
Really? Calling AddonManager.getAddonsByIDs with an empty array is largely a no-op. If that is costing us then we have real issues :(
Attachment #597593 - Flags: review?(dtownsend+bugmail) → review+
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.
Flags: in-testsuite-
Flags: in-litmus-
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 736542
Bug 736542 backed this out, as it broke:
Resolution: FIXED → ---
Whiteboard: [fixed-in-fx-team]
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 :)
Attached patch Patch v1.2 (obsolete) — Splinter Review
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.
Attachment #597593 - Attachment is obsolete: true
Attachment #607890 - Flags: review?(dtownsend+bugmail)
This one is ever betterer! (Just a little code polish after comments from mak)
Attachment #607890 - Attachment is obsolete: true
Attachment #607896 - Flags: review?(dtownsend+bugmail)
Attachment #607890 - Flags: review?(dtownsend+bugmail)
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
Attachment #607890 - Attachment is obsolete: false
Comment on attachment 607896 [details] [diff] [review]
Patch v1.2.000001

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

yes what I meant
Attachment #607896 - Flags: review?(dtownsend+bugmail) → review+
Attachment #607890 - Attachment is obsolete: true
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: Firefox 13 → Firefox 14
You need to log in before you can comment on or make changes to this bug.