Closed Bug 1369209 Opened 3 years ago Closed 2 years ago
Provide a management
.install command for themes only
46 bytes, text/x-phabricator-request
|Details | Review|
798 bytes, application/x-zip-compressed
313.82 KB, image/gif
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.
Assignee: nobody → mixedpuppy
webextensions: --- → ?
Priority: -- → P3
Whiteboard: [themes, triaged], personas
Whiteboard: [themes, triaged], personas → [themes, triaged], [outreach personas]
Whiteboard: [themes, triaged], [outreach personas] → [themes], [outreach][awe:firstname.lastname@example.org], triaged
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.
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?
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?
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.
I'm not sure I have all the information needed to provide meaningful feedback. What work would be necessary in PersonasPlus to do that?
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.
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...
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?
(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
Closed: 3 years ago
Resolution: --- → WONTFIX
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 → ---
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
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.
Priority: P5 → P2
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 on attachment 8999168 [details] Bug 1369209 - Implement management.install for themes only r=robwu,kmag Rob Wu [:robwu] has approved the revision.
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.
Bulk move of bugs per https://bugzilla.mozilla.org/show_bug.cgi?id=1483958
Component: Untriaged → General
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 - Attachment description: Bug 1369209 - Implement management.install for themes only r?robwu,aswan → Bug 1369209 - Implement management.install for themes only r=robwu,kmag
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/ffe0fc382b2f Implement management.install for themes only r=kmag,robwu
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
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/29cdfd4dec23 Implement management.install for themes only r=kmag,robwu
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
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e3bbdb2f97fe Implement management.install for themes only r=robwu,kmag
Turns out using arc/Lando ends up in empty binary files landing, see bug 1486026.
Hello, Can you please provide an example of webextension using this api in order to test this bug? Thanks, Victor
Here is a simple example, you can right-click, or click on the browser action to cycle through several static themes from AMO.
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
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?
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.
Sorry, did it again. Comment 36 applies to another bug.
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
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).
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
(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
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
(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.
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...
You need to log in before you can comment on or make changes to this bug.