The list in the customization view is getting a little long with the addition of compact themes and 5 recommended themes. Let's cut it to a top 3, sourced from AMO. It seems like we've just hard-coded 5 specific themes here. Happy to just hard code 3 of them for now.

Top 20 lightweight themes based on the current theme reported to FHR in longitudinal: https://sql.telemetry.mozilla.org/queries/313 percent persona ======= ============ 98.680 none 0.083 recommended-2 Space Fantasy 0.061 firefox-devedition@mozilla.org 0.045 https://addons.mozilla.org/en-US/firefox/addon/18066 Dark Fox 0.021 recommended-4 Pastel Gradient 0.021 https://addons.mozilla.org/en-US/firefox/addon/60179 little flowers 0.020 recommended-1 a web browser renaissance 0.014 https://addons.mozilla.org/en-US/firefox/addon/381047 (Not on AMO anymore?) 0.011 recommended-5 Carbon light 0.011 https://addons.mozilla.org/en-US/firefox/addon/165163 Three Wolf Moon Shirt 0.009 recommended-3 Linen Light 0.009 https://addons.mozilla.org/en-US/firefox/addon/64769 Rainy day 2 (disabled by an AMO administrator) 0.009 https://addons.mozilla.org/en-US/firefox/addon/15131 Groovy Blue 0.007 https://addons.mozilla.org/en-US/firefox/addon/7610 Youlicit Toolbar 0.006 https://addons.mozilla.org/en-US/firefox/addon/25991 FIR3FOX 0.006 https://addons.mozilla.org/en-US/firefox/addon/80276 The Good Shephered 0.006 https://addons.mozilla.org/en-US/firefox/addon/46852 Sunset Over Water by MaDonna 0.006 https://addons.mozilla.org/en-US/firefox/addon/171543 Stylus Blue 0.006 https://addons.mozilla.org/en-US/firefox/addon/143671 Firefox Carbon 0.006 https://addons.mozilla.org/en-US/firefox/addon/24500 Glass - Black As a sanity check, I compared the data to https://addons.mozilla.org/en-US/firefox/themes/?sort=popular and it seems to be in the same relative order for the top 3 listed ones but then differs a bit.

Based on Matt's analysis I'm thinking we keep recommended-2, recommended-4, and recommended-1 in the pref (possibly reordering?). We'd delete the others from the pref but make sure the assets are still in tree so that if you previously installed it then it'd still work. Need to do some testing to confirm it still works even when not present in the pref.

Peter, any feedback on Comment 2? This would mean "Space Fantasy", "Pastel Gradient", and "A web browser renaissance" would stay while "Linen Light" and "Carbon Light" would go.

