908999 - Update Registered/Allowed Connections for Inter-App Communication API When App Gets Uninstalled

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gene Lian (I already quit Mozilla)]

As title. We need to update _registeredConnections/_allowedConnections in the InterAppCommService.js when an app gets installed.

Bug 841736 is a good example.

Comment 1

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] from comment #0)
> As title. We need to update _registeredConnections/_allowedConnections in
> the InterAppCommService.js when an app gets installed.
                                              ^^^^^^^^^ Sorry. I mean uninstalled.
> 
> Bug 841736 is a good example.

Comment 2

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch v1

Comment 3

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8444239 [details] [diff] [review]
Patch v1

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

Close! Just some minor nits.

::: dom/apps/src/InterAppCommService.jsm
@@ +942,5 @@
> +      case "webapps-clear-data":
> +        let params =
> +          aSubject.QueryInterface(Ci.mozIApplicationClearPrivateDataParams);
> +        if (!params) {
> +          debug("Error updating IAC registered/allowed connections for an " +

if (DEBUG) {
  debut("...");
}

Don't need the keyword "IAC". We already have the prefix in the debug message and it's clear we are debugging for IAC functions.

@@ +947,5 @@
> +                "uninstalled app.");
> +          return;
> +        }
> +
> +        // Only update registered/allowed connections.

// Only update IAC registered/allowed connections from non-browser requests.

Also, can you just make it a debug:

if (params.browserOnly) {
  if (DEBUG) {
    debug("Only update registered/allowed connections from non-browser requests.");
  }
}

@@ +955,5 @@
> +
> +        let manifestURL = appsService.getManifestURLByLocalId(params.appId);
> +        if (!manifestURL) {
> +          debug("Error updating IAC registered/allowed connections for an " +
> +                "uninstalled app.");

Ditto.

@@ +961,5 @@
> +        }
> +
> +        // Update registered connections.
> +        for (let keyword in this._registeredConnections) {
> +          let keywordConnections = this._registeredConnections[keyword];

s/keywordConnections/subAppManifestURLs/ to have a consistent coding semantic.

@@ +965,5 @@
> +          let keywordConnections = this._registeredConnections[keyword];
> +          if (keywordConnections[manifestURL]) {
> +            delete keywordConnections[manifestURL];
> +            debug("Remove " + manifestURL + " from IAC registered connections" +
> +                  " due to app uninstallation.");

Ditto. Add DEBUG check and remove "IAC".

@@ +971,5 @@
> +        }
> +
> +        // Update allowed connections.
> +        for (let keyword in this._allowedConnections) {
> +          let keywordConnections = this._allowedConnections[keyword];

s/keywordConnections/allowedPubAppManifestURLs/

@@ +979,5 @@
> +                  " connections due to app uninstallation.");
> +          }
> +
> +          let pubAppManifestURLs = Object.getOwnPropertyNames(keywordConnections);
> +          for (let i = 0; i < pubAppManifestURLs.length; i++) {

Why not just:

for (let pubAppManifestURL in pubAppManifestURLs)

?

@@ +981,5 @@
> +
> +          let pubAppManifestURLs = Object.getOwnPropertyNames(keywordConnections);
> +          for (let i = 0; i < pubAppManifestURLs.length; i++) {
> +            let subAppManifestURLs = keywordConnections[pubAppManifestURLs[i]];
> +            for (let j = subAppManifestURLs.length - 1; j >= 0; j--) {

And use s/j/i/ here.

@@ +986,5 @@
> +              if (subAppManifestURLs[j] === manifestURL) {
> +                subAppManifestURLs.splice(j, 1);
> +                debug("Remove " + manifestURL + " (as a sub app to pub " +
> +                      pubAppManifestURLs[i] + ") from IAC allowed connections" +
> +                      " due to app uninstallation.");

Ditto.

Also, s/pubAppManifestURLs[i]/pubAppManifestURL/.

@@ +992,5 @@
> +            }
> +          }
> +        }
> +        debug("Finish updating IAC registered/allowed connections for an " +
> +              "uninstalled app.");

Ditto.

Comment 4

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch v2

Updating based on Gene's comments.

Comment 5

[Sean Lin [:seanlin] (inactive from Mar 2016)]

THe try run for this patch. FYI.
https://tbpl.mozilla.org/?tree=Try&rev=7a7510cf12db

Comment 6

[Gene Lian (I already quit Mozilla)]

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

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

Nice! r=gene

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2c67da7f871

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c2c67da7f871