Last Comment Bug 511771 - Web content needs a way to install lightweight themes
: Web content needs a way to install lightweight themes
Status: VERIFIED FIXED
: verified1.9.2
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: P1 normal with 2 votes (vote)
: Firefox 3.7a1
Assigned To: Dão Gottwald [:dao]
:
Mentors:
Depends on: 510909 511107 511108 522522
Blocks: 511104 597997
  Show dependency treegraph
 
Reported: 2009-08-20 15:56 PDT by Dão Gottwald [:dao]
Modified: 2010-09-30 10:57 PDT (History)
18 users (show)
mbeltzner: blocking‑firefox3.6+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1-fixed


Attachments
the event approach (3.08 KB, patch)
2009-08-24 11:46 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Review
the event approach (3.08 KB, patch)
2009-08-24 11:49 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Review
test page (1.22 KB, text/html)
2009-09-10 05:36 PDT, Dão Gottwald [:dao]
no flags Details
patch (10.17 KB, patch)
2009-09-12 10:01 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Review
patch (10.09 KB, patch)
2009-09-12 10:02 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Review
patch v2 (10.14 KB, patch)
2009-09-12 12:11 PDT, Dão Gottwald [:dao]
l10n: review+
Details | Diff | Review
strings (landed on trunk and 192) (1.46 KB, patch)
2009-09-14 07:11 PDT, Dão Gottwald [:dao]
mbeltzner: approval1.9.2+
Details | Diff | Review
patch sans strings (8.88 KB, patch)
2009-09-14 07:29 PDT, Dão Gottwald [:dao]
mconnor: review-
Details | Diff | Review
using the permission manager (7.85 KB, patch)
2009-09-14 08:48 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Review
whitelisting getpersonas.com (9.23 KB, patch)
2009-09-15 07:22 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Review
trying to whitelist getpersonas.com (12.97 KB, patch)
2009-09-15 08:21 PDT, Dão Gottwald [:dao]
mconnor: review+
Details | Diff | Review
review comments addressed (12.96 KB, patch)
2009-09-15 15:27 PDT, Dão Gottwald [:dao]
vladimir: superreview+
Details | Diff | Review
test page (1.22 KB, text/html)
2010-07-15 12:01 PDT, Dão Gottwald [:dao]
no flags Details

Description Dão Gottwald [:dao] 2009-08-20 15:56:06 PDT
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 2009-08-21 09:16:17 PDT
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] 2009-08-21 09:48:08 PDT
(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] 2009-08-24 11:46:16 PDT
Created attachment 396257 [details] [diff] [review]
the event approach

nasty :(
Comment 4 Dão Gottwald [:dao] 2009-08-24 11:49:47 PDT
Created attachment 396258 [details] [diff] [review]
the event approach
Comment 5 Dão Gottwald [:dao] 2009-09-10 05:36:26 PDT
Created attachment 399709 [details]
test page
Comment 6 Dão Gottwald [:dao] 2009-09-12 10:01:09 PDT
Created attachment 400290 [details] [diff] [review]
patch
Comment 7 Dão Gottwald [:dao] 2009-09-12 10:02:36 PDT
Created attachment 400291 [details] [diff] [review]
patch

forgot to remove some cruft...
Comment 8 Dão Gottwald [:dao] 2009-09-12 12:11:27 PDT
Created attachment 400298 [details] [diff] [review]
patch v2

forgot to catch exceptins from makeURLAbsolute
Comment 9 Axel Hecht [:Pike] 2009-09-13 04:41:44 PDT
Just to make sure, what's the target milestone for this?
Comment 10 Dão Gottwald [:dao] 2009-09-13 04:51:46 PDT
3.6, eventually.
Comment 11 Axel Hecht [:Pike] 2009-09-13 05:46:05 PDT
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] 2009-09-14 07:11:13 PDT
Created attachment 400489 [details] [diff] [review]
strings (landed on trunk and 192)
Comment 13 Dão Gottwald [:dao] 2009-09-14 07:29:18 PDT
Created attachment 400492 [details] [diff] [review]
patch sans strings
Comment 14 Mike Connor [:mconnor] 2009-09-14 08:26:15 PDT
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] 2009-09-14 08:48:05 PDT
Created attachment 400513 [details] [diff] [review]
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] 2009-09-14 10:21:51 PDT
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] 2009-09-14 10:45:22 PDT
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] 2009-09-14 16:28:38 PDT
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 20 Dão Gottwald [:dao] 2009-09-14 16:47:13 PDT
(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] 2009-09-14 17:24:49 PDT
(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] 2009-09-14 18:25:27 PDT
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] 2009-09-15 01:34:54 PDT
(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] 2009-09-15 07:22:29 PDT
Created attachment 400755 [details] [diff] [review]
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] 2009-09-15 07:24:58 PDT
(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] 2009-09-15 07:38:07 PDT
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] 2009-09-15 07:46:09 PDT
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] 2009-09-15 08:21:53 PDT
Created attachment 400767 [details] [diff] [review]
trying to whitelist getpersonas.com

exciting
Comment 29 Mike Connor [:mconnor] 2009-09-15 14:53:27 PDT
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] 2009-09-15 15:27:13 PDT
Created attachment 400885 [details] [diff] [review]
review comments addressed
Comment 31 Dão Gottwald [:dao] 2009-09-22 01:47:18 PDT
http://hg.mozilla.org/mozilla-central/rev/a534af2896e2
Comment 32 Mike Beltzner [:beltzner, not reading bugmail] 2009-09-22 07:12:58 PDT
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 34 [not reading bugmail] 2009-09-24 21:07:41 PDT
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] 2009-09-25 01:39:28 PDT
(In reply to comment #34)
getpersonas.com needs to be updated.
Comment 36 Dave Townsend [:mossop] 2009-09-25 03:12:50 PDT
(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] 2009-09-25 04:06:08 PDT
bug 518806
Comment 38 Tony Chung [:tchung] 2009-11-12 12:33:54 PST
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 [:kohei] 2010-04-05 05:03:04 PDT
(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] 2010-07-15 12:01:12 PDT
Created attachment 457621 [details]
test page

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

Note You need to log in before you can comment on or make changes to this bug.