Closed
Bug 1369209
Opened 8 years ago
Closed 6 years ago
Provide a management.install command for themes only
Categories
(WebExtensions :: General, enhancement, P2)
WebExtensions
General
Tracking
(firefox63 verified)
People
(Reporter: andy+bugzilla, Assigned: zombie)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [themes], [outreach][awe:personas@christopher.beard], triaged)
Attachments
(3 files)
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•7 years ago
|
Assignee: nobody → mixedpuppy
webextensions: --- → ?
Priority: -- → P3
Whiteboard: [themes, triaged], personas
Updated•7 years ago
|
Whiteboard: [themes, triaged], personas → [themes, triaged], [outreach personas]
Updated•7 years ago
|
Whiteboard: [themes, triaged], [outreach personas] → [themes], [outreach][awe:personas@christopher.beard], triaged
Reporter | ||
Updated•7 years ago
|
Assignee: mixedpuppy → aswan
Comment 1•7 years ago
|
||
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•7 years 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.
Comment 3•7 years ago
|
||
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•7 years ago
|
||
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•7 years 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)
Comment 6•7 years ago
|
||
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•7 years ago
|
Flags: needinfo?(awagner)
Comment 7•7 years ago
|
||
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)
Comment 8•7 years ago
|
||
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•7 years ago
|
||
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•7 years 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.
Comment 11•7 years ago
|
||
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)
Comment 12•7 years ago
|
||
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)
Comment 13•7 years ago
|
||
(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: 7 years ago
Resolution: --- → WONTFIX
Comment 14•7 years 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.
Comment 15•7 years ago
|
||
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•7 years ago
|
Assignee: aswan → nobody
Reporter | ||
Updated•7 years ago
|
Priority: -- → P5
Comment 16•7 years ago
|
||
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 years ago
|
Assignee: nobody → tomica
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Comment 17•6 years ago
|
||
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•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(tomica)
Attachment #8999168 -
Flags: review?(rob)
Attachment #8999168 -
Flags: review?(aswan)
Updated•6 years 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•6 years 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•6 years 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)
Comment 21•6 years ago
|
||
Bulk move of bugs per https://bugzilla.mozilla.org/show_bug.cgi?id=1483958
Component: Untriaged → General
Updated•6 years ago
|
Attachment #8999168 -
Flags: review?(aswan)
Assignee | ||
Updated•6 years ago
|
Attachment #8999168 -
Flags: review?(aswan)
Assignee | ||
Updated•6 years ago
|
Attachment #8999168 -
Flags: review?(aswan)
Comment 22•6 years ago
|
||
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•6 years 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•6 years ago
|
||
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ffe0fc382b2f
Implement management.install for themes only r=kmag,robwu
Comment 24•6 years ago
|
||
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
Flags: needinfo?(tomica)
Comment 25•6 years ago
|
||
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/29cdfd4dec23
Implement management.install for themes only r=kmag,robwu
Comment 26•6 years ago
|
||
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•6 years 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•6 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 7 years ago → 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Assignee | ||
Comment 29•6 years ago
|
||
Turns out using arc/Lando ends up in empty binary files landing, see bug 1486026.
Flags: needinfo?(tomica)
Comment 30•6 years ago
|
||
Hello,
Can you please provide an example of webextension using this api in order to test this bug?
Thanks,
Victor
Flags: needinfo?(tomica)
Updated•6 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 31•6 years ago
|
||
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•6 years 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•6 years ago
|
||
Updated•6 years ago
|
Comment 34•6 years 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•6 years 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•6 years ago
|
||
Sorry, did it again. Comment 36 applies to another bug.
Comment 38•6 years 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•6 years 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•6 years 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•6 years ago
|
Flags: needinfo?(ismith)
Comment 41•6 years 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•6 years 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•6 years 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•6 years 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...
Updated•6 years ago
|
QA Contact: ddurst
You need to log in
before you can comment on or make changes to this bug.
Description
•