511771 - Web content needs a way to install lightweight themes

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Dão Gottwald [:dao]]

Personas enables this by listening to a SelectPersona event. That event needs to be dispatched from a node with a "persona" attribute, which contains the persona data in a JSON string.

Ideally, I think we want a method like InstallTrigger.install, though with a lightweight-themes specific API if we can't or don't want to use the XPI format.

Comment 1

[Ed Lee :Mardak]

Is this intending for any page to install these or just something like getpersonas.com? Is there anything particular wrong with listening for the existing SelectPersona event used on that site?

If you go with InstallTrigger, you could maybe overload that function or simpler.. add another function. Or perhaps we should use the navigator. namespace to add the callback.

But providing a function instead of listening on an event would make it easier for the caller to get notified if the theme install succeeded and get triggered when it does.

Comment 2

[Dão Gottwald [:dao]]

(In reply to comment #1)
> Is this intending for any page to install these or just something like
> getpersonas.com?

I think we want this for every page with a notification asking for the user's permission, similar to extensions.

> Is there anything particular wrong with listening for the
> existing SelectPersona event used on that site?

Dispatching an event is a strange way for a web page to communicate with the browser, and the dependency on the persona attribute makes it even stranger.

> If you go with InstallTrigger, you could maybe overload that function or
> simpler.. add another function. Or perhaps we should use the navigator.
> namespace to add the callback.

Either InstallTrigger or navigator is probably the right place for this, although I'm not sure how they can be extended.

Comment 3

[Dão Gottwald [:dao]]

Attachment: the event approach

nasty :(

Comment 4

[Dão Gottwald [:dao]]

Attachment: the event approach

Comment 5

[Dão Gottwald [:dao]]

Attachment: test page

Comment 6

[Dão Gottwald [:dao]]

Attachment: patch

Comment 7

[Dão Gottwald [:dao]]

Attachment: patch

forgot to remove some cruft...

Comment 8

[Dão Gottwald [:dao]]

Attachment: patch v2

forgot to catch exceptins from makeURLAbsolute

Comment 9

[Axel Hecht [:Pike]]

Just to make sure, what's the target milestone for this?

Comment 10

[Dão Gottwald [:dao]]

3.6, eventually.

Comment 11

[Axel Hecht [:Pike]]

Comment on attachment 400298 [details] [diff] [review]
patch v2

r with nit, please add a localization note on what %S is replaced with.

As this has l10n impact, this needs to land on 1.9.2 by tomorrow.

Comment 12

[Dão Gottwald [:dao]]

Attachment: strings (landed on trunk and 192)

Comment 13

[Dão Gottwald [:dao]]

Attachment: patch sans strings

Comment 14

[Mike Connor [:mconnor]]

Comment on attachment 400492 [details] [diff] [review]
patch sans strings

this shouldn't reinvent domain handling.  Discussed on IRC, we should add getpersonas.com to the xpinstall whitelist (sorry Mossop) and use permission manager for allow/block/unknown UI behaviours.

Comment 15

[Dão Gottwald [:dao]]

Attachment: using the permission manager

I haven't yet figured out how to whitelist getpersonas.com by default, but I'm on the run and wanted to get this up.

Comment 16

[Dave Townsend [:mossop]]

Can I strongly suggest we do bug 461399 for this rather than hardcoding yet more preferences for the whitelist. It is slightly more work, but not much and will make life easier if we have to do this in the future, it'll also make the lives of other application developers far easier.

We can probably also ditch xpinstall.whitelist.add.103 and set the default xpinstall.whitelist.add to addons.mozilla.org

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

a192+ on the strings, but as a slight nit to fix on checkin, please drop the "Clicl Allow to proceed" - it's not necessary if there's an allow button.

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This looks fine, though doesn't the "install" permission also mean permission to install addons?  Do we not want/need to distinguish between the two?

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 400489 [details] [diff] [review]
strings (landed on trunk and 192)

http://hg.mozilla.org/mozilla-central/rev/d22314f3a981
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/60c949adb01f

Comment 20

[Dão Gottwald [:dao]]

(In reply to comment #18)
> This looks fine, though doesn't the "install" permission also mean permission
> to install addons?  Do we not want/need to distinguish between the two?

From a UX point of view, I don't think we want to distinguish between them, as we're going to call lighweight themes just "Themes", and the "install" permission is already called "Install Extensions or Themes". From a technical point of view, I think it's reasonable to allow extension-whitelisted sites to install lightweight themes as well.

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #20)
> (In reply to comment #18)
> > This looks fine, though doesn't the "install" permission also mean permission
> > to install addons?  Do we not want/need to distinguish between the two?
> 
> From a UX point of view, I don't think we want to distinguish between them, as
> we're going to call lighweight themes just "Themes", and the "install"
> permission is already called "Install Extensions or Themes".

Yeah, though that's talking about the heavyweight themes.  But I have no issues with the UI aspects of this.

> From a technical
> point of view, I think it's reasonable to allow extension-whitelisted sites to
> install lightweight themes as well.

Right, I'm more worried about the reverse -- if I allow a site to install lightweight themes, it now has the ability to install (well, offer to install) any random addon it wants.

Comment 22

[Mike Connor [:mconnor]]

I think that's fine.  It's basically just letting a site pop a modal dialog instead of an infobar.  The relative harm is fairly low, and I don't think it'll get abused enough to add the extra complexity.

Comment 23

[Dão Gottwald [:dao]]

(In reply to comment #21)
> Right, I'm more worried about the reverse -- if I allow a site to install
> lightweight themes, it now has the ability to install (well, offer to install)
> any random addon it wants.

We wouldn't provide a way to whitelist sites for lightweight themes specifically, and then silently enable them to install any add-on. You'd have to enable them via "Install Extensions or Themes", which you might not want, but at least you would be required to explicitly make that decision.

Comment 24

[Dão Gottwald [:dao]]

Attachment: whitelisting getpersonas.com

Looks like I can get away with modifying the existing prefs, since update.mozilla.org is out of service.

Comment 25

[Dave Townsend [:mossop]]

(In reply to comment #24)
> Created an attachment (id=400755) [details]
> whitelisting getpersonas.com
> 
> Looks like I can get away with modifying the existing prefs, since
> update.mozilla.org is out of service.

That won't work. Users with fresh profiles will get the site whitelisted but those upgrading won't.

Comment 26

[Dão Gottwald [:dao]]

Hm, I tested it with bugzilla.mozilla.org and made sure it wasn't whitelisted before. Not sure why that worked.

Comment 27

[Dave Townsend [:mossop]]

You're also going to need to add the code to import the preferences into the permissions manager during the lightweight theme install (at a guess in the _isAllowed function) in case the user hasn't tried to install anything before.

Comment 28

[Dão Gottwald [:dao]]

Attachment: trying to whitelist getpersonas.com

exciting

Comment 29

[Mike Connor [:mconnor]]

Comment on attachment 400767 [details] [diff] [review]
trying to whitelist getpersonas.com


>-pref("xpinstall.whitelist.add", "update.mozilla.org");
>-pref("xpinstall.whitelist.add.103", "addons.mozilla.org");
>+pref("xpinstall.whitelist.add", "addons.mozilla.org");
>+pref("xpinstall.whitelist.add.104", "getpersonas.com");

.36 please (103 referred to the version, let's do that again here rather than use arbitrary numbers)

I'm fine with not fixing bug 461399 here for the sake of getting this in sooner, conditional on us fixing that bug properly before 3.6 beta.

>diff --git a/toolkit/content/LightweightThemeConsumer.jsm b/toolkit/content/LightweightThemeConsumer.jsm

>@@ -96,19 +98,19 @@ LightweightThemeConsumer.prototype = {

>-      root.setAttribute("activetitlebarcolor", aData.dominantColor
>+      root.setAttribute("activetitlebarcolor", (active && aData.dominantColor)
>                           || root.getAttribute("originalactivetitlebarcolor"));
>-      root.setAttribute("inactivetitlebarcolor", aData.dominantColor
>+      root.setAttribute("inactivetitlebarcolor", (active && aData.dominantColor)
>                           || root.getAttribute("originalinactivetitlebarcolor"));

you want s/dominantColor/accentcolor/ here, I assume.

r=mconnor

Comment 30

[Dão Gottwald [:dao]]

Attachment: review comments addressed

Comment 31

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/a534af2896e2

Comment 32

[Mike Beltzner [:beltzner, not reading bugmail]]

Might be noise, but we should watch for more ...:

Regression: Txul increase 0.57% on Leopard Firefox

Previous results: 305.211 from build 20090922010845 of revision 58922624da1c at 2009-09-22 02:10:00 on talos-rev2-leopard15 run # 0

New results: 306.947 from build 20090922014806 of revision a534af2896e2 at 2009-09-22 02:43:00 on talos-rev2-leopard03 run # 0

Suspected checkin range: from revision 58922624da1c to revision a534af2896e2

Comment 33

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/78af85507f49

Comment 34

[[not reading bugmail]]

Correct me if I'm wrong, but so far though, I don't see how to install a lightweight theme by using getpersonas.com, it still wants me to download the add-on.  Is there another piece that has to work first?

Comment 35

[Dão Gottwald [:dao]]

(In reply to comment #34)
getpersonas.com needs to be updated.

Comment 36

[Dave Townsend [:mossop]]

(In reply to comment #35)
> (In reply to comment #34)
> getpersonas.com needs to be updated.

Is there a bug filed on that?

Comment 37

[Dão Gottwald [:dao]]

bug 518806

Comment 38

[Tony Chung [:tchung]]

Verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US;
rv:1.9.3a1pre) Gecko/20091111 Minefield/3.7a1pre and Mozilla/5.0 (Macintosh; U;
Intel Mac OS X 10.5; en-US; rv:1.9.2b3pre) Gecko/20091112 Namoroka/3.6b3pre.

Comment 39

[Kohei Yoshino]

(In reply to comment #5)
> Created an attachment (id=399709) [details]
> test page

I just added the self-hosting code snippet to
https://developer.mozilla.org/en/Themes/Lightweight_themes

Comment 40

[Dão Gottwald [:dao]]

Attachment: test page

fixed an html syntax error, since the html5 parser doesn't permit it