786303 - Use the manifestURL in WebApps notifications instead of the origin

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Mounir Lamouri (:mounir)]

We should use manifestURL instead of origin and update getAppFromObserverMessage in nsIAppsService. If needed, we could even remove that method and have callers call getAppByManifestURL.

Comment 1

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 2

[Marco Castelluccio [:marco]]

Attachment: Alternative patch

Comment 3

[[:fabrice] Fabrice Desré]

Comment on attachment 778174 [details] [diff] [review]
Patch

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

::: dom/push/src/PushService.jsm
@@ +344,5 @@
>          }
>          break;
>        case "webapps-uninstall":
>          debug("webapps-uninstall");
> +        let manifestURL = aData.manifestURL;

aData is a string, so this will not work. You need to parse it as JSON first.

@@ +349,3 @@
>          let appsService = Cc["@mozilla.org/AppsService;1"]
>                              .getService(Ci.nsIAppsService);
> +        var app = appsService.getAppByManifestURL(manifestURL);

I know this is not your fault, but s/var/let.

Comment 4

[Marco Castelluccio [:marco]]

Attachment: Patch v2

(In reply to Fabrice Desré [:fabrice] from comment #3)
> aData is a string, so this will not work. You need to parse it as JSON first.

Oh, I didn't notice that because two lines after there was "aData.origin".

Comment 5

[[:fabrice] Fabrice Desré]

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

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

We're almost there!

::: dom/push/src/PushService.jsm
@@ +344,5 @@
>          }
>          break;
>        case "webapps-uninstall":
>          debug("webapps-uninstall");
> +        let data = JSON.parse(aData);

that can throw.

@@ +350,3 @@
>          let appsService = Cc["@mozilla.org/AppsService;1"]
>                              .getService(Ci.nsIAppsService);
> +        if (!appsService.getAppByManifestURL(manifestURL)) {

Since you don't use the app afterward, use getAppLocalIdByManifestURL to not create a useless app object.

Comment 6

[Marco Castelluccio [:marco]]

Attachment: Patch v3

Comment 7

[[:fabrice] Fabrice Desré]

Comment on attachment 779277 [details] [diff] [review]
Patch v3

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

::: dom/push/src/PushService.jsm
@@ +350,5 @@
> +          data = JSON.parse(aData);
> +        } catch (ex) {
> +          debug("webapps-uninstall: JSON parsing error: " + aData);
> +          return;
> +        }

Nit: blank line after }

@@ +356,3 @@
>          let appsService = Cc["@mozilla.org/AppsService;1"]
>                              .getService(Ci.nsIAppsService);
> +        if (!appsService.getAppLocalIdByManifestURL(manifestURL)) {

getAppLocalIdByManifestURL will return Ci.nsIScriptSecurityManager.NO_APP_ID when failing, not null (see https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#121)

Comment 8

[Marco Castelluccio [:marco]]

Attachment: Patch v4

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/62cea72bee19

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/62cea72bee19