Closed Bug 908999 Opened 8 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: airpingu, Assigned: selin)

References

Details

Attachments

(1 file, 1 obsolete file)

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

Bug 841736 is a good example.
(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.
Summary: Update Registered/Allowed Connections for Inter-App Communication API When App Gets Installed → Update Registered/Allowed Connections for Inter-App Communication API When App Gets Uninstalled
No longer depends on: inter-app-comm-api
Attached patch Patch v1 (obsolete) — Splinter Review
Assignee: nobody → selin
Attachment #8444239 - Flags: review?(gene.lian)
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.
Attachment #8444239 - Flags: review?(gene.lian)
Attached patch Patch v2Splinter Review
Updating based on Gene's comments.
Attachment #8444239 - Attachment is obsolete: true
Attachment #8445033 - Flags: review?(gene.lian)
Comment on attachment 8445033 [details] [diff] [review]
Patch v2

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

Nice! r=gene
Attachment #8445033 - Flags: review?(gene.lian) → review+
https://hg.mozilla.org/mozilla-central/rev/c2c67da7f871
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.