Provide a management.install command for themes only

VERIFIED FIXED in Firefox 63

Status

P2
enhancement
VERIFIED FIXED
2 years ago
2 months ago

People

(Reporter: andy+bugzilla, Assigned: zombie)

Tracking

({dev-doc-complete})

unspecified
mozilla63
dev-doc-complete

Firefox Tracking Flags

(firefox63 verified)

Details

(Whiteboard: [themes], [outreach][awe:personas@christopher.beard], triaged)

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
We'd like to be to be allow personas plus to exist and we've gotten the ability to enable and disable themes (only) in WebExtensions through the management commands added in bug 1336908.

Currently personas plus also has an API which pulls the add-on from AMO and installs it locally. It would be nice to do the same. Presumably we don't need to do the pulling from AMO part and it would just be management.install(blob)... or something.

Needless to say, this should ONLY be for signed themes. Anything else throws an error.

Updated

2 years ago
Assignee: nobody → mixedpuppy
webextensions: --- → ?
Priority: -- → P3
Whiteboard: [themes, triaged], personas

Updated

2 years ago
Whiteboard: [themes, triaged], personas → [themes, triaged], [outreach personas]

Updated

2 years ago
Whiteboard: [themes, triaged], [outreach personas] → [themes], [outreach][awe:personas@christopher.beard], triaged
(Reporter)

Updated

a year ago
Assignee: mixedpuppy → aswan
We currently have both lightweight themes and the new webextension-packaged themes.  They are very different, if we need to handle both of them I think we should have separate bugs for the two formats.  But this will be a lot simpler if we can just do the webextension format and leave the old LWTs out.  So actually, I'll treat this bug as being just for webextensions and please file a separate bug if LWT support is needed.
(Reporter)

Comment 2

a year ago
I'd like to invert that please. WebExtension static themes work, but without AMO support aren't much use. The much more useful one at this point is LWT.
Well that's pretty simple then.  Do we want any safeguards beyond the "management" permission?  Like should this only be callable when handling user input so the theme doesn't suddnely change?  Should we put up the "do you want to revert" notification bar that we put up when you install a LWT from AMO?
Wait, looking more closely at existing LWTs, this proposed API is going to be rather similar to browser.themes.update() with two significant differences:
- Different names for all the object properties (though the things settable through the themes api appear to be a superset of things settable through an LWT)
- LWTs appear in the "Appearance" section of about:addons, extensions that use browser.theme.update() do not

On general principles, can we agree on one way of doing this and not have two gratuitously different ways to do essentially the same thing?
Flags: needinfo?(amckay)
(Reporter)

Comment 5

a year ago
I feel like this question goes beyond this API into the why do we have LWT, browser.themes.update and the new static themes. The answer is we are trying to remove LWT and just have new themes, but we haven't had the time to do convert everything other to the new theme API, remove LWT completely, change AMO and so on.

Landing this API would allow personas plus to work. But, I'm not sure I'm the right person to answer the in depth questions here, maybe Mike de Boer might be able to help?
Flags: needinfo?(amckay)
If the need for this is limited to personas plus, then how about having personas plus do the conversion from the LWT format to the webextensions theme API?  That doesn't require any new code in central and specifically, it avoids having to land something that is intended to be ripped right back out.

Updated

a year ago
Flags: needinfo?(awagner)
I'm not sure I have all the information needed to provide meaningful feedback. What work would be necessary in PersonasPlus to do that?
Flags: needinfo?(awagner)
Here is the documentation on the new theme format and API:
https://developer.mozilla.org/en-US/Add-ons/Themes/Theme_concepts

I don't know if the actual data format for lightweight themes is documented anywhere but my understanding is that it has all the same information but in a different format.  I think the biggest difference is that if personas plus applied theme updates directly, then they wouldn't also be visible in the "Appearance" section of about:addons (where associated information like author, version number, etc is also displayed).  If that's a deal-breaker for PP then we can implement something specifically for LWTs but if we're going to be ripping LWTs out soon, then PP needs to plan for this anyway...
Aha! Yes, technically, that should work.

Yes, the main issues I see with that approach are:
* No obvious way to 'remove' the theme other than using the Add-ons Manager to switch to a different theme. PP could provide a "reset" functionality as well (and probably has to anyway), but that won't be as intuitive as the AOM.
* No update capabilities, should the theme on AMO get updated. I'm not sure how often that happens though.
(Reporter)

Comment 10

a year ago
Bug 1373850 provides a reset API. Bug 1373851 suggests that we should do something in about:addons when theme update gets called to provide that.

