Closed
Bug 1255040
Opened 8 years ago
Closed 8 years ago
Add a way to install new add-ons with no UI feedback
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: mossop, Assigned: aswan)
References
Details
Attachments
(2 files, 1 obsolete file)
No description provided.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → aswan
Assignee | ||
Comment 1•8 years ago
|
||
The draft doc for this API defined an AddonInstall object that has properties state, error, progress, and maxProgress. But the actual install will be happening in the chrome process so making these things look like properties on that object in the content process would entail sending a bunch of update messages from chrome to content even if nothing is looking at those properties. I think it would be simpler to change them from properties to asynchronous methods (ie, methods that return a Promise that resolves to the requested thing). Any objections?
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(scolville)
Flags: needinfo?(mstriemer)
Comment 2•8 years ago
|
||
Sounds good to me. Do you have a link to the docs in question?
Flags: needinfo?(scolville)
Assignee | ||
Comment 3•8 years ago
|
||
https://docs.google.com/document/d/1qE1_4k8kQ12CL_ClDkrvbBsB9jRYVOe5sW9t0KR-CWE/edit?ts=56df173c
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45591/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45591/
Assignee | ||
Comment 5•8 years ago
|
||
This is a work in progress on bug 1255040. Things remaining to be done include: - wiring up the listener interface - cleaning up installs when an underlying content page is unloaded (or the content process crashes) - much more thorough testing This is my first adventure with WebIDL and with e10s message passing so just hoping to get a quick sanity check that I'm on the right track before finishing the items listed above. Review commit: https://reviewboard.mozilla.org/r/45593/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45593/
Assignee | ||
Updated•8 years ago
|
Attachment #8740158 -
Flags: feedback?(dtownsend)
Assignee | ||
Updated•8 years ago
|
Attachment #8740159 -
Flags: feedback?(dtownsend)
Reporter | ||
Comment 6•8 years ago
|
||
https://reviewboard.mozilla.org/r/45591/#review42735 f+ ::: dom/webidl/AddonManager.webidl:56 (Diff revision 1) > + Promise<long> getProgress(); > + // How many total bytes will need to be downloaded or -1 if unknown > + Promise<long> getMaxProgress(); > + > + void addListener(AddonInstallListener listener); > + void removeListener(AddonInstallListener listener); This all looks ok to me but you should have a DOM peer to the final look at the webidl as I barely know more than you! One thing that would make this clearer would be to instead just use an event-emitter style API. void on(DOMString event, EventCallback listener); void off(DOMString event, EventCallback listener); And then manager the actual event names in JS. Makes it easier to add new stuff without changing the webidl. Kind of up to you and the AMO folks what you want to do though.
Reporter | ||
Comment 7•8 years ago
|
||
https://reviewboard.mozilla.org/r/45593/#review42743 f+ certainly along the right track here ::: toolkit/mozapps/extensions/AddonManager.jsm:3055 (Diff revision 1) > - ERROR_FILE_ACCESS: -4, > + ["FILE_ACCESS", -4], > - // The add-on must be signed and isn't. > + // The add-on must be signed and isn't. > - ERROR_SIGNEDSTATE_REQUIRED: -5, > + ["SIGNEDSTATE_REQUIRED", -5], > - // The downloaded add-on had a different type than expected. > + // The downloaded add-on had a different type than expected. > - ERROR_UNEXPECTED_ADDON_TYPE: -6, > + ["UNEXPECTED_ADDON_TYPE", -6], > + ]), Bleh. I wish there was a nicer way to do this but I can't think of one. I'd prefer you include the full name in the strings though since otherwise it makes dxr'ing for the source of a constant more difficult. ::: toolkit/mozapps/extensions/amWebAPI.js:122 (Diff revision 1) > + }), > + > + getMaxProgress: WebAPITask(function*() { > + let max = yield APIBroker.sendRequest("addonInstallGetMaxProgress", this.id); > + return max; > + }), Sorry I just realised what you were doing in the other IDL. I don't think these properties need to be async functions, just readonly attributes should be fine. You can assume that they only update when one of the AddonListener events is fired, so whenever that happens in the chrome side send down updated versions of the properties along with the event.
Reporter | ||
Updated•8 years ago
|
Attachment #8740158 -
Flags: feedback?(dtownsend) → feedback+
Reporter | ||
Updated•8 years ago
|
Attachment #8740159 -
Flags: feedback?(dtownsend) → feedback+
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8740158 [details] MozReview Request: Bug 1255040 Add webidl for install/uninstall via AddonManager Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45591/diff/1-2/
Attachment #8740159 -
Attachment description: MozReview Request: Bug 1255040 wip on mozAddonManager.createInstall() → MozReview Request: Bug 1255040 Implement mozAddonManager.createInstall()
Attachment #8740158 -
Flags: feedback+
Attachment #8740159 -
Flags: feedback+
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8740159 [details] MozReview Request: Bug 1255040 Implement mozAddonManager.createInstall() r?rhelmer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45593/diff/1-2/
Assignee | ||
Updated•8 years ago
|
Attachment #8740158 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•8 years ago
|
Attachment #8740159 -
Flags: review?(rhelmer)
Updated•8 years ago
|
Attachment #8740158 -
Flags: review?(bzbarsky) → review+
Comment 11•8 years ago
|
||
Comment on attachment 8740158 [details] MozReview Request: Bug 1255040 Add webidl for install/uninstall via AddonManager https://reviewboard.mozilla.org/r/45591/#review43479 ::: dom/webidl/AddonManager.webidl:34 (Diff revision 2) > +}; > + > +[ChromeOnly, JSImplementation="dummy"] > +interface AddonInstall { > + // One of the STATE_* symbols from AddonManager.jsm > + readonly attribute DOMString state; This could arguably be an enum, but probably doesn't matter that much... ::: dom/webidl/AddonManager.webidl:40 (Diff revision 2) > + // One of the ERROR_* symbols from AddonManager.jsm, or null > + readonly attribute DOMString? error; > + // How many bytes have been downloaded > + readonly attribute long progress; > + // How many total bytes will need to be downloaded or -1 if unknown > + readonly attribute long maxProgress; This should be long long. Never know when someone has a really big add-on. ::: dom/webidl/AddonManager.webidl:42 (Diff revision 2) > + // How many bytes have been downloaded > + readonly attribute long progress; > + // How many total bytes will need to be downloaded or -1 if unknown > + readonly attribute long maxProgress; > + > + void on(DOMString event, EventHandler handler); Is there a reason we're doing it this way instead of just making this an EventTarget? ::: dom/webidl/AddonManager.webidl:50 (Diff revision 2) > + Promise<void> install(); > + Promise<void> cancel(); > +}; > + > +dictionary addonInstallOptions { > + DOMString url; Should this be a required member? Seems like there's not much you can do in createInstall without a URL... ::: toolkit/mozapps/extensions/amWebAPI.js:108 (Diff revision 2) > let addonInfo = yield APIBroker.sendRequest("getAddonByID", id); > - return addonInfo ? new Addon(addonInfo) : null; > + return addonInfo ? new Addon(this.window, addonInfo) : null; > }), > > + createInstall() { > + return Promise.reject("not yet implemented"); this.window.Promise, right?
Comment 12•8 years ago
|
||
Comment on attachment 8740158 [details] MozReview Request: Bug 1255040 Add webidl for install/uninstall via AddonManager https://reviewboard.mozilla.org/r/45591/#review43483 Oh, one more thing. Rejecting with a non-Error value is not a great pattern. Probably OK here because these rejections are temporary, but not a good habit to get into.
Attachment #8740158 -
Flags: review+
Assignee | ||
Comment 13•8 years ago
|
||
https://reviewboard.mozilla.org/r/45591/#review43479 > Is there a reason we're doing it this way instead of just making this an EventTarget? No reason other than my own ignorance... Do you happen to know of an existing js-implemented EventTarget that I could look at as a reference?
Comment 14•8 years ago
|
||
"grep -3 -r JSImpl dom/webidl/ | grep -2 EventTarget" shows a bunch of options; see RTCPeerConnection or Downloads for examples.
Assignee | ||
Updated•8 years ago
|
Attachment #8740159 -
Flags: review?(rhelmer)
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8740158 [details] MozReview Request: Bug 1255040 Add webidl for install/uninstall via AddonManager Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45591/diff/2-3/
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8740159 [details] MozReview Request: Bug 1255040 Implement mozAddonManager.createInstall() r?rhelmer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45593/diff/2-3/
Assignee | ||
Updated•8 years ago
|
Attachment #8740158 -
Flags: review?(bzbarsky)
Attachment #8740159 -
Flags: review?(rhelmer)
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8740159 [details] MozReview Request: Bug 1255040 Implement mozAddonManager.createInstall() r?rhelmer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45593/diff/3-4/
Attachment #8740159 -
Attachment description: MozReview Request: Bug 1255040 Implement mozAddonManager.createInstall() → MozReview Request: Bug 1255040 Implement mozAddonManager.createInstall() r?rhelmer
Assignee | ||
Updated•8 years ago
|
Attachment #8740158 -
Attachment is obsolete: true
Attachment #8740158 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 18•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/47263/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47263/
Attachment #8742534 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8740159 [details] MozReview Request: Bug 1255040 Implement mozAddonManager.createInstall() r?rhelmer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45593/diff/4-5/
Assignee | ||
Comment 20•8 years ago
|
||
sorry for the reviewboard spam, I tried to update one of the patches without updating the other and I appear to have inadvertently killed the original review for the first patch in the process. ugh.
Assignee | ||
Comment 21•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=341cca98f441
Comment 22•8 years ago
|
||
Comment on attachment 8742534 [details] MozReview Request: Bug 1255040 Add webidl for install/uninstall via AddonManager r?bz https://reviewboard.mozilla.org/r/47263/#review44287 r=me. I assume we're not adding any onfoo things to the IDL on purpose (and expecting consumers to just addEventListener), right?
Attachment #8742534 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 23•8 years ago
|
||
https://reviewboard.mozilla.org/r/47263/#review44287 I guess mostly to keep the changes relatively small and simple. As of this moment, we expect to have exactly one user of this API, the discovery page that appears inside about:addons. And the authors of the code that will use the API are not at all picky about what the API should look like so the `addEventListener()` route will be sufficient.
Comment 24•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/00bb04b8c9a0495fe026f609b18e09835e49c927 Bug 1255040 Add webidl for install/uninstall via AddonManager r=bz
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
I had to back this out in https://hg.mozilla.org/integration/fx-team/rev/f60bd9567a05 for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8811606&repo=fx-team
Flags: needinfo?(aswan)
Assignee | ||
Comment 26•8 years ago
|
||
Crap, sorry about that. Revision (and more diligence with try before landing) forthcoming.
Flags: needinfo?(aswan)
Comment 27•8 years ago
|
||
Comment on attachment 8740159 [details] MozReview Request: Bug 1255040 Implement mozAddonManager.createInstall() r?rhelmer https://reviewboard.mozilla.org/r/45593/#review44553 All nits except that the one about the argument to `Error`, this lgtm to me otherwise. ::: toolkit/mozapps/extensions/AddonManager.jsm:2787 (Diff revision 5) > get hotfixID() { > return gHotfixID; > }, > > webAPI: { > - getAddonByID(id) { > + // id <integer> -> AddonInstall what does this comment mean? :) ::: toolkit/mozapps/extensions/AddonManager.jsm:2839 (Diff revision 5) > + }, > + > + forgetInstall(id) { > + let info = this.installs.get(id); > + if (!info) { > + throw new Error(`forgetInstall cannot find ${id}\n`); is the newline necessary? we usually don't have these in exceptions ::: toolkit/mozapps/extensions/amWebAPI.js:24 (Diff revision 5) > _nextID: 0, > > init() { > this._promises = new Map(); > > + // id -> AddonInstall what does this comment mean? :) ::: toolkit/mozapps/extensions/amWebAPI.js:53 (Diff revision 5) > } > + > + case MSG_INSTALL_EVENT: { > + let install = this._installMap.get(payload.id); > + if (!install) { > + let err = new Error("Got install event for unknown install ${payload.id}"); Doesn't this need backticks to be a template string, so `${payload.id}` will be interpolated? ::: toolkit/mozapps/extensions/test/browser/browser_webapi_install.js:31 (Diff revision 5) > + > + Services.ppmm.addMessageListener(MSG, listener); > + }); > +} > + > +// Wrapper around a common task to run in the contet process to test s/contet/content/
Attachment #8740159 -
Flags: review?(rhelmer)
Assignee | ||
Comment 28•8 years ago
|
||
https://reviewboard.mozilla.org/r/45593/#review44553 > what does this comment mean? :) Its meant to document that the Map created on the next line has keys that are ids (which are integers) with corresponding values that are AddonInstall objects. The formatting is kind of goofy though, I'll clear it up. > what does this comment mean? :) Same as in AddonManager.jsm, documenting the keys and values of the Map created on the next line.
Assignee | ||
Comment 29•8 years ago
|
||
Comment on attachment 8742534 [details] MozReview Request: Bug 1255040 Add webidl for install/uninstall via AddonManager r?bz Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47263/diff/1-2/
Attachment #8740159 -
Flags: review?(rhelmer)
Assignee | ||
Comment 30•8 years ago
|
||
Comment on attachment 8740159 [details] MozReview Request: Bug 1255040 Implement mozAddonManager.createInstall() r?rhelmer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45593/diff/5-6/
Assignee | ||
Comment 31•8 years ago
|
||
New version should address rhelmer's feedback, and also moves the WebAPITask binding change into the first commit to fix the test breakage from comment 25.
Comment 32•8 years ago
|
||
Comment on attachment 8740159 [details] MozReview Request: Bug 1255040 Implement mozAddonManager.createInstall() r?rhelmer https://reviewboard.mozilla.org/r/45593/#review44567
Attachment #8740159 -
Flags: review?(rhelmer) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open → checkin-needed
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 35•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/71e2c45790dc https://hg.mozilla.org/integration/fx-team/rev/a035f41ef237
Keywords: checkin-needed
Comment 36•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/71e2c45790dc https://hg.mozilla.org/mozilla-central/rev/a035f41ef237
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Updated•8 years ago
|
Iteration: --- → 48.3 - Apr 25
You need to log in
before you can comment on or make changes to this bug.
Description
•