Closed
Bug 727637
Opened 13 years ago
Closed 13 years ago
nsBrowserGlue does unnecessary work when there are no new add-ons installed
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 14
People
(Reporter: Unfocused, Assigned: Unfocused)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
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)
Comment 2•13 years ago
|
||
Really? Calling AddonManager.getAddonsByIDs with an empty array is largely a no-op. If that is costing us then we have real issues :(
Updated•13 years ago
|
Attachment #597593 -
Flags: review?(dtownsend+bugmail) → review+
Assignee | ||
Comment 3•13 years ago
|
||
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.
Assignee | ||
Comment 4•13 years ago
|
||
Flags: in-testsuite-
Flags: in-litmus-
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13
Comment 5•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 6•13 years ago
|
||
Bug 736542 backed this out, as it broke:
https://hg.mozilla.org/integration/fx-team/rev/d23d714bc491
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [fixed-in-fx-team]
Comment 7•13 years ago
|
||
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 :)
Assignee | ||
Comment 8•13 years ago
|
||
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)
Assignee | ||
Comment 9•13 years ago
|
||
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 10•13 years ago
|
||
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 11•13 years ago
|
||
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+
Assignee | ||
Comment 12•13 years ago
|
||
Whiteboard: [fixed-in-fx-team]
Updated•13 years ago
|
Attachment #607890 -
Attachment is obsolete: true
Comment 13•13 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 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.
Description
•