Agree the two things we lose here are the ability to manage themes in about:addons (showing what theme is enabled when) and updates.
So what's the plan here?  I can implement browser.management.installLWT() if that's what we need, I just wanted to make sure we thought it through first...
Flags: needinfo?(awagner)
Since there is no dedicated PM for the AOM (that I know of), I talked to Jorge; he believes going forward with using `browser.theme.update()` is good enough, given that
1) theme update are quite rare, as mentioned,
2) installed themes using that method will show up in the AOM eventually (bug 1373851). Thanks Andy, I hadn't seen that bug before.

I believe there actually is nothing to do here, then?
Flags: needinfo?(awagner)
(In reply to Andreas Wagner [:TheOne] [use NI] from comment #12)
> I believe there actually is nothing to do here, then?

I think so.  Please feel free to re-open or open new bugs if things come up as you work on PP.
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → WONTFIX

Comment 14

a year ago
Personas Plus webextension API migration is at an MVP version: https://github.com/wagnerand/personas-plus/tree/webext

A work around was found (and partially developed, thanks Andreas), making this initial request not blocking.
At some point I'd still like to make use of an API that allows install of themes. The current approach using browser.themes.update() for everything works, but has some drawbacks for users.

Of course, this is not relevant for 57. I reset the priority.
Status: RESOLVED → REOPENED
Priority: P3 → --
Resolution: WONTFIX → ---

Updated

a year ago
Assignee: aswan → nobody

Updated

a year ago
See Also: → bug 1280237
(Reporter)

Updated

a year ago
Priority: -- → P5
After reading through the comments, it seems like the intent of this API has come full circle and now applies to static (WebExtension) themes, as aswan described in Comment 1. If that is the case, we'll need to track this with the corresponding AMO changes.
Severity: normal → enhancement
(Assignee)

Updated

7 months ago
Assignee: nobody → tomica

Updated

5 months ago
Product: Toolkit → WebExtensions
There's no issue tracking turning on static theme uploads on AMO, but it's during 63 Nightly.

So this would be good to get in 63.
Flags: needinfo?(tomica)
Priority: P5 → P2
(Assignee)

Comment 18

3 months ago
Created attachment 8999168 [details]
Bug 1369209 - Implement management.install for themes only r=robwu,kmag
(Assignee)

Updated

3 months ago
Flags: needinfo?(tomica)
Attachment #8999168 - Flags: review?(rob)
Attachment #8999168 - Flags: review?(aswan)

Updated

3 months ago
Attachment #8999168 - Attachment description: Bug 1369209 - Imlement management.install for themes only r?robwu,aswan → Bug 1369209 - Implement management.install for themes only r?robwu,aswan

Comment 19

3 months ago
Comment on attachment 8999168 [details]
Bug 1369209 - Implement management.install for themes only r=robwu,kmag

Rob Wu [:robwu] has approved the revision.
Attachment #8999168 - Flags: review+

Comment 20

3 months ago
Comment on attachment 8999168 [details]
Bug 1369209 - Implement management.install for themes only r=robwu,kmag

Trying to change r? and r+ to just r+. Let's hope that it works.
Attachment #8999168 - Flags: review?(rob)

Updated

3 months ago
Attachment #8999168 - Flags: review?(aswan)
(Assignee)

Updated

3 months ago
Attachment #8999168 - Flags: review?(aswan)
(Assignee)

Updated

3 months ago
Attachment #8999168 - Flags: review?(aswan)
Comment on attachment 8999168 [details]
Bug 1369209 - Implement management.install for themes only r=robwu,kmag

Kris Maglione [:kmag] has approved the revision.
Attachment #8999168 - Flags: review+

Updated

3 months ago
Attachment #8999168 - Attachment description: Bug 1369209 - Implement management.install for themes only r?robwu,aswan → Bug 1369209 - Implement management.install for themes only r=robwu,kmag

Comment 23

3 months ago
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ffe0fc382b2f
Implement management.install for themes only r=kmag,robwu

Comment 25

3 months ago
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/29cdfd4dec23
Implement management.install for themes only r=kmag,robwu

Comment 27

3 months ago
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3bbdb2f97fe
Implement management.install for themes only r=robwu,kmag

Comment 28

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e3bbdb2f97fe
Status: REOPENED → RESOLVED
Last Resolved: a year ago3 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(Assignee)

Comment 29

3 months ago
Turns out using arc/Lando ends up in empty binary files landing, see bug 1486026.
Flags: needinfo?(tomica)

Comment 30

3 months ago
Hello, 

Can you please provide an example of webextension using this api in order to test this bug?

Thanks,
Victor
Flags: needinfo?(tomica)
Keywords: dev-doc-needed
(Assignee)

Comment 31

2 months ago
Created attachment 9007437 [details]
management_install_example.zip

Here is a simple example, you can right-click, or click on the browser action to cycle through several static themes from AMO.
Flags: needinfo?(tomica)

Comment 32

2 months ago
I was able to use the extension from https://bugzilla.mozilla.org/show_bug.cgi?id=1369209#c31 and switching between the static themes worked as expected in latest FF63(63.0b4). Also I can confirm that same extension is not working in Firefox61.

I will attach a postfix video.
Status: RESOLVED → VERIFIED

Comment 33

2 months ago
Created attachment 9007798 [details]
Postfix video

Updated

2 months ago
status-firefox63: fixed → verified

Comment 34

2 months ago
A couple of questions:

The existing methods are documented as applying to add-ons in general, in other words not limited to themes. Should I be saying that management.install applies only to themes?

Also, reading through the comments, it seemed this was for LWT. Isn't that going away? Or does this apply to themes in general?
Flags: needinfo?(tomica)
(Assignee)

Comment 35

2 months ago
This is the only method limited to themes (for now, perhaps forever), and that should be clearly stated in the documentation.

The LWT comments are from a year ago, and not relevant anymore.  Though it's probably worth mentioning that this method only works with "static" (web extension-based, manifest-based) themes, but I'm not sure what's the latest official terminology for them on AMO.
Flags: needinfo?(tomica)
Comment hidden (obsolete)

Comment 37

2 months ago
Sorry, did it again. Comment 36 applies to another bug.

Comment 38

2 months ago
Irene, should this be dev-doc-needed again?

And FYI, you can visually mark comments as obsolete via comment tagging - click on the upper-right "Tag" button at a comment and enter one of the predefined tags, such as "obsolete" - see https://wiki.mozilla.org/BMO/comment_tagging

Comment 39

2 months ago
I didn't put it back because I was working on it and am now done. I just have too many tabs open at once. Thanks for telling me about the tag! I had no idea.

I created the page for install: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/management/install

Added a description to the management page: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/management

And, finally, added the following to the release notes:

The method management.install() allows web extensions to install and enable signed browser themes (bug 1369209).
Flags: needinfo?(tomica)
(Assignee)

Comment 40

2 months ago
This is close, though technically the `install` method doesn't take the `url` parameter, it takes the `options` object, which includes the `url` property, but also an optional `hash` property.

You can usually see full API specification in JSON Schema files included with the patch:
https://hg.mozilla.org/mozilla-central/rev/e3bbdb2f97fe#l6.58
Flags: needinfo?(tomica)
(Assignee)

Updated

2 months ago
Flags: needinfo?(ismith)

Comment 41

2 months ago
(In reply to Tomislav Jovanovic :zombie from comment #40)
> This is close, though technically the `install` method doesn't take the
> `url` parameter, it takes the `options` object, which includes the `url`
> property, but also an optional `hash` property.
> 
> You can usually see full API specification in JSON Schema files included
> with the patch:
> https://hg.mozilla.org/mozilla-central/rev/e3bbdb2f97fe#l6.58

I changed the description of the parameter to:

An object that includes the URL of the XPI file of the theme at addons.mozilla.org and an optional a hash of the XPI file, using sha256 or stronger.

However, just to clarify, this is just an object, not an object of a particular type?
Flags: needinfo?(ismith) → needinfo?(tomica)
QA Contact: ddurst
(Assignee)

Comment 42

2 months ago
This is correct.

I'm not sure about the style of putting description in prose, I think we usually explicitly list property names, like this: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/downloads/download#Parameters
Flags: needinfo?(tomica)

Comment 43

2 months ago
(In reply to Tomislav Jovanovic :zombie from comment #42)
> This is correct.
> 
> I'm not sure about the style of putting description in prose, I think we
> usually explicitly list property names, like this:
> https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/
> downloads/download#Parameters

Sorry I didn't make it more obvious that I did follow that format. This is exactly what I have:


options
    An object that includes the URL of the XPI file of the theme at addons.mozilla.org and an optional a hash of the XPI file,   using sha256 or stronger.
(Assignee)

Comment 44

2 months ago
Thanks, though I was talking about properties of the options param, sorry if that wasn't clear.

We usually explicitly list each valid property by name, with type, optinal-ity and a short description.  Like in the `downloads.download` example I linked above, we would enumerate:

 - url
    A string URL of the XPI file...
 - hash | Optional
    A hash of the xpi file...
QA Contact: ddurst
You need to log in before you can comment on or make changes to this bug.