Web content needs a way to install lightweight themes

VERIFIED FIXED in Firefox 3.7a1

Status

()

defect
P1
normal
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

({verified1.9.2})

Trunk
Firefox 3.7a1
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

10 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

10 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

10 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

10 years ago
Depends on: 511108
Assignee

Comment 3

10 years ago
Posted patch the event approach (obsolete) — Splinter Review
nasty :(
Assignee

Comment 4

10 years ago
Posted patch the event approach (obsolete) — Splinter Review
Assignee

Updated

10 years ago
Attachment #396257 - Attachment is obsolete: true
Assignee

Updated

10 years ago
Depends on: 511107, 510909
Assignee

Updated

10 years ago
Assignee: nobody → dao
Assignee

Comment 5

10 years ago
Posted file test page (obsolete) —
Assignee

Comment 6

10 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #396258 - Attachment is obsolete: true
Attachment #400290 - Flags: ui-review?(beltzner)
Assignee

Comment 7

10 years ago
Posted patch patch (obsolete) — Splinter Review
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

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

Comment 8

10 years ago
Posted patch patch v2 (obsolete) — Splinter Review
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

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

Comment 9

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

Comment 10

10 years ago
3.6, eventually.
Flags: blocking-firefox3.6?
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

10 years ago
Attachment #400489 - Flags: approval1.9.2?
Assignee

Comment 13

10 years ago
Posted patch patch sans strings (obsolete) — Splinter Review
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

10 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

10 years ago
Posted patch using the permission manager (obsolete) — Splinter Review
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

10 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

10 years ago
Whiteboard: [strings]
Assignee

Comment 20

10 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

10 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

10 years ago
Posted patch whitelisting getpersonas.com (obsolete) — Splinter Review
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

10 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

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

10 years ago
Attachment #400767 - Attachment is obsolete: true
Attachment #400885 - Flags: superreview?(vladimir)
Attachment #400767 - Flags: superreview?(vladimir)
Assignee

Updated

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

Comment 31

10 years ago
http://hg.mozilla.org/mozilla-central/rev/a534af2896e2
Status: NEW → RESOLVED
Last Resolved: 10 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
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

10 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

10 years ago
bug 518806
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

9 years ago
Posted file 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.