Closed Bug 597282 Opened 13 years ago Closed 8 years ago

Show performance warnings in addon manager

Categories

(Toolkit :: Add-ons Manager, enhancement)

Product:
Toolkit
Component:
Add-ons Manager
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1071880

People

(Reporter: geeknik, Unassigned)

References

Details

Attachments

(2 files, 4 obsolete files)

Screenshot of addon listing
337.71 KB, image/png
Details
WIP Patch
33.95 KB, patch
Details | Diff | Splinter Review 
IE9 has a new function that shows you how much startup time you will save by disabling add-ons.  Apparently IE9 will also intervene when the savings are over a certain user defined time.

Image of the IE9 feature:
http://i.imgur.com/TC3Rb.jpg

I'm pretty sure this will get duped, however, I can't find any bugs talking about this, but I recall seeing something about it in the past.
Whiteboard: [parity-IE]
Depends on: 522375
Component: General → Add-ons Manager
OS: Windows 7 → All
Product: Firefox → Toolkit
QA Contact: general → add-ons.manager
Hardware: x86_64 → All
Very good ideea!
Since Mozilla is going to start testing extensions to see how slow they are, will this get integrated into the add-on manager?

http://blog.mozilla.com/addons/2011/04/01/improving-add-on-performance/
It is certainly on our radar to roll this information into the add-ons manager as well as the website, no fixed plans as yet though.
For those that might not have used IE, this particular aspect:

