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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Iteration:
48.3 - Apr 25
Tracking Status
firefox48 --- fixed

People

(Reporter: mossop, Assigned: aswan)

References

Details

Attachments

(2 files, 1 obsolete file)

      No description provided.
Assignee: nobody → aswan
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?
Flags: needinfo?(scolville)
Flags: needinfo?(mstriemer)
Sounds good to me. Do you have a link to the docs in question?
Flags: needinfo?(scolville)
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/
Attachment #8740158 - Flags: feedback?(dtownsend)
Attachment #8740159 - Flags: feedback?(dtownsend)
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.
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.
Attachment #8740158 - Flags: feedback?(dtownsend) → feedback+
Attachment #8740159 - Flags: feedback?(dtownsend) → feedback+
Promises sound good to me.
Flags: needinfo?(mstriemer)
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+
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/
Attachment #8740158 - Flags: review?(bzbarsky)
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+
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.
Attachment #8740159 - Flags: review?(rhelmer)
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/
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/
Attachment #8740158 - Flags: review?(bzbarsky)
Attachment #8740159 - Flags: review?(rhelmer)
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
Attachment #8740158 - Attachment is obsolete: true
Attachment #8740158 - Flags: review?(bzbarsky)
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/
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+
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.
Keywords: leave-open
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)
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.
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)
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/
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+
Keywords: checkin-needed
Keywords: checkin-needed
Iteration: --- → 48.3 - Apr 25
You need to log in before you can comment on or make changes to this bug.