Add a way to install new add-ons with no UI feedback

RESOLVED FIXED in Firefox 48

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mossop, Assigned: aswan)

Tracking

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → aswan
(Assignee)

Comment 1

2 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

2 years ago
Flags: needinfo?(scolville)
Flags: needinfo?(mstriemer)
Sounds good to me. Do you have a link to the docs in question?
Flags: needinfo?(scolville)
(Assignee)

Comment 4

2 years ago
Created attachment 8740158 [details]
MozReview Request: Bug 1255040 Add webidl for install/uninstall via AddonManager

Review commit: https://reviewboard.mozilla.org/r/45591/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45591/
(Assignee)

Comment 5

2 years ago
Created attachment 8740159 [details]
MozReview Request: Bug 1255040 Implement mozAddonManager.createInstall() r?rhelmer

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

2 years ago
Attachment #8740158 - Flags: feedback?(dtownsend)
(Assignee)

Updated

2 years ago
Attachment #8740159 - Flags: feedback?(dtownsend)
(Reporter)

Comment 6

2 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

2 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

2 years ago
Attachment #8740158 - Flags: feedback?(dtownsend) → feedback+
(Reporter)

Updated

2 years ago
Attachment #8740159 - Flags: feedback?(dtownsend) → feedback+
Promises sound good to me.
Flags: needinfo?(mstriemer)
(Assignee)

Comment 9

2 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

2 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

2 years ago
Attachment #8740158 - Flags: review?(bzbarsky)
(Assignee)

Updated

2 years ago
Attachment #8740159 - Flags: review?(rhelmer)
Attachment #8740158 - Flags: review?(bzbarsky) → review+
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 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

2 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?
"grep -3 -r JSImpl dom/webidl/ | grep -2 EventTarget" shows a bunch of options; see RTCPeerConnection or Downloads for examples.
(Assignee)

Updated

2 years ago
Attachment #8740159 - Flags: review?(rhelmer)
(Assignee)

Comment 15

2 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

2 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

2 years ago
Attachment #8740158 - Flags: review?(bzbarsky)
Attachment #8740159 - Flags: review?(rhelmer)
(Assignee)

Comment 17

2 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

2 years ago
Attachment #8740158 - Attachment is obsolete: true
Attachment #8740158 - Flags: review?(bzbarsky)
(Assignee)

Comment 18

2 years ago
Created attachment 8742534 [details]
MozReview Request: Bug 1255040 Add webidl for install/uninstall via AddonManager r?bz

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

2 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

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

2 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.
(Assignee)

Updated

2 years ago
Keywords: leave-open
(Assignee)

Comment 26

2 years ago
Crap, sorry about that.  Revision (and more diligence with try before landing) forthcoming.
Flags: needinfo?(aswan)
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

2 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

2 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

2 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

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

2 years ago
Keywords: leave-open → checkin-needed
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 36

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/71e2c45790dc
https://hg.mozilla.org/mozilla-central/rev/a035f41ef237
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Updated

2 years ago
Iteration: --- → 48.3 - Apr 25
You need to log in before you can comment on or make changes to this bug.