(In reply to comment #0)
> Apparently IE9 will also intervene when the savings are
> over a certain user defined time.
> 
> Image of the IE9 feature:
> http://i.imgur.com/TC3Rb.jpg

Is indeed very effective.

Within the first few times I ran IE9 it warned me that addons X Y and Z (all third party addons I did not choose to install, presumably inherited from the IE8 install) were increasing startup time by X seconds and gave me the choice to disable.

This may appear only subtly different from passively informing the user in the AOM; but given that many users won't frequent the AOM regularly (or may not even know what it is; if the addons in question were installed by third party software/another user) - it will actually make a huge difference to whether users actually even know about the performance figures.
Depends on: 599169
Whiteboard: [parity-IE] → [parity-IE] DUPEME
The Addon Manager should display the addon perfomance warnings that are listed in AMO[1] in the addon detail and listing. 

Right now this information is not available to the AOM, but an enhancement to the AMO API has been requested in bug 659794.

1: https://addons.mozilla.org/en-US/firefox/performance/
Assignee: nobody → colmeiro
Depends on: 659794
Summary: Make add-on manager show how much startup time you will save by disabling add-on. → Show performance warnings in addon manager
Whiteboard: [parity-IE] DUPEME
Attached patch WIP Patch (obsolete) — — Splinter Review 
This is a WIP, as the startupTime is being faked until it comes from the AMO API, but all the rest should be the same once that data is available.
Attachment #535786 - Flags: feedback?(dtownsend)
This is the way the information will be displayed, though the wording is not final (I just took it from the page in AMO).
Attachment #535788 - Flags: feedback?(jboriss)
I'd make the wording more like "This add-on can slow down Firefox start-up by up to 25%"
I don't think "up to" is correct, if the 25% is an average value. It can probably easily be more than 25%.

We could use "This add-on can slow down Firefox start-up by 25%" but the "can" is rather ambiguous because the user (and probably we, too) don't know under which circumstances the slowdown happens.

I think the original solution is nice, an alternative might be something like "This add-on slows down Firefox start-up by 25% on average".
Comment on attachment 535786 [details] [diff] [review]
WIP Patch

As a first pass yeah this is looking great
Attachment #535786 - Flags: feedback?(dtownsend) → feedback+
(In reply to comment #9)
> I don't think "up to" is correct, if the 25% is an average value. It can
> probably easily be more than 25%.

Except that (as I understand it), it's an average value based on a test environment doing warm start with a clean profile. The average start in the real world is likely to be much slower (the average is more than 10 times slower), so the percentage is likely to be much lower. IE9's warning uses the user's own numbers, but that's not possible (yet) in Firefox.  "Up to" sounds reasonable to me...
As per bug #659794 we are not showing the exact slowdown an addon makes, so the wording will have to change. I propose just saying "This addon is slowing down your Firefox".
Attached patch WIP Patch (obsolete) — — Splinter Review 
Updated the patch so it works with the proposed <performance> data from the API and changing the wording of the message. Just missing tests.
Dave, what do you think?
Attachment #535786 - Attachment is obsolete: true
Attachment #546285 - Flags: feedback?(dtownsend)
Comment on attachment 546285 [details] [diff] [review]
WIP Patch

Driveby:

>+details.notification.performance.startupTime=This addon is slowing down Firefox.

You can't hardcode "Firefox" in strings, since this will be used by applications not called "Firefox". Do the same the the other notification strings, and replace it with gStrings.brandShortName.

Also, you'll need a separate string for the items in list-view (even if it's just the same wording in every language you know, it's a different context, and that matters in some languages).
Will start looking at this in the weekend, as bug #659794 is fixed.
Unfocused, peregrino, is this still something that could happen in Firefox 9 (landed in the next week?) Seems like it's stalled some.
(In reply to Asa Dotzler [:asa] from comment #16)
> Unfocused, peregrino, is this still something that could happen in Firefox 9
> (landed in the next week?) Seems like it's stalled some.

Dependent on Hernan's free time. I can do reviews while Dave is away. The existing patch is on the right path, but it needs updating and tests. Also, I'd expect it to have bitrotten with bug 664895, which also changed the addons.sqlite schema (and added schema migration).

As an aside, the patch all needs a code-style cleanup - it's missing various semicolons, and missing spaces in "if (".
Yeah, I've been quite busy the last months, but will do my best to get this ready before the 27th. If I can't and miss for a few days, well, it can catch the next train. But this one is leaving the station as soon as possible :)
I won't make it for the upcoming merge on 27th, I'm having some issues trying to get unbitrotten. It's going to wait for the next train.
After some work I could update the patch to work with the current source, but I'm blocked by #659794.

Since the system data that comes from the AMO API doesn't match the one I can get from nsSystemInfo, it is very difficult to get the closer match from the results. The data that comes from the API is like this: 

"appName":"fx","appVersion":"7.0.1","platformName":"MacOSX","platformVersion":"10.6.2 (rev3)"

When the corresponding OS data from nsSystemInfo would be

"platformName":"Darwin","platformVersion":"10.2.0"
Attached patch WIP Patch (obsolete) — — Splinter Review 
Unbitrotten and addresses comments by Unfocused, still missing tests.
Attachment #546285 - Attachment is obsolete: true
Attachment #546285 - Flags: feedback?(dtownsend)
Working on this again, now I think I have to un-bitrot it again.
Attached patch WIP Patch (obsolete) — — Splinter Review 
This is the WIP Patch working with latest mozilla-central. This feature is likely to take longer than expected to land due to the performance numbers turning out not to be so reliable, but I'm uploading the latest changes I made to the patch.
Attachment #566074 - Attachment is obsolete: true
Attached patch WIP Patch — — Splinter Review 
Forgot to remove some debugging dumps...
Attachment #598689 - Attachment is obsolete: true
Is this still on the radar for eventual inclusion in Firefox?
(In reply to Brian Carpenter [:geeknik] from comment #26)
> Is this still on the radar for eventual inclusion in Firefox?

I can't answer that specifically, but I can tell you that all bugs on the AMO side have been wontfixed
(In reply to Wil Clouser [:clouserw] from comment #27)
> (In reply to Brian Carpenter [:geeknik] from comment #26)
> > Is this still on the radar for eventual inclusion in Firefox?
> 
> I can't answer that specifically, but I can tell you that all bugs on the
> AMO side have been wontfixed

It's been a long time, so I'm clearly out of the loop, but who should I contact about this? If everything on the AMO side has been wontfixed, then this will probably need to be wonfixed too. Without the data coming from AMO there isn't much to do.
Talk to me if there are questions, but yes, I think this should be wontfixed.
Ok, then if no-one opposes I'll just mark this as wontfix.
I just talked about this with :unfocused and he has a valid point. While right now this is not fixable, is something that we would like to have in the addons manager at some point. So, not wontfixing it, but leaving it open as a reminder of that.
Assignee: colmeiro → nobody
Feel free to assign this back to me if this is considered again.
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
Flags: firefox-backlog+ → firefox-backlog-
Attachment #535788 - Flags: feedback?(jboriss)
Do you consider this is fixed? Firefox now warns when an addon is slowing down Firefox and you can check some numbers on about:peformance. If you think so, please mark this as a dupe of bug 1071880
considering fixed with current warnings.  additional work is planned for experience based warnings with e10s.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.