(In reply to Brian Grinstead [:bgrins] from comment #3) > Peter, any feedback on Comment 2? This would mean "Space Fantasy", "Pastel > Gradient", and "A web browser renaissance" would stay while "Linen Light" > and "Carbon Light" would go. I would say that generally makes sense, but the only question I have is why not "little flowers" instead of "A web browser renaissance" if it is, indeed, more popular?

(In reply to Peter Dolanjski [:pdol] from comment #4) > (In reply to Brian Grinstead [:bgrins] from comment #3) > > Peter, any feedback on Comment 2? This would mean "Space Fantasy", "Pastel > > Gradient", and "A web browser renaissance" would stay while "Linen Light" > > and "Carbon Light" would go. > > I would say that generally makes sense, but the only question I have is why > not "little flowers" instead of "A web browser renaissance" if it is, > indeed, more popular? The recommended themes are different than "Dark Fox" / "little flowers" since they are shipped with the browser and show up in Customize Mode by default. It may not be a bad idea to reach out to authors of top lwts about somehow surfacing them, but for this bug the idea is to remove 2 of the existing recommended themes.

From local testing, it seems OK to remove the recommended themes from the pref even for people who have the theme applied already. Steps I used to test: ./mach build && ./mach run --profile /tmp/test-themes Go to customize mode and select "Carbon Light" Apply patch that removes "Linen Light" and "Carbon Light" ./mach build && ./mach run --profile /tmp/test-themes In this case, the "Carbon Light" theme is still applied. I think what happens is that the lightweightThemes.recommendedThemes pref becomes modified once a theme is chosen from customize mode (it removes the selected theme from recommendedThemes), which means that the default pref value change doesn't affect this user. So, anyone who has selected a recommended theme (or maybe even opened the menu in customize mode [0]) will continue to see all 5 recommended along with compact themes. For people who haven't entered customize mode they will see only the 3 recommended along with compact themes. [0]: https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizeMode.jsm#1442

This is what it will look like for users who haven't applied a theme through customize mode before

This is what it will look like for someone who had already applied one of the recommended themes being removed. It'll look basically the same for someone who had applied one of the recommended themes that are staying, except "Carbon Light" will swap positions with whatever is applied

Gijs I see you reviewed relevant work in Bug 1007336 so I flagged you for review, but feel free to redirect

(In reply to Brian Grinstead [:bgrins] from comment #8) > Created attachment 8828086 [details] > had-applied-a-removed-recommended-theme-previously.png > > This is what it will look like for someone who had already applied one of > the recommended themes being removed. It'll look basically the same for > someone who had applied one of the recommended themes that are staying, > except "Carbon Light" will swap positions with whatever is applied Since there will still be cases where people see all the current recommended themes and the compact themes it might make sense to also tighten up the padding among these popup items to make it less likely to overflow the browser window and cause bad panel behavior. That could be done in a follow-up if so.

(In reply to Brian Grinstead [:bgrins] from comment #6) > So, anyone who has selected a recommended theme (or maybe even opened the > menu in customize mode [0]) will continue to see all 5 recommended along > with compact themes. For people who haven't entered customize mode they > will see only the 3 recommended along with compact themes. > > [0]: > https://dxr.mozilla.org/mozilla-central/source/browser/components/ > customizableui/CustomizeMode.jsm#1442 That's an oncommand handler, so it'll only happen if you select the theme from customize mode.

Comment on attachment 8828089 [details] Bug 1323833 - Trim the list of recommended themes from 5 to 3; https://reviewboard.mozilla.org/r/105604/#review106616 r=me BUT ... can we remove the now-unused strings from https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/lightweightThemes.properties#5 ? Or does that break displaying these themes for users that will still see them? From code inspection, it *looks* to me like those should be OK, because once set we will have saved these names/descriptions into the other pref and they will persist without us having the strings, but it'd be good to check that that's really true. :-)

(In reply to Brian Grinstead [:bgrins] from comment #11) > (In reply to Brian Grinstead [:bgrins] from comment #8) > > Created attachment 8828086 [details] > > had-applied-a-removed-recommended-theme-previously.png > > > > This is what it will look like for someone who had already applied one of > > the recommended themes being removed. It'll look basically the same for > > someone who had applied one of the recommended themes that are staying, > > except "Carbon Light" will swap positions with whatever is applied > > Since there will still be cases where people see all the current recommended > themes and the compact themes it might make sense to also tighten up the > padding among these popup items to make it less likely to overflow the > browser window and cause bad panel behavior. That could be done in a > follow-up if so. Can you file and needinfo someone from UX about this? I'm assuming someone's been involved with the changes to compact themes, hopefully they have cycles to give some advice here. :-)

(In reply to :Gijs from comment #14) > (In reply to Brian Grinstead [:bgrins] from comment #11) > > (In reply to Brian Grinstead [:bgrins] from comment #8) > > > Created attachment 8828086 [details] > > > had-applied-a-removed-recommended-theme-previously.png > > > > > > This is what it will look like for someone who had already applied one of > > > the recommended themes being removed. It'll look basically the same for > > > someone who had applied one of the recommended themes that are staying, > > > except "Carbon Light" will swap positions with whatever is applied > > > > Since there will still be cases where people see all the current recommended > > themes and the compact themes it might make sense to also tighten up the > > padding among these popup items to make it less likely to overflow the > > browser window and cause bad panel behavior. That could be done in a > > follow-up if so. > > Can you file and needinfo someone from UX about this? I'm assuming someone's > been involved with the changes to compact themes, hopefully they have cycles > to give some advice here. :-) The "My Themes" list is limited to five entries plus the default theme. It's always been pretty easy to hit that limit while also seeing the five recommended themes, so I'd hope that we handle that case reasonably and don't need to change anything for the addition of the compact themes.

(In reply to Dão Gottwald [:dao] from comment #15) > (In reply to :Gijs from comment #14) > > (In reply to Brian Grinstead [:bgrins] from comment #11) > > > (In reply to Brian Grinstead [:bgrins] from comment #8) > > > > Created attachment 8828086 [details] > > > > had-applied-a-removed-recommended-theme-previously.png > > > > > > > > This is what it will look like for someone who had already applied one of > > > > the recommended themes being removed. It'll look basically the same for > > > > someone who had applied one of the recommended themes that are staying, > > > > except "Carbon Light" will swap positions with whatever is applied > > > > > > Since there will still be cases where people see all the current recommended > > > themes and the compact themes it might make sense to also tighten up the > > > padding among these popup items to make it less likely to overflow the > > > browser window and cause bad panel behavior. That could be done in a > > > follow-up if so. > > > > Can you file and needinfo someone from UX about this? I'm assuming someone's > > been involved with the changes to compact themes, hopefully they have cycles > > to give some advice here. :-) > > The "My Themes" list is limited to five entries plus the default theme. It's > always been pretty easy to hit that limit while also seeing the five > recommended themes, so I'd hope that we handle that case reasonably and > don't need to change anything for the addition of the compact themes. At least in my case it's filling up the vertical space available in the window with a couple of lw themes installed. On top of it not looking great, the popup begins to behave very badly once that happens (the mouse events don't seem to fire on the correct item once it's overflowing the window). I filed 1332406 so we can discuss further there

(In reply to :Gijs from comment #13) > Comment on attachment 8828089 [details] > Bug 1323833 - Trim the list of recommended themes from 5 to 3; > > https://reviewboard.mozilla.org/r/105604/#review106616 > > r=me BUT ... can we remove the now-unused strings from > https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/ > browser/lightweightThemes.properties#5 ? Or does that break displaying these > themes for users that will still see them? From code inspection, it *looks* > to me like those should be OK, because once set we will have saved these > names/descriptions into the other pref and they will persist without us > having the strings, but it'd be good to check that that's really true. :-) I've tested this scenario and I still see the strings properly in both addons manager and customize mode, so I'll remove the strings

Pushed by bgrinstead@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/06bced347abc Trim the list of recommended themes from 5 to 3;r=Gijs

I have reproduced this bug with Nightly 53.0a1 (2016-12-15) on Windows 8.1(64-bit). This bug's fix is verified on Aurora 53.0a2. Build ID : 20170126004004 User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0 [bugday-20170125]

Thanks!

Unfortunately, the string removal here broke these lookups: https://dxr.mozilla.org/mozilla-central/rev/7d15bc419c6cd7e9f3b4d41370c3b0e5990c8d1b/browser/components/customizableui/CustomizeMode.jsm#1403-1404 which broke recommended themes for people whose list still included the old ones (the prefs get customized based on using those themes). I'll try to fix in bug 1402981.