909001 - Update Registered Pages for System Message 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 _pages in the SystemMessageInternal.js when an app gets uninstalled.

Bug 841736 is a good example.

Comment 1

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

Attachment: First patch

First patch.

Comment 2

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

Comment on attachment 8417286 [details] [diff] [review]
First patch

# HG changeset patch
# User Sean Lin <[e-mail]>
# Date 1399285866 -28800
#      Mon May 05 18:31:06 2014 +0800
# Node ID 608d87513ec0d0b29bf9bbf2fbd86a9aaad3ea94
# Parent  ba31cd1620bdae2b3cd1fb9c6c82594947ef8a96
imported patch bug-xxx

diff --git a/dom/messages/SystemMessageInternal.js b/dom/messages/SystemMessageInternal.js
--- a/dom/messages/SystemMessageInternal.js
+++ b/dom/messages/SystemMessageInternal.js
@@ -78,16 +78,17 @@ function SystemMessageInternal() {
 
   this._cpuWakeLocks = {};
 
   this._configurators = {};
 
   Services.obs.addObserver(this, "xpcom-shutdown", false);
   Services.obs.addObserver(this, "webapps-registry-start", false);
   Services.obs.addObserver(this, "webapps-registry-ready", false);
+  Services.obs.addObserver(this, "webapps-clear-data", false);
   kMessages.forEach(function(aMsg) {
     ppmm.addMessageListener(aMsg, this);
   }, this);
 
   Services.obs.notifyObservers(this, "system-message-internal-ready", null);
 }
 
 SystemMessageInternal.prototype = {
@@ -494,16 +495,17 @@ SystemMessageInternal.prototype = {
     switch (aTopic) {
       case "xpcom-shutdown":
         kMessages.forEach(function(aMsg) {
           ppmm.removeMessageListener(aMsg, this);
         }, this);
         Services.obs.removeObserver(this, "xpcom-shutdown");
         Services.obs.removeObserver(this, "webapps-registry-start");
         Services.obs.removeObserver(this, "webapps-registry-ready");
+        Services.obs.removeObserver(this, "webapps-clear-data");
         ppmm = null;
         this._pages = null;
         this._bufferedSysMsgs = null;
         break;
       case "webapps-registry-start":
         this._webappsRegistryReady = false;
         break;
       case "webapps-registry-ready":
@@ -519,16 +521,47 @@ SystemMessageInternal.prototype = {
               break;
             case "broadcast":
               this.broadcastMessage(aSysMsg.type, aSysMsg.msg, aSysMsg.extra);
               break;
           }
         }, this);
         this._bufferedSysMsgs.length = 0;
         break;
+      case "webapps-clear-data":
+        let params =
+          aSubject.QueryInterface(Ci.mozIApplicationClearPrivateDataParams);
+        if (!params) {
+          debug("Error updating registered pages for an uninstalled app.");
+          return;
+        }
+
+        // Only update registered pages for apps.
+        if (params.browserOnly) {
+          return;
+        }
+
+        let appsService = Cc["@mozilla.org/AppsService;1"]
+                            .getService(Ci.nsIAppsService);
+        let manifestURL = appsService.getManifestURLByLocalId(params.appId);
+        if (!manifestURL) {
+          debug("Error updating registered pages for an uninstalled app.");
+          return;
+        }
+
+        for (let i = this._pages.length - 1; i >= 0; i--) {
+          let page = this._pages[i];
+          if (page.manifestURL === manifestURL) {
+            this._pages.splice(i, 1);
+            debug("Remove " + page.pageURL + " @ " + page.manifestURL +
+                  " from registered pages due to app uninstallation");
+          }
+        }
+        debug("Finish updating registered pages for an uninstalled app.");
+        break;
     }
   },
 
   _queueMessage: function(aPage, aMessage, aMessageID) {
     // Queue the message for this page because we've never known if an app is
     // opened or not. We'll clean it up when the app has already received it.
     aPage.pendingMessages.push({ msg: aMessage, msgID: aMessageID });
     if (aPage.pendingMessages.length > kMaxPendingMessages) {

Comment 3

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

Attachment: Corrected first patch

Correct the attachment of the first patch

Comment 4

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8417293 [details] [diff] [review]
Corrected first patch

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

This is good. Just had some minor comments as below. Also, please add the bug number, description and the reviewer in your commit message. You can use:

  hg qrefresh -e

to add:

  Bug 909001 - Some descriptions about this commit (can be directly the bug title if it's clear enough). r=gene

Please ask for the review again after this is done. Note that r=foo means this patch has got foo's review+. You have to add this keyword before landing. We also have f=foo which means getting foo's feedback+. Also, a=foo means getting foo's approval for some special uplifting (this usually happens when we want to uplift a urgent patch for some branches that have been somehow fixed/released).

Regarding how to make HG patch, please refer to [1] to learn how to use HG to create patches for review. You don't have privilege to summit codes for now so you only need to read the sections before "How can I generate a patch for somebody else to check-in for me". To learn more about how to use the HG MQ extension you can look into [2] for more details. Also, there are some basic rules to learn for the code review process at [3], [4] and [5] (they are quite a lot I know but most of them are just describing the same thing).

Also, to make sure your codes don't break any existing functions, you also need to learn how to commit your patches to the Mozilla Try Server [6] to run thorough the automatic tests. You can see your testing results at [7] to make sure everything is green before asking for a check-in. Although you cannot commit the codes to the central for now, you can commit them to the Try Server, where the former needs level-3 permission and the latter only needs level-1 permission [8].

[1] https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
[2] https://developer.mozilla.org/en-US/docs/Mercurial_Queues
[3] https://developer.mozilla.org/en-US/docs/Code_Review_FAQ
[4] https://developer.mozilla.org/en-US/docs/Introduction
[5] https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch
[6] https://wiki.mozilla.org/Build:TryServer
[7] https://tbpl.mozilla.org/?tree=Try
[8] http://www.mozilla.org/hacking/commit-access-policy/

The last thing is: is that possible to make a test for this bug? To be honest, I'm not pretty sure. The closet test I can think of is writing a XPCSHELL test for it. Please refer to the patch part 1-3 at bug 915884. I make the codes to be a jsm which can be loaded into the XPCSHELL test to test some internal function logic (you can prepare your own |_pages| and fire a "webapps-clear-data" by yourself). But I don't think it's worth it for now. We can do it in a separate bug and land this first since this is bug for newbie.

After this lands, we can start to find a new bug for your next step. ;)

::: dom/messages/SystemMessageInternal.js
@@ +539,5 @@
> +          return;
> +        }
> +
> +        let appsService = Cc["@mozilla.org/AppsService;1"]
> +                            .getService(Ci.nsIAppsService);

This is fine but I'd prefer using lazyGetter to instantiate the appService:

XPCOMUtils.defineLazyGetter(this, "appsService", function() {
  return Cc["@mozilla.org/AppsService;1"].getService(Ci.nsIAppsService);
});

@@ +551,5 @@
> +          let page = this._pages[i];
> +          if (page.manifestURL === manifestURL) {
> +            this._pages.splice(i, 1);
> +            debug("Remove " + page.pageURL + " @ " + page.manifestURL +
> +                  " from registered pages due to app uninstallation");

nit: add a period at the end of sentence.

Comment 5

[Gene Lian (I already quit Mozilla)]

(In reply to Sean Lin [:seanlin] from comment #2)
> Comment on attachment 8417286 [details] [diff] [review]

Btw, don't need to post your patch in the comment but just make a brief description for it. I can review it through the "review" tag besides the patch.

Comment 6

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

Attachment: Second patch

The second patch updated based on Gene's comments.

Comment 7

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

Try-server results for this patch FYI: https://tbpl.mozilla.org/?tree=Try&rev=9629c9b19b7c

Comment 8

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8417883 [details] [diff] [review]
Second patch

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

Nice patch! Please fix the following minor comment before asking for landing. Don't need to ask for another review if you already got review+.

::: dom/messages/SystemMessageInternal.js
@@ +26,5 @@
>                                     "nsIPowerManagerService");
>  
> +XPCOMUtils.defineLazyGetter(this, "appsService", function() {
> +  return Cc["@mozilla.org/AppsService;1"].getService(Ci.nsIAppsService);
> +});

Sorry, please follow others which use XPCOMUtils.defineLazyServiceGetter to have the coding style, although this is doing the same thing.

Comment 9

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

Attachment: Third patch

Third patch: slightly updated based on Gene's approval comments on the second patch

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Gene Lian (I already quit Mozilla)]

Congratulations! Sean!