Web content needs a way to install lightweight themes

VERIFIED FIXED in Firefox 3.7a1

Status

()

Firefox
General
P1
normal
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

(Blocks: 1 bug, {verified1.9.2})

Trunk
Firefox 3.7a1
verified1.9.2
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3.6 +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

Attachments

(3 attachments, 10 obsolete attachments)

(Assignee)

Description

8 years ago
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

8 years ago
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.
(Assignee)

Comment 2

8 years ago
(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.
(Assignee)

Updated

8 years ago
Depends on: 511108
(Assignee)

Comment 3

8 years ago
Created attachment 396257 [details] [diff] [review]
the event approach

nasty :(
(Assignee)

Comment 4

8 years ago
Created attachment 396258 [details] [diff] [review]
the event approach
(Assignee)

Updated

8 years ago
Attachment #396257 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Depends on: 511107, 510909
(Assignee)

Updated

8 years ago
Assignee: nobody → dao
(Assignee)

Comment 5

8 years ago
Created attachment 399709 [details]
test page
(Assignee)

Comment 6

8 years ago
Created attachment 400290 [details] [diff] [review]
patch
Attachment #396258 - Attachment is obsolete: true
Attachment #400290 - Flags: ui-review?(beltzner)
(Assignee)

Comment 7

8 years ago
Created attachment 400291 [details] [diff] [review]
patch

forgot to remove some cruft...
Attachment #400290 - Attachment is obsolete: true
Attachment #400291 - Flags: ui-review?(beltzner)
Attachment #400290 - Flags: ui-review?(beltzner)
(Assignee)

Updated

8 years ago
Attachment #400291 - Flags: superreview?(vladimir)
Attachment #400291 - Flags: review?(mconnor)
(Assignee)

Comment 8

8 years ago
Created attachment 400298 [details] [diff] [review]
patch v2

forgot to catch exceptins from makeURLAbsolute
Attachment #400291 - Attachment is obsolete: true
Attachment #400298 - Flags: ui-review?(beltzner)
Attachment #400298 - Flags: superreview?(vladimir)
Attachment #400298 - Flags: review?(mconnor)
Attachment #400291 - Flags: ui-review?(beltzner)
Attachment #400291 - Flags: superreview?(vladimir)
Attachment #400291 - Flags: review?(mconnor)
(Assignee)

Updated

8 years ago
Attachment #400298 - Flags: review?(l10n)

Comment 9

8 years ago
Just to make sure, what's the target milestone for this?
(Assignee)

Comment 10

8 years ago
3.6, eventually.
Flags: blocking-firefox3.6?

Comment 11

8 years ago
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.
Attachment #400298 - Flags: review?(l10n) → review+
(Assignee)

Comment 12

8 years ago
Created attachment 400489 [details] [diff] [review]
strings (landed on trunk and 192)
Attachment #400489 - Flags: approval1.9.2?
(Assignee)

Comment 13

8 years ago
Created attachment 400492 [details] [diff] [review]
patch sans strings
Attachment #400298 - Attachment is obsolete: true
Attachment #400492 - Flags: superreview?(vladimir)
Attachment #400492 - Flags: review?(mconnor)
Attachment #400298 - Flags: ui-review?(beltzner)
Attachment #400298 - Flags: superreview?(vladimir)
Attachment #400298 - Flags: review?(mconnor)
(Assignee)

Updated

8 years ago
Whiteboard: [strings]
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.
Attachment #400492 - Flags: superreview?(vladimir)
Attachment #400492 - Flags: review?(mconnor)
Attachment #400492 - Flags: review-
(Assignee)

Comment 15

8 years ago
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.
Attachment #400492 - Attachment is obsolete: true
Attachment #400513 - Flags: superreview?(vladimir)
Attachment #400513 - Flags: review?(mconnor)
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
Attachment #400489 - Flags: approval1.9.2? → approval1.9.2+
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.
Flags: blocking-firefox3.6? → blocking-firefox3.6+
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?
(Assignee)

Comment 19

8 years ago
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
Attachment #400489 - Attachment description: strings → strings (landed on trunk and 192)
(Assignee)

Updated

8 years ago
Whiteboard: [strings]
(Assignee)

Comment 20

8 years ago
(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.
(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.
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.
(Assignee)

Comment 23

8 years ago
(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.
(Assignee)

Comment 24

8 years ago
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.
Attachment #400513 - Attachment is obsolete: true
Attachment #400755 - Flags: superreview?(vladimir)
Attachment #400755 - Flags: review?(mconnor)
Attachment #400513 - Flags: superreview?(vladimir)
Attachment #400513 - Flags: review?(mconnor)
(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.
(Assignee)

Comment 26

8 years ago
Hm, I tested it with bugzilla.mozilla.org and made sure it wasn't whitelisted before. Not sure why that worked.
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.
(Assignee)

Comment 28

8 years ago
Created attachment 400767 [details] [diff] [review]
trying to whitelist getpersonas.com

exciting
Attachment #400755 - Attachment is obsolete: true
Attachment #400767 - Flags: superreview?(vladimir)
Attachment #400767 - Flags: review?(mconnor)
Attachment #400755 - Flags: superreview?(vladimir)
Attachment #400755 - Flags: review?(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
Attachment #400767 - Flags: review?(mconnor) → review+
(Assignee)

Comment 30

8 years ago
Created attachment 400885 [details] [diff] [review]
review comments addressed
Attachment #400767 - Attachment is obsolete: true
Attachment #400885 - Flags: superreview?(vladimir)
Attachment #400767 - Flags: superreview?(vladimir)
(Assignee)

Updated

8 years ago
Priority: -- → P1
Attachment #400885 - Flags: superreview?(vladimir) → superreview+
(Assignee)

Comment 31

8 years ago
http://hg.mozilla.org/mozilla-central/rev/a534af2896e2
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
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
(Assignee)

Comment 33

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/78af85507f49
status1.9.2: --- → beta1-fixed
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?
(Assignee)

Comment 35

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

Is there a bug filed on that?
(Assignee)

Comment 37

8 years ago
bug 518806
Depends on: 522522
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.
Status: RESOLVED → VERIFIED
Keywords: verified1.9.2
(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
(Assignee)

Comment 40

7 years ago
Created attachment 457621 [details]
test page

fixed an html syntax error, since the html5 parser doesn't permit it
Attachment #399709 - Attachment is obsolete: true
Blocks: 597997
You need to log in before you can comment on or make changes to this bug.