Closed Bug 511771 Opened 11 years ago Closed 11 years ago

Web content needs a way to install lightweight themes

Categories

(Firefox :: General, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 3.7a1
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: verified1.9.2)

Attachments

(3 files, 10 obsolete files)

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.
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.
(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.
Depends on: 511108
Attached patch the event approach (obsolete) — Splinter Review
nasty :(
Attached patch the event approach (obsolete) — Splinter Review
Attachment #396257 - Attachment is obsolete: true
Depends on: 511107, 510909
Assignee: nobody → dao
Attached file test page (obsolete) —
Attached patch patch (obsolete) — Splinter Review
Attachment #396258 - Attachment is obsolete: true
Attachment #400290 - Flags: ui-review?(beltzner)
Attached 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)
Attachment #400291 - Flags: superreview?(vladimir)
Attachment #400291 - Flags: review?(mconnor)
Attached 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)
Attachment #400298 - Flags: review?(l10n)
Just to make sure, what's the target milestone for this?
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+
Attachment #400489 - Flags: approval1.9.2?
Attached 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)
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-
Attached 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?
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)
Whiteboard: [strings]
(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.
(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.
Attached 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.
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.
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+
Attachment #400767 - Attachment is obsolete: true
Attachment #400885 - Flags: superreview?(vladimir)
Attachment #400767 - Flags: superreview?(vladimir)
Priority: -- → P1
Attachment #400885 - Flags: superreview?(vladimir) → superreview+
http://hg.mozilla.org/mozilla-central/rev/a534af2896e2
Status: NEW → RESOLVED
Closed: 11 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?
(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?
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
Attached 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.