841736 - Alarm API - Delete Alarm API data on app uninstallation (and clear data?)

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Mounir Lamouri (:mounir)]

A discussion on the sysapps WG made me realise that we might not take care of deleting the alarms set by an app when we delete the application.

Also, should we delete the alarms when we clear the data of an application?

Note that this is important only because the Alarm API has a |data| attribute when we create an alarm which may leak sensitive data that could be retrieved.

Comment 1

[Gene Lian (I already quit Mozilla)]

(In reply to Mounir Lamouri (:mounir) from comment #0)
> A discussion on the sysapps WG made me realise that we might not take care
> of deleting the alarms set by an app when we delete the application.

Nice catch!

Each alarm has an attribute |manifestURL| to identify its belonging app. Both in AlarmService and AlarmDB, we should remove the deprecated alarms based on the |manifestURL| when we're aware of an app is uninstalled.

I think adding observers for "webapps-uninstall" should be the time point we want.

> Also, should we delete the alarms when we clear the data of an application?

The same. Add observer to catch "webapps-clear-data" might work.

Comment 2

[Mounir Lamouri (:mounir)]

(In reply to Gene Lian [:gene] from comment #1)
> (In reply to Mounir Lamouri (:mounir) from comment #0)
> > Also, should we delete the alarms when we clear the data of an application?
> 
> The same. Add observer to catch "webapps-clear-data" might work.

You should simply listen to 'webapps-clear-data'. It is sent when an apps is uninstalled too.

Comment 3

[Gene Lian (I already quit Mozilla)]

I'm super busy with the MMS stuff right now. I'll appreciate if someone could please help me take this. This one sounds not a critically blocking bug and should be suitable for a newbie. Happy to be the mentor for solving this bug.

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Bug 851368 can be used as inspiration for somebody who wants to implement this.

Comment 5

[Sebastian Wong]

I'd love to take a crack at this. Could someone give me an idea as to where to start looking in the codebase?

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Sebastian,

dom/alarm/AlarmService is the alarms code.
app-uninstall is a system-wide event fired by the webapps system which can be observed using Services.obs.addObserver.

As I mentioned in comment 4 dom/push/src/PushService.js does something similar and is a good example.

Comment 7

[Sebastian Wong]

Attachment: First attempt at bug

Attached is my first attempt at the bug.

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

@Sebastian:

You should attach a diff and not the file itself.

Something like "hg diff > 841736.patch" and then attach 841736.patch. Ask on #developers if you run into problems.

Comment 9

[Andrew McCreight [:mccr8]]

This page has some information about how to submit a patch:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 10

[Josh Matthews [:jdm]]

Comment on attachment 740011 [details] [diff] [review]
First attempt at bug

Clearing the review since this isn't a reviewable patch.

Comment 11

[Sebastian Wong]

Attachment: Attempt at patch v1.

Added an observer for "app-uninstall" and introduced call to _removeAlarmFromDb. 

Please let me know what I can change. Apologies for the earlier mess up.

Comment 12

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 740162 [details] [diff] [review]
Attempt at patch v1.

Some comments.

1) Indentation is 2 spaces throughout the file, please cleanup the patch for that.

2) An app can install multiple alarms, it seems like the patch is removing only one.

It's a good first attempt, thanks for the patch!

Comment 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

@Sebastian: once you make the changes, please mark the patch as patch and :gene as the reviewer (r?)

Comment 14

[Gene Lian (I already quit Mozilla)]

Hi Sebastian, glad to take this review when you're done. Thanks!

Comment 15

[Mounir Lamouri (:mounir)]

We might also want to use 'webapps-clear-data' instead of 'webapps-uninstall' if we want to clear data when the user asks to clear all data for the application. We should probably do that otherwise, if the alarms are associated with an IndexedDB entry, we might end up with alarms coming but the IDB entry would have been deleted.

Comment 16

[Sebastian Wong]

Hi all, 

I'll be working on this during the weekend. I'm a bogged down with studying for a midterm at the moment.

Comment 17

[Sebastian Wong]

I can't quite figure out how to remove multiple alarms. Could someone point me in the right direction?

Comment 18

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

alarmdb.getAll() might be what you are looking for. you'll get the manifest URL using the appid from the data component of the uninstall event.

Comment 19

[Sebastian Wong]

Attachment: Attempt at patch v2.

Changed the event to "webapps-uninstall" as per suggestion and called _db.getAll to remove alarms in a manner similar to _restoreAlarmsFromDB() function. Please let me know what I should change.

Comment 20

[[:fabrice] Fabrice Desré]

Comment on attachment 747267 [details] [diff] [review]
Attempt at patch v2.

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

I think that's close, but I'll Gene check the alarm api details. Thanks!

::: dom/alarm/AlarmService.jsm
@@ +66,5 @@
>    },
>  
> +  observe: function(subject, topic, data) {
> +    data = JSON.parse(data);
> +    data.mm = subject; 

You don't need that.

@@ +67,5 @@
>  
> +  observe: function(subject, topic, data) {
> +    data = JSON.parse(data);
> +    data.mm = subject; 
> +    

Nit: trailing whitespace.

@@ +69,5 @@
> +    data = JSON.parse(data);
> +    data.mm = subject; 
> +    
> +    switch(topic) {
> +      case "webapps-uninstall": 

Nit: trailing whitespace

@@ +71,5 @@
> +    
> +    switch(topic) {
> +      case "webapps-uninstall": 
> +      this._db.getAll(
> +        data.manifestURL, 

Nit: trailing whitespace

@@ +75,5 @@
> +        data.manifestURL, 
> +        function getAllSuccessCb(aAlarms) {
> +          aAlarms.forEach(function addAlarm(aAlarm) {
> +            this._removeAlarmFromDb(aAlarm.id, null);
> +          }.bind(this));

No need to bind with forEach. You can just do aAlarms.forEach(function() {...}, this);

You don't need to name the function either (and the other callbacks also). The js engine makes up nice names now.

@@ +78,5 @@
> +            this._removeAlarmFromDb(aAlarm.id, null);
> +          }.bind(this));
> +        }.bind(this),
> +        function getAllErrorCb(aErrorMsg) {
> +

nit: remove blank line

Comment 21

[Gene Lian (I already quit Mozilla)]

Comment on attachment 747267 [details] [diff] [review]
Attempt at patch v2.

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

Thanks for the patch. Please also follow Fabrice's comments.

::: dom/alarm/AlarmService.jsm
@@ +43,4 @@
>      let alarmHalService = this._alarmHalService = Cc["@mozilla.org/alarmHalService;1"].getService(Ci.nsIAlarmHalService);
>      alarmHalService.setAlarmFiredCb(this._onAlarmFired.bind(this));
>      alarmHalService.setTimezoneChangedCb(this._onTimezoneChanged.bind(this));
> +  Services.obs.addObserver(this, "webapps-uninstall", false);

Two spaces for indention, please.

@@ +65,4 @@
>      this._restoreAlarmsFromDb();
>    },
>  
> +  observe: function(subject, topic, data) {

Please add a name for the anonymous function and let's use a unified naming convention for the parameters within this file:

observe: function observe(aSubject, aTopic, aData) {
  ...
}

@@ +68,5 @@
> +  observe: function(subject, topic, data) {
> +    data = JSON.parse(data);
> +    data.mm = subject; 
> +    
> +    switch(topic) {

Please add one space between "switch" and "(".

@@ +70,5 @@
> +    data.mm = subject; 
> +    
> +    switch(topic) {
> +      case "webapps-uninstall": 
> +      this._db.getAll(

Two spaces for indention and please do the alignment for all the following codes:

switch(topic) {
  case "webapps-uninstall":
    this._db.getAll(

@@ +75,5 @@
> +        data.manifestURL, 
> +        function getAllSuccessCb(aAlarms) {
> +          aAlarms.forEach(function addAlarm(aAlarm) {
> +            this._removeAlarmFromDb(aAlarm.id, null);
> +          }.bind(this));

I think it's fine to add a name for the anonymous function but I'd suggest s/addAlarm/removeAlarm since what you're trying to do is to remove alarm in this callback.

In addition, there is an important issue you didn't cover here. Your codes only remove the alarms in DB but don't clean up the cached queue. I think you have to do:

  this._removeAlarmFromDb(
    aAlarm.id,
    function removeSuccessCb() {...}
  );

where the .removeSuccessCb() is exactly the same as the one at:

  case "AlarmsManager:Remove":
    this._removeAlarmFromDb(
      json.id,
      json.manifestURL,
      function removeSuccessCb() {
        ...
      }.bind(this)
    );
    break;

Please try to refactorise the common part.

@@ +78,5 @@
> +            this._removeAlarmFromDb(aAlarm.id, null);
> +          }.bind(this));
> +        }.bind(this),
> +        function getAllErrorCb(aErrorMsg) {
> +

Please add:

  throw Components.results.NS_ERROR_NOT_IMPLEMENTED;

Comment 22

[[:fabrice] Fabrice Desré]

(In reply to Gene Lian [:gene] from comment #21)

> 
> Please add a name for the anonymous function and let's use a unified naming
> convention for the parameters within this file:

We really don't need to name anonymous functions anymore.

Comment 23

[Gene Lian (I already quit Mozilla)]

(In reply to Fabrice Desré [:fabrice] from comment #22)
> We really don't need to name anonymous functions anymore.

Some reviewers used to tell me it's a kind of coding convention, because the debug log can show the debug message with the belonging function name. If it's an anonymous function, then it only shows the nearest outer function name that isn't anonymous. It helps us to position the debug message. Please correct me if I'm wrong. Thanks!

Comment 24

[Gene Lian (I already quit Mozilla)]

Hi Sebastian,

Since you are a newbie (I guess), you probably need to know about the Mozilla automatic test. If you've already know, please ignore this message. :)

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 [1] to run thorough the automatic tests. You can see your testing results at [2] to make sure everything is green before asking for a check-in (some non-green items might be due to random bugs, which can be ignored). Btw, 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. Therefore, please apply for your level-1 permission while you're working on this bug because that application usually takes some time and some paper work to be done [3].

Don't hesitate to ask questions when you're stuck. Thanks!

[1] https://wiki.mozilla.org/Build:TryServer
[2] https://tbpl.mozilla.org/?tree=Try
[3] http://www.mozilla.org/hacking/commit-access-policy/

Comment 25

[Sebastian Wong]

Attachment: added requested changes

I've made what I think are the required changes. Could you take a look and tell me if it looks right? I'll request try access Try server soon. Thanks for the help so far.

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think you only need to listen to "webapps-clear-data" here. We send that also when we're uninstalling apps. In most other APIs we're only listening to "webapps-clear-data"

Comment 27

[Gene Lian (I already quit Mozilla)]

Comment on attachment 748091 [details] [diff] [review]
added requested changes

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

::: dom/alarm/AlarmService.jsm
@@ +43,4 @@
>      let alarmHalService = this._alarmHalService = Cc["@mozilla.org/alarmHalService;1"].getService(Ci.nsIAlarmHalService);
>      alarmHalService.setAlarmFiredCb(this._onAlarmFired.bind(this));
>      alarmHalService.setTimezoneChangedCb(this._onTimezoneChanged.bind(this));
> +    Services.obs.addObserver(this, "webapps-uninstall", false);

As Jonas' comment #26 and Mounir's comment #2, you should listen to the "webapps-clear-data" observer topic, instead of the "webapps-uninstall".

@@ +70,5 @@
> +
> +    switch (topic) {
> +      case "webapps-uninstall":
> +        this._db.getAll(
> +        data.manifestURL,

2-space indention from here and below, please.

@@ +72,5 @@
> +      case "webapps-uninstall":
> +        this._db.getAll(
> +        data.manifestURL,
> +        function getAllSuccessCb(aAlarms) {
> +          aAlarms.forEach(function addAlarm(aAlarm) {

Please see my comment #21. s/addAlarm/removeAlarm.

@@ +74,5 @@
> +        data.manifestURL,
> +        function getAllSuccessCb(aAlarms) {
> +          aAlarms.forEach(function addAlarm(aAlarm) {
> +            this._removeAlarmFromDb(
> +              aAlarm.id, 

nit: trailing space (TS).

@@ +75,5 @@
> +        function getAllSuccessCb(aAlarms) {
> +          aAlarms.forEach(function addAlarm(aAlarm) {
> +            this._removeAlarmFromDb(
> +              aAlarm.id, 
> +              function removeSuccessCb() {

Please try to refactor the common part in .remove() instead of copying all the codes.

@@ +82,5 @@
> +                  return;
> +                }
> +
> +                let alarmQueue = this._alarmQueue;
> +                if (this._currentAlarm.id != data.id || 

s/data.id/aAlarm.id/

nit: TS.

@@ +85,5 @@
> +                let alarmQueue = this._alarmQueue;
> +                if (this._currentAlarm.id != data.id || 
> +                    this._currentAlarm.manifestURL != data.manifestURL) {
> +                  for (let i = 0; i < alarmQueue.length; i++) {
> +                    if (alarmQueue[i].id == data.id && 

s/data.id/aAlarm.id/

nit: TS.

Comment 28

[Sebastian Wong]

Attachment: attempt at patch v3.

Comment 29

[Sebastian Wong]

Comment on attachment 750874 [details] [diff] [review]
attempt at patch v3.

refactored common code and added clearAlarm function. I have also submitted my try access application. Please let me know what else I should do.

Comment 30

[Sebastian Wong]

Comment on attachment 750874 [details] [diff] [review]
attempt at patch v3.

Refactored code and added clearAlarm function. I have also completed the try access paperwork. Please let me know if there is anything I need to do.

Comment 31

[Gene Lian (I already quit Mozilla)]

Just a reminder: please obsolete the deprecated patches when you come up with a new patch.

Comment 32

[Gene Lian (I already quit Mozilla)]

Comment on attachment 750874 [details] [diff] [review]
attempt at patch v3.

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

In general, the patch looks good. :) Some comments:

1. I'd prefer using _removeAlarm() as a private member function instead of clearAlarm().

2. Please rebase your patch on top of the latest code base, you will get lots of conflicts due to Bug 862322 and Bug 871428. I hope to check the rebased results again before giving you review+. Thanks!

::: dom/alarm/AlarmService.jsm
@@ +43,4 @@
>      let alarmHalService = this._alarmHalService = Cc["@mozilla.org/alarmHalService;1"].getService(Ci.nsIAlarmHalService);
>      alarmHalService.setAlarmFiredCb(this._onAlarmFired.bind(this));
>      alarmHalService.setTimezoneChangedCb(this._onTimezoneChanged.bind(this));
> +    Services.obs.addObserver(this, "webapps-clear-data", false);

You need to remove the observer topic when shutting down. Recently, we just land an uninit() for it (Bug 871428). You need to rebase your codes and add the following line there:

Services.obs.removeObserver(this, "webapps-clear-data");

@@ +72,5 @@
> +      case "webapps-clear-data":
> +        this._db.getAll(
> +          data.manifestURL,
> +          function getAllSuccessCb(aAlarms) {
> +            aAlarms.forEach(function removeAlarm(aAlarms) {

s/aAlarms/aAlarm because you only deal with one alarm in this callback.

@@ +74,5 @@
> +          data.manifestURL,
> +          function getAllSuccessCb(aAlarms) {
> +            aAlarms.forEach(function removeAlarm(aAlarms) {
> +              this._removeAlarmFromDb(
> +                aAlarms.id,

s/aAlarms/aAlarm

@@ +77,5 @@
> +              this._removeAlarmFromDb(
> +                aAlarms.id,
> +                function removeSuccessCb() {
> +                  clearAlarm.bind(this,aAlarms.id,data.manifestURL);
> +                }

Please rewrite it as:

function removeSuccessCb() {
  this.clearAlarm(aAlarm.id, data.manifestURL);
}.bind(this)

Notes:
1. s/aAlarms/aAlarm
2. You need .bind(this), otherwise the |this| in the callback cannot be recognized.
3. It's fine to use func.bind(this, ...) but I'd prefer this.func(...).
4. Please add spaces among parameters.

@@ +78,5 @@
> +                aAlarms.id,
> +                function removeSuccessCb() {
> +                  clearAlarm.bind(this,aAlarms.id,data.manifestURL);
> +                }
> +              )

);

Please add a |;| at the end of a statement.

@@ +79,5 @@
> +                function removeSuccessCb() {
> +                  clearAlarm.bind(this,aAlarms.id,data.manifestURL);
> +                }
> +              )
> +            },this);

Add a space before |this|.

@@ +200,4 @@
>            json.id,
>            json.manifestURL,
>            function removeSuccessCb() {
> +            clearAlarm.bind(this,json.id,json.manifestURL);

this.clearAlarm(json.id, json.manifestURL)

@@ +379,4 @@
>    },
>  }
>  
> +function clearAlarm(id,manifestURL) {

Add spaces among parameters.

Recently, we just had lots of code changes at Bug 862322. Some comments within this function are modified. You need to rebase your patch on top of that.

Also, please s/clearAlarm/_removeAlarm everywhere and put _removeAlarm(...) into the scope of this.AlarmService to make it as a private member function.

Comment 33

[Sebastian Wong]

Attachment: Attempt at patch v4.

Comment 34

[Sebastian Wong]

Comment on attachment 750874 [details] [diff] [review]
attempt at patch v3.

Bug 862322 added a remove function that is called within "AlarmsManager:Remove".

Comment 35

[Sebastian Wong]

Attachment: Attempt at patch v4. redone

The latest version of the code has already refactored the "remove" case code. I also merged the current observe function with my observe function.

Comment 36

[Gene Lian (I already quit Mozilla)]

Comment on attachment 751172 [details] [diff] [review]
Attempt at patch v4. redone

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

Thanks for merging the codes. However, the new patch becomes wrong.

::: dom/alarm/AlarmService.jsm
@@ +58,4 @@
>  
>      alarmHalService.setAlarmFiredCb(this._onAlarmFired.bind(this));
>      alarmHalService.setTimezoneChangedCb(this._onTimezoneChanged.bind(this));
> +    Services.obs.addObserver(this, "webapps-clear-data", false);

Please move this line up to the next line of:

Services.obs.addObserver(this, "profile-change-teardown", false);

which is easier to keep track of all the added observers.

@@ +92,5 @@
> +      case "webapps-clear-data":
> +        this._db.getAll(
> +          data.manifestURL,
> +          function getAllSuccessCb(aAlarm) {
> +            aAlarm.forEach(function removeAlarm(aAlarm) {

Please use |aAlarms| for the getAllSuccessCb and use |aAlarm| for the removeAlarm, since the former is dealing with multiple alarms for latter is dealing with single alarm.

@@ +98,5 @@
> +                aAlarm.id,
> +                function removeSuccessCb() {
> +                  this.remove(aAlarm.id, data.manifestURL);
> +                }.bind(this);
> +              );

This is not right. The .remove(...) has called ._removeAlarmFromDb(...) in itself. You should simply call:

this.remove(aAlarm.id, data.manifestURL);

Comment 37

[Sebastian Wong]

Attachment: added changes

Comment 38

[Gene Lian (I already quit Mozilla)]

Hi Sebastian,

The patch looks good. However, it seems this patch wasn't created by HG because I didn't see any HG header on the top of this patch. Regarding how to make HG patch, please refer to [1] to learn how to use HG to create patches for review. I believe you shouldn'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.

Could you please come back with a new patch created by HG? Thanks!

[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

Comment 39

[Sebastian Wong]

Attachment: Created patch with hg

Here you go! I think I created the patch correctly. Please let me know if its wrong.

Comment 40

[Gene Lian (I already quit Mozilla)]

Comment on attachment 752032 [details] [diff] [review]
Created patch with hg

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

Wait... Have you tested your codes by practically removing an App and see if the AlarmService can receive a notification from that? Please check Webapps.jsm to see how it fires the "webapps-clear-data" notification, which doesn't use the |data|, |subject| instead. Also, you probably need to use .getManifestURLByLocalId() (in AppsService.js) to convert the subject.appId into the manifest URL to do the comparison.

This patch is impossible to work. Sorry, give r- again. :O Btw, the HG header looks good!

Comment 41

[Gene Lian (I already quit Mozilla)]

Hi Fabrice,

We're going to catch the "webapps-clear-data" observer topic in the AlarmService. We need to know that app's manifest URL. However, the observer subject only carries appId. I'm wondering at this moment can we still use .getManifestURLByLocalId() to get its manifest URL when that app has been killed?

Comment 42

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] from comment #41)
> I'm wondering at this moment can we still use
> .getManifestURLByLocalId() to get its manifest URL when that app has been
> killed?
  ^^^^^^ Sorry I mean "uninstalled".

Comment 43

[Sebastian Wong]

Would it look something like this? 
      case "webapps-clear-data":
        manifestURL = appsService.getManifestURLByLocalId(subject.appId);
        this._db.getAll(
          manifestURL,
          function getAllSuccessCb(aAlarms) {
            aAlarms.forEach(function removeAlarm(aAlarm) {
              this.remove(aAlarm.id, manifestURL);
            }, this);
          }.bind(this),
          function getAllErrorCb(aErrorMsg) {
            throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
          }
        );
Sorry, I didn't realize changing the event would affect the data being passed in and I assumed it would just work. Bad assumptions... Thanks a ton for putting up with me so far!

Comment 44

[Gene Lian (I already quit Mozilla)]

Correct! but you need to create that appsService first.

Comment 45

[[:fabrice] Fabrice Desré]

(In reply to Gene Lian [:gene] from comment #41)
> Hi Fabrice,
> 
> We're going to catch the "webapps-clear-data" observer topic in the
> AlarmService. We need to know that app's manifest URL. However, the observer
> subject only carries appId. I'm wondering at this moment can we still use
> .getManifestURLByLocalId() to get its manifest URL when that app has been
> killed?

Yes, you can. We remove the app from the registry at the last step of the uninstall process.

Comment 46

[Sebastian Wong]

Attachment: Attempt at patch v5.

Added call to appsService.getManifestURLByLocalId

Comment 47

[Gene Lian (I already quit Mozilla)]

Comment on attachment 752397 [details] [diff] [review]
Attempt at patch v5.

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

Sorry, review- again because this patch couldn't be working. Close to be done!

::: dom/alarm/AlarmService.jsm
@@ +89,5 @@
> +        this.uninit();
> +        break;
> +
> +      case "webapps-clear-data":
> +        let appsService = Cc["@mozilla.org/AppsService;1"];

This is impossible to work. That would be appreciated if you could run you codes first before asking for a review. Please add the following lines on the top of the file:

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

The lazy getter means the service won't be actually instantiated until the first use.

Comment 48

[Sebastian Wong]

Attachment: current state

This is what I have so far. I've tried removing the clock and calendar apps and I haven't had any problems. I suspect I might be testing this wrong. Could you tell me how I should go about testing this thing if I'm doing it wrong?

Comment 49

[Gene Lian (I already quit Mozilla)]

Comment on attachment 753448 [details] [diff] [review]
current state

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

Basically, if you want to test your codes, you can set |const DEBUG = true;| in the

AlarmService.jsm
AlarmDB.jsm
AlarmsManager.js

and use |adb logcat| to check the log.

::: dom/alarm/AlarmService.jsm
@@ +84,5 @@
>  
>      this._restoreAlarmsFromDb();
>    },
>  
> +  observe: function(Subject, Topic, Data) {

Wait...

s/Subject/subject
s/Topic/topic
s/Data/data

Why don't you use your previous patch which didn't have this typo?

@@ +85,5 @@
>      this._restoreAlarmsFromDb();
>    },
>  
> +  observe: function(Subject, Topic, Data) {
> +    switch (Topic) {

s/Topic/topic

@@ +89,5 @@
> +    switch (Topic) {
> +      case "profile-change-teardown":
> +        this.uninit();
> +        break;
> +	  case "webapps-clear-data":

The alignment is bad. Please correct it. Please don't use TABs.

@@ +91,5 @@
> +        this.uninit();
> +        break;
> +	  case "webapps-clear-data":
> +		debug("webapps-clear-data");
> +        manifestURL = appsService.getManifestURLByLocalId(subject.appId);

I just tested your codes. You should do:

let params =
  subject.QueryInterface(Ci.mozIApplicationClearPrivateDataParams);
let manifestURL = appsService.getManifestURLByLocalId(params.appId);

@@ -496,5 @@
> -    switch (aTopic) {
> -      case "profile-change-teardown":
> -        this.uninit();
> -    }
> -  },

Why don't you add your new codes in the existing observe()? Please move your codes to here by extending the switch-case here.

Comment 50

[Sebastian Wong]

Attachment: Attempt at patch v6.

Okay I hope to god this doesn't contain any errors. I tried running the code and I haven't had any issues. Really sorry if it does.

Comment 51

[Gene Lian (I already quit Mozilla)]

Comment on attachment 754065 [details] [diff] [review]
Attempt at patch v6.

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

Great! Please go ahead to land this. :)

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/6a4bbef0f572

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a4bbef0f572

Comment 54

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Gene, would be nice to try for approval-b2g18 on this. I don't know the testing field, so if you could fill out the approval request.

Comment 55

[Gene Lian (I already quit Mozilla)]

Sounds good. I'll do this later.