Closed Bug 520482 Opened 15 years ago Closed 14 years ago

Firefox doesn't remember more than 8 lightweight themes

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla2.0b3
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: phazon, Assigned: mossop)

References

()

Details

Attachments

(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091004 Minefield/3.7a1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091004 Minefield/3.7a1pre

I have no problem to install up to 8 Lightweight themes from getpersonas.com(not using Personas add-on) but once i have 8 themes, installation of additional theme replace one of previous installed Lightweight theme.

Reproducible: Always

Steps to Reproduce:
1. Go to http://www.getpersonas.com/en-US/gallery/
2. Install 8 different Lightweight themes, calculate and check number of lightweight themes = 8
3. Install one or two additional Lightweight themes, calculate number of installed in add-on manager again 
Actual Results:  
You still have only 8 themes! One or several of previously installed missing now.

Expected Results:  
Having 8 previously installed + one or several additional.
Blocks: 511104
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Component: General → Add-ons Manager
Ever confirmed: true
Keywords: uiwanted
OS: Windows XP → All
Product: Firefox → Toolkit
QA Contact: general → add-ons.manager
Hardware: x86 → All
Are they actaully missing or just not shown to be installed due to not being able to scroll the dialog?
This is basically intentional but we could reconsider this. Currently lightweight themes aren't "installed" as such, we just remember the most recent 8 used themes.
dave, is there a config value to make this MRU configurable?  I know of lightweightThemes.usedThemes, but dont think thats the same.
That number is currently hardcoded in LightweightThemeManager.jsm.
I find myself trying not to install too many lwthemes because there are some I want to keep, so I have to keep managing the one's I don't want.  I would like to see this improved also, because I think there also be users who want to keep so many as their favorites, like myself.  Otherwise I have to go back to getpersonas.com and re-wear them.
If the number of kept themes in the add-on manager is limited (by 8 or by any other number, if the number is changed), this limitation should be made clear in the themes section of the add-on manager.  By being in the add-on manager, people expect these themes to have the same behavior (of being permanently installed) as the old themes, the extensions, and the plugins.

I myself would prefer that there was no limit at all, but there are good reasons for the limitation, I am sure that notification would help.
Why is there a limitation? Is there a technical reason? This should at least get moved to a pref before 3.6 ships.
I think the issue here is twofold:

One is that the number of lightweight themes is limited to 8.  This is already a low number in comparison to the allowed number of regular themes, but when you consider the sheer number of lightweight themes available and the celerity with which they can be switched, it would seem like a good idea to increase this limit.

The second issue is that Firefox removes lightweight themes with with no warning.  Adding insult to injury, if the user doesn't like the new theme, he/she cannot get the removed theme back.  There's no way to undo this operation (i.e. if the user attempts to install a 9th lightweight theme and then presses the "undo" button, he/she will then have *8* themes).

A workaround for these issues is to install the Personas extension, create an account, and populate the favorites list with themes.  This may be a suboptimal solution for some people.
Summary: Minefield not allowing to install more then 8 Lightweight themes → Firefox doesn't remember more then 8 lightweight themes
Dao,

Could you please comment on this bug?

Was there a technical reason you put this limit in place?

Was it because of the JSON representation of the themes?
Since installing a lightweight theme is a mostly invisible process, I think users could end up quite easily with a large history. Presenting the whole history seems unhelpful, as at some point you'll have trouble finding something in there. Users could clean this up manually by uninstalling themes, but for the large-history scenario this seems like unnecessary PITA. So I think we want some limit, although it doesn't need to be 8.
Hi,
please increase the size to 50 or so. Very large collection of beautiful lightweight themes are available for installation. So the limit for 8 themes is not practical as we can't go back and select the older theme (>8).
Dave, Dao; how much work and possibilty would it to take to make this into a user configuration setting?  Of course we could note that anything over recommended 8 is use-at-your-own-risk, but i've seen enough questions and comments in other avenues that people want to know if 8 can be configured.
Yeah I think we should turn this into a pref and default it to something a bit higher, 15 maybe
I have a change for this.  I moved the hard-coded value to a user preference with a default of 15, and added code to check this.  Do you guys want it?
(In reply to comment #15)
> I have a change for this.  I moved the hard-coded value to a user preference
> with a default of 15, and added code to check this.  Do you guys want it?

Yeah, please attach the patch and request review from me.

Boriss, what do you think about remembering the past 15, too large or small?
Patch submitted.  I made the following changes:

Replaced "MAX_USED_THEMES_COUNT" constant with preference "lightweightThemes.maxUsedThemes" that has a default value of 15.

LightweightThemeManager.jsm was modified to check this value when updating the used themes list.
Attachment #423726 - Flags: review?(dtownsend)
Comment on attachment 423726 [details] [diff] [review]
Patch to increase allowed number of lightweight themes and add user a user preference for this limit

>--- a/browser/app/profile/firefox.js
>+++ b/browser/app/profile/firefox.js
>@@ -172,16 +172,17 @@ pref("extensions.dss.switchPending", fal
> 
> pref("extensions.{972ce4c6-7e08-4474-a285-3208198ce6fd}.name", "chrome://browser/locale/browser.properties");
> pref("extensions.{972ce4c6-7e08-4474-a285-3208198ce6fd}.description", "chrome://browser/locale/browser.properties");
> 
> pref("xpinstall.whitelist.add", "addons.mozilla.org");
> pref("xpinstall.whitelist.add.36", "getpersonas.com");
> 
> pref("lightweightThemes.update.enabled", true);
>+pref("lightweightThemes.maxUsedThemes", 15);

You need to set lightweightThemes.maxUsedThemes in modules/libpref/src/init/all.js, since you're reading it in toolkit. Note that lightweightThemes.update.enabled is being read in a try-catch block.

>+  var max_themes = _prefs.getIntPref("maxUsedThemes");

nit: maxThemes rather than max_themes is the prevailing style
Attachment #423726 - Flags: review?(dtownsend) → review-
(In reply to comment #18)
> You need to set lightweightThemes.maxUsedThemes in
> modules/libpref/src/init/all.js, since you're reading it in toolkit.

Okay, should I move "update.enabled" to all.js too?

> Note that
> lightweightThemes.update.enabled is being read in a try-catch block.

In the case of "maxUsedThemes", should I just return on error, or try to use a sensible default?  Using a default would truncate the list if the user changed the length, but bailing wouldn't add or remove anything.  I'm not sure what the preferred method is here.

> nit: maxThemes rather than max_themes is the prevailing style

No problem.  Not sure why I did that anyway.
(In reply to comment #19)
> Okay, should I move "update.enabled" to all.js too?

No, it's a per-product decision to set this pref.

> In the case of "maxUsedThemes", should I just return on error, or try to use a
> sensible default?  Using a default would truncate the list if the user changed
> the length, but bailing wouldn't add or remove anything.  I'm not sure what the
> preferred method is here.

Putting it in all.js will always give it a default value, so you don't expect anything else in your code (no error handling, no hardcoded default).
I moved the pref to the correct file, and fixed the variable name.
Attachment #423792 - Flags: review?(dao)
Comment on attachment 423726 [details] [diff] [review]
Patch to increase allowed number of lightweight themes and add user a user preference for this limit

Marked old patch as obsolete.
Attachment #423726 - Attachment is obsolete: true
Attachment #423792 - Flags: review?(dao) → review?(dtownsend)
Assignee: nobody → virtualctor
Comment on attachment 423792 [details] [diff] [review]
Resubmit of patch with changes requested by reviewer

You're going to need to add a pref observer so that you can prune the list if the user changes the pref. I'd also like to see an automated test verifying the behaviour.
Attachment #423792 - Flags: review?(dtownsend) → review-
I never saw an answer to the question if there was a technical reason for the limit.

Even with a higher limit, we still have the problem that themes disappear from the theme dialog.

Imagine if after installing a certain number of extensions one just disappeared?

That just seems wrong.
(In reply to comment #24)
> I never saw an answer to the question if there was a technical reason for the
> limit.

No technical reasons, just usability ones.
(In reply to comment #23)
> (From update of attachment 423792 [details] [diff] [review])
> You're going to need to add a pref observer so that you can prune the list if
> the user changes the pref. I'd also like to see an automated test verifying the
> behaviour.

Okay, I will add these and resubmit.  Is "toolkit/mozapps/extensions/test" the correct place for this test?
(In reply to comment #26)
> (In reply to comment #23)
> > (From update of attachment 423792 [details] [diff] [review] [details])
> > You're going to need to add a pref observer so that you can prune the list if
> > the user changes the pref. I'd also like to see an automated test verifying the
> > behaviour.
> 
> Okay, I will add these and resubmit.  Is "toolkit/mozapps/extensions/test" the
> correct place for this test?

Yes
From discussion we've agreed to drop this restriction in Firefox 4. Tim if you don't have time to come up with a new patch to just completely drop the limit then please unassign this bug from you so we know that we need to resource it.
blocking2.0: --- → beta2+
Keywords: uiwanted
Summary: Firefox doesn't remember more then 8 lightweight themes → Firefox doesn't remember more than 8 lightweight themes
This will probably require more significant changes than just dropping the limit.

The current datastore is JSON in a pref.

If the number of themes is unlimited, this storage mechanism will probably become unwieldy.

What is the limit to the length of a pref?
(In reply to comment #28)
> From discussion we've agreed to drop this restriction in Firefox 4. Tim if you
> don't have time to come up with a new patch to just completely drop the limit
> then please unassign this bug from you so we know that we need to resource it.

Does it mean we want to use the extensions.sqlite file to store that kind of information now?
Assignee: virtualctor → nobody
blocking2.0: beta2+ → final+
blocking2.0: final+ → betaN+
(In reply to comment #29)
> This will probably require more significant changes than just dropping the
> limit.
> 
> The current datastore is JSON in a pref.
> 
> If the number of themes is unlimited, this storage mechanism will probably
> become unwieldy.

I think you're right, but unfortunately we do not have the time to switch to a different storage mechanism for Firefox 4. I'd still like to alleviate the problem here but I think the more sensible choice is to go with the original plan for now and increase the limit to a large size, maybe 30. What do you think about that Ryan?

> What is the limit to the length of a pref?

My understanding is that there is no limit however we should run some performance tests to see what happens when prefs get that large, it seems that no-one knows.
Assignee: nobody → dtownsend
(In reply to comment #31)
>  I think the more sensible choice is to go with the original
> plan for now and increase the limit to a large size, maybe 30. What do you
> think about that Ryan?

I'd go big if we need a limit. 100 would be better.
I grabbed a very small Persona (no description) and it was 534 characters.

So in theory you could end up with a 53000 character preference (and a 53000 character line in prefs.js)

And then there is the issue of performance using JSON to parse that line.
(In reply to comment #33)
> I grabbed a very small Persona (no description) and it was 534 characters.
> 
> So in theory you could end up with a 53000 character preference (and a 53000
> character line in prefs.js)
> 
> And then there is the issue of performance using JSON to parse that line.

The preferences files aren't parsed by the javascript interpreter, I'll run some tests to see what effect preference length has on preference load time.
I was referring to the JSON parsing that LightweightThemeManager does.

It parses the lightweightThemes preference whenever it needs a list of all the themes or for insert and delete.
I ran some tests just now and the results are pretty good. I created a small python script that would output a line for prefs.js containing as many personas as I wanted (actually all copies of http://www.getpersonas.com/en-US/persona/172700 with unique IDs). I also added timing around the preferences load on startup, save on shutdown and also the read and parse of usedThemes inside of LightweightThemeManager.jsm. I haven't done any clever averaging here but did run most of them a few times and the numbers came out near identical each time (as I'd expect for such simple operations that we're timing).

1 Persona:

Time to load prefs: 0ms
Time to get and parse usedThemes (642 characters): 0ms
Time to save prefs: 0ms

10 Personas:

Time to load prefs: 0ms
Time to get and parse usedThemes (6393 characters): 0ms
Time to save prefs: 1ms

100 Personas:

Time to load prefs: 0ms
Time to get and parse usedThemes (63993 characters): 2ms
Time to save prefs: 3ms

1000 Personas:

Time to load prefs: 6ms
Time to get and parse usedThemes (640893 characters): 20ms
Time to save prefs: 30ms

10000 Personas:

Time to load prefs: 76ms
Time to get and parse usedThemes (6418893 characters): 254ms
Time to save prefs: 365ms

These are only a few results but they seem to be pretty linear, around 7.5us startup cost per persona, around 37us shutdown cost and 26us parsing at runtime (this happens a number of times but could easily be alleviated by caching the result of the first parse).

On the basis of this I don't think there is any performance reason why we couldn't increase the limit to 100, the only concern would be how manageable the list would be at that point.
Attached patch updated patch (obsolete) — Splinter Review
This is a variant of the original patch with tests and the pref observer. The default theme count is set to 30 but it can be modified by tinkering with lightweightThemes.maxUsedThemes. Currently that is a hidden pref but we can expose it if we think it is necessary.
Attachment #423792 - Attachment is obsolete: true
Attachment #460398 - Flags: review?(robert.bugzilla)
Status: NEW → ASSIGNED
There's no reason for 'observe' to be part of the exported object, is there? Seems like it could be a _prefObserver function in the module's global scope.
Attachment #460398 - Flags: review?(robert.bugzilla) → review+
Comment on attachment 460398 [details] [diff] [review]
updated patch

dao has a good point. I forgot to consider this is a jsm. Can you change this to not export observe?
Attachment #460398 - Flags: review+ → review-
Sorry Rob, this just makes the one change that Dao suggested.
Attachment #460398 - Attachment is obsolete: true
Attachment #460404 - Flags: review?(robert.bugzilla)
Comment on attachment 460404 [details] [diff] [review]
move out _prefObserver

Thanks!
Attachment #460404 - Flags: review?(robert.bugzilla) → review+
Note that _prefObserver can be a straight function instead of {observe: function () {}}.
Stuck with the object.

Fixed: http://hg.mozilla.org/mozilla-central/rev/c0167bb2c3b4
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b3
Status: RESOLVED → VERIFIED
Verified on :

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b3) Gecko/20100805 Firefox/4.0b3
verified on:

Mozilla/5.0 (Windows; Windows NT 6.0; rv:2.0b3) Gecko/20100805 Firefox/4.0b3
IMHO it's best to allow saving of chosen "lightweight themes" under categories or tags.

Maybe the greatest advantage of persona over theme is a relatively easy way to implement site-sensitive style without bothering the user much, both on the user end and as a choice proposed by sites themselves. This is very likely to be one of the next things to come. Making it easy would be a nice touch.

Now, if instead of client-side recognition every other visit from regulars will require an extra download to see the theme, it's unnecessary transfer that eats bandwith - on both ends, so it may discourage smaller sites from providing this option.
We added site specific Personas with Personas Interactive.

http://brandthunder.com/personas/

We also removed all limits on the number of Personas.

I like your tagging idea. Maybe I'll add that.
Never mind. I was looking at some magical mozilla2.0 branch apparently isn't being used for FF4 even though mxr says it is...
You need to log in before you can comment on or make changes to this bug.