919981 - Dispatch app (un)install events via the webapps actor

Status: VERIFIED FIXED

(DevTools Graveyard :: WebIDE, defect, P1)

Description

[Alexandre Poirot [:ochameau]]

So far, we can listen for app being opened/closed, and we can list installed apps, but it would also be usefull to receive events when an app is installed or uninstalled. That would help managing the list of installed apps in the device panel.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Fire appInstall/appInstalled events from the webapps actor, when listening for event on watchApps call. r=fabrice

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Proper tests and also fire such even when installing an app via the actor

Comment 3

[Alexandre Poirot [:ochameau]]

https://tbpl.mozilla.org/?tree=Try&rev=70299691ac26

Comment 4

[[:fabrice] Fabrice Desré]

Comment on attachment 809235 [details] [diff] [review]
Proper tests and also fire such even when installing an app via the actor

Review of attachment 809235 [details] [diff] [review]:
-----------------------------------------------------------------

It's a bit sad to add new observer notifications, but we'll fix that later.

::: dom/apps/src/Webapps.jsm
@@ +2077,5 @@
>          this._saveApps((function() {
>            this.updateAppHandlers(null, aManifest, appObject);
>            this.broadcastMessage("Webapps:AddApp", { id: aId, app: appObject });
> +          let appClone = AppsUtils.cloneAppObject(app);
> +          Services.obs.notifyObservers(null, "webapps-installed", JSON.stringify(appClone));

Since you only need the manifest url in webapps.js, don't use cloneAppObject which is expensive, but just create an object with the manifest url.

::: toolkit/devtools/server/actors/webapps.js
@@ +189,5 @@
>                                 oid: "foo",
>                                 requestID: "bar"
>                               });
>  
> +        Services.obs.notifyObservers(null, "webapps-installed", JSON.stringify({manifestURL: aApp.manifestURL}));

nit: is that < 80 chars?

Comment 5

[Paul Rouget [:paul]]

Any reason to send an app JSON object? If we only need the manifestURL, why not just send it?

Comment 6

[Paul Rouget [:paul]]

Attachment: Patch v2

Comment 7

[[:fabrice] Fabrice Desré]

Comment on attachment 812708 [details] [diff] [review]
Patch v2

Review of attachment 812708 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed

::: dom/apps/src/Webapps.jsm
@@ +2077,5 @@
>          this._saveApps((function() {
>            this.updateAppHandlers(null, aManifest, appObject);
>            this.broadcastMessage("Webapps:AddApp", { id: aId, app: appObject });
> +          Services.obs.notifyObservers(null, "webapps-installed",
> +            JSON.stringify({manifestURL: appObject.manifestURL}));

nit: { manifestURL: appObject.manifestURL }

@@ +2210,5 @@
>      this._saveApps((function() {
>        this.broadcastMessage("Webapps:AddApp", { id: id, app: appObject });
>        this.broadcastMessage("Webapps:Install:Return:OK", aData);
> +      Services.obs.notifyObservers(null, "webapps-installed",
> +        JSON.stringify({manifestURL: app.manifestURL}));

idem

::: toolkit/devtools/server/actors/webapps.js
@@ +190,5 @@
>                                 requestID: "bar"
>                               });
>  
> +        Services.obs.notifyObservers(null, "webapps-installed",
> +          JSON.stringify({manifestURL: aApp.manifestURL}));

nit: { manifestURL: aApp.manifestURL }

Comment 8

[[:fabrice] Fabrice Desré]

Comment on attachment 812708 [details] [diff] [review]
Patch v2

Review of attachment 812708 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed

::: dom/apps/src/Webapps.jsm
@@ +2077,5 @@
>          this._saveApps((function() {
>            this.updateAppHandlers(null, aManifest, appObject);
>            this.broadcastMessage("Webapps:AddApp", { id: aId, app: appObject });
> +          Services.obs.notifyObservers(null, "webapps-installed",
> +            JSON.stringify({manifestURL: appObject.manifestURL}));

nit: { manifestURL: appObject.manifestURL }

@@ +2210,5 @@
>      this._saveApps((function() {
>        this.broadcastMessage("Webapps:AddApp", { id: id, app: appObject });
>        this.broadcastMessage("Webapps:Install:Return:OK", aData);
> +      Services.obs.notifyObservers(null, "webapps-installed",
> +        JSON.stringify({manifestURL: app.manifestURL}));

idem

::: toolkit/devtools/server/actors/webapps.js
@@ +190,5 @@
>                                 requestID: "bar"
>                               });
>  
> +        Services.obs.notifyObservers(null, "webapps-installed",
> +          JSON.stringify({manifestURL: aApp.manifestURL}));

nit: { manifestURL: aApp.manifestURL }

Comment 9

[Paul Rouget [:paul]]

Attachment: patch v2.1 - patch to land

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/858e4ce84c04

Comment 11

[Paul Rouget [:paul]]

Comment on attachment 813591 [details] [diff] [review]
patch v2.1 - patch to land

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature (app manager)
User impact if declined: installed apps won't show up
Testing completed (on m-c, etc.): fx-team
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/858e4ce84c04

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c53578f24a06

Comment 14

[Petruta Horea [:phorea], Desktop Test Engineering]

Verified as fixed on latest Aurora 27.0a2 (20131112004004) and Firefox 26 beta 4 (20131111154639) using Win 7 64-bit, Ubuntu 13.04 32-bit and Mac OS X 10.6.8.