597282 - Show performance warnings in addon manager

Status: RESOLVED DUPLICATE of bug 1071880

(Toolkit :: Add-ons Manager, enhancement)

Description

[Brian Carpenter [:geeknik]]

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.

Comment 1

[Raul Nicolae Malea]

Very good ideea!

Comment 2

[Brian Carpenter [:geeknik]]

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/

Comment 3

[Dave Townsend [:mossop]]

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.

Comment 4

[Ed Morley [:emorley]]

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.

Comment 5

[Hernán Rodriguez Colmeiro (:peregrino)]

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/

Comment 6

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: WIP Patch

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.

Comment 7

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: Screenshot of addon listing

This is the way the information will be displayed, though the wording is not final (I just took it from the page in AMO).

Comment 8

[Justin Scott [:fligtar]]

I'd make the wording more like "This add-on can slow down Firefox start-up by up to 25%"

Comment 9

[Jonathan Haas]

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 10

[Dave Townsend [:mossop]]

Comment on attachment 535786 [details] [diff] [review]
WIP Patch

As a first pass yeah this is looking great

Comment 11

[Michael Lefevre]

(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...

Comment 12

[Hernán Rodriguez Colmeiro (:peregrino)]

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".

Comment 13

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: WIP Patch

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?

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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).

Comment 15

[Hernán Rodriguez Colmeiro (:peregrino)]

Will start looking at this in the weekend, as bug #659794 is fixed.

Comment 16

[Asa Dotzler [:asa]]

Unfocused, peregrino, is this still something that could happen in Firefox 9 (landed in the next week?) Seems like it's stalled some.

Comment 17

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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 (".

Comment 18

[Hernán Rodriguez Colmeiro (:peregrino)]

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 :)

Comment 19

[Hernán Rodriguez Colmeiro (:peregrino)]

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.

Comment 20

[Hernán Rodriguez Colmeiro (:peregrino)]

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"

Comment 21

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: WIP Patch

Unbitrotten and addresses comments by Unfocused, still missing tests.

Comment 22

[Hernán Rodriguez Colmeiro (:peregrino)]

Working on this again, now I think I have to un-bitrot it again.

Comment 23

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: WIP Patch

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.

Comment 24

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: WIP Patch

Forgot to remove some debugging dumps...

Comment 26

[Brian Carpenter [:geeknik]]

Is this still on the radar for eventual inclusion in Firefox?

Comment 27

[Wil Clouser [:clouserw]]

(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

Comment 28

[Hernán Rodriguez Colmeiro (:peregrino)]

(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.

Comment 29

[Wil Clouser [:clouserw]]

Talk to me if there are questions, but yes, I think this should be wontfixed.

Comment 30

[Hernán Rodriguez Colmeiro (:peregrino)]

Ok, then if no-one opposes I'll just mark this as wontfix.

Comment 31

[Hernán Rodriguez Colmeiro (:peregrino)]

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.

Comment 32

[Hernán Rodriguez Colmeiro (:peregrino)]

Feel free to assign this back to me if this is considered again.

Comment 33

[Guilherme Lima]

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

Comment 34

[:shell escalante]

considering fixed with current warnings.  additional work is planned for experience based warnings with e10s.