1369209 - Provide a management.install command for themes only

Status: VERIFIED FIXED

(WebExtensions :: General, enhancement, P2)

Description

[Andy McKay]

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.

Comment 1

[Andrew Swan [: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.

Comment 2

[Andy McKay]

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.

Comment 3

[Andrew Swan [:aswan]]

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?

Comment 4

[Andrew Swan [:aswan]]

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?

Comment 5

[Andy McKay]

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?

Comment 6

[Andrew Swan [:aswan]]

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.

Comment 7

[Andreas Wagner [:TheOne] [use NI]]

I'm not sure I have all the information needed to provide meaningful feedback. What work would be necessary in PersonasPlus to do that?

Comment 8

[Andrew Swan [:aswan]]

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...

Comment 9

[Andreas Wagner [:TheOne] [use NI]]

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.

Comment 10

[Andy McKay]

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.

Comment 11

[Andrew Swan [:aswan]]

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...

Comment 12

[Andreas Wagner [:TheOne] [use NI]]

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?

Comment 13

[Andrew Swan [:aswan]]

(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.

Comment 14

[:shell escalante]

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.

Comment 15

[Andreas Wagner [:TheOne] [use NI]]

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.

Comment 16

[Mike Conca [:mconca]]

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.

Comment 17

[David Durst [:ddurst]]

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.

Comment 18

[Tomislav Jovanovic :zombie]

Attachment: Bug 1369209 - Implement management.install for themes only r=robwu,kmag

Comment 19

[Rob Wu [:robwu]]

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

Rob Wu [:robwu] has approved the revision.

Comment 20

[Rob Wu [:robwu]]

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.

Comment 21

[Firefox Bug Husbandry Bot]

Bulk move of bugs per https://bugzilla.mozilla.org/show_bug.cgi?id=1483958

Comment 22

[Kris Maglione [:kmag]]

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

Kris Maglione [:kmag] has approved the revision.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ffe0fc382b2f
Implement management.install for themes only r=kmag,robwu

Comment 24

[Narcis Beleuzu [:NarcisB]]

Backed out for bc failures on browser_ext_management.js

Back out link: https://hg.mozilla.org/integration/autoland/rev/a28f7398ddff28f51ee1ac68203a02286bca6367
Push link: https://hg.mozilla.org/integration/autoland/rev/ffe0fc382b2f0c8aa80f562e6a52993f19d48162
Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=195386323&repo=autoland&lineNumber=19401

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29cdfd4dec23
Implement management.install for themes only r=kmag,robwu

Comment 26

[Cosmin Sabou [:CosminS]]

Backed out for browser chrome failures on browser_ext_management.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=29cdfd4dec2329d15b97543e8189073ef99aa6c7&tochange=15a645ce3c645ca39165d1402395db305d0fccc7&filter-searchStr=bc&selectedJob=195592012

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=195592012&repo=autoland&lineNumber=2096

Backout link: https://hg.mozilla.org/integration/autoland/rev/15a645ce3c645ca39165d1402395db305d0fccc7

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3bbdb2f97fe
Implement management.install for themes only r=robwu,kmag

Comment 28

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/e3bbdb2f97fe

Comment 29

[Tomislav Jovanovic :zombie]

Turns out using arc/Lando ends up in empty binary files landing, see bug 1486026.

Comment 30

[Victor Carciu]

Hello, 

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

Thanks,
Victor

Comment 31

[Tomislav Jovanovic :zombie]

Attachment: 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.

Comment 32

[Victor Carciu]

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.

Comment 33

[Victor Carciu]

Attachment: Postfix video

Comment 34

[Irene Smith]

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?

Comment 35

[Tomislav Jovanovic :zombie]

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.

Comment 36

[Irene Smith]

Themes can now define the colors for sidebars, such as the bookmarks sidebar (bug 1418602). The relevant properties include:

    sidebar: The background color for sidebars.
    sidebar_text: The text color for sidebars.
    sidebar_highlight: The background color of a selected item in a sidebar.
    sidebar_highlight_text: The text color of a selected item in a sidebar.

Comment 37

[Irene Smith]

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

Comment 38

[Rob Wu [:robwu]]

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

[Irene Smith]

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).

Comment 40

[Tomislav Jovanovic :zombie]

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

Comment 41

[Irene Smith]

(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?

Comment 42

[Tomislav Jovanovic :zombie]

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

Comment 43

[Irene Smith]

(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.

Comment 44

[Tomislav Jovanovic :zombie]

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...