749551 - (alarm-api) Alarm API

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

As discussed in https://groups.google.com/forum/#!topic/mozilla.dev.webapi/pkx1uz_pnhQ/discussion I think we should add an API to schedule a notification or wake-up event for an application to be started, at a specific time.

The alarm should be stored persistently and the application needs a way to register and unregister the alarm, maybe use the url as identifier.

I will post the proposal to dev-webapi.

Comment 1

[Gene Lian (I already quit Mozilla)]

In the initiative step, the following functions are going to be added in hal/gonk to support the fundamental alarm functionality:

int InitAlarm();
void CloseAlarm(int fd);
void SetAlarm(int fd, int type, long seconds, long nanoseconds);
int SetAlarmKernelTimezone(int fd, int minswest);
int WaitForAlarm(int fd);

which are corresponding to the current Android alarm service JNI: http://androidxref.com/source/xref/frameworks/base/services/jni/com_android_server_AlarmManagerService.cpp

Gene

Comment 2

[Mounir Lamouri (:mounir)]

We should make hal matches real high-level needs. That means we shouldn't add into Hal.h methods that are only going to be needed by the gonk implementation.

Comment 3

[Gene Lian (I already quit Mozilla)]

Mounir,

I agree :) I only listed some lower-layer functions that could probably be needed for upper layers based on what Android has already done. In the long run, all the WebAPIs for high-level needs will be supported as well to meet our goals ;)

Gene

Comment 4

[Gene Lian (I already quit Mozilla)]

The proposal for Alarm DOM interfaces are summarized by Kanru at: http://groups.google.com/group/mozilla.dev.webapi/browse_thread/thread/a64c75bb3fe99e14/bae1a2fc6637b782?lnk=gst&q=alarm+API#bae1a2fc6637b782 which are currently under construction.

Gene

Comment 5

[Gene Lian (I already quit Mozilla)]

Attachment: WIP Patch v.1

Uploaded a WIP patch which contains:

1. hal/gonk has been implemented
2. provided a psuedo DOM interface

The core back-end logic is still under construction. Please feel free to fire any questions or suggestions.

Thanks,
Gene

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 622145 [details] [diff] [review]
WIP Patch v.1

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

Just a quick go through.

::: dom/alarm/Alarm.cpp
@@ +32,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

Use MPL 2.0 headers. https://www.mozilla.org/MPL/headers/

@@ +43,5 @@
> +
> +/**
> +  * We have to use macros here because our leak analysis tool things we are
> +  * leaking strings when we have |static const nsString|. Sad :(
> +  */

Leftover comment?

::: dom/alarm/Alarm.h
@@ +32,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

MPL2.0 here too.

@@ +53,5 @@
> +  Alarm() {};
> +  virtual ~Alarm() {};
> +  
> +private:
> +  int mId;

Probably PRUint32 here.

::: dom/alarm/AlarmService.cpp
@@ +32,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

MPL2.0 header here too.

::: dom/alarm/AlarmService.h
@@ +32,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

MPL2.0 header here too.

::: dom/alarm/AlarmsManager.cpp
@@ +32,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

MPL2.0 header here too.

::: dom/alarm/AlarmsManager.h
@@ +32,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

MPL 2.0 header here too.

::: dom/alarm/Makefile.in
@@ +31,5 @@
> +# and other provisions required by the GPL or the LGPL. If you do not delete
> +# the provisions above, a recipient may use your version of this file under
> +# the terms of any one of the MPL, the GPL or the LGPL.
> +#
> +# ***** END LICENSE BLOCK *****

...

::: dom/alarm/nsIAlarmService.idl
@@ +32,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

...

@@ +39,5 @@
> +
> +interface nsIDOMMozAlarm;
> +
> +%{C++
> +#define NS_ALARMSERVICE_CID { 0x18c2e238, 0x3a0a, 0x4153, {0x89, 0xfc, 0x16, 0x6b, 0x3b, 0x14, 0x65, 0xa1 } }

Do not reuse CID. Ask firebot on irc for a new CID.

@@ +43,5 @@
> +#define NS_ALARMSERVICE_CID { 0x18c2e238, 0x3a0a, 0x4153, {0x89, 0xfc, 0x16, 0x6b, 0x3b, 0x14, 0x65, 0xa1 } }
> +#define ALARMSERVICE_CONTRACTID "@mozilla.org/alarm/alarmservice;1"
> +%}
> +
> +[scriptable, builtinclass, uuid(8a8e667a-95bf-11e1-bf61-0050c2490048)]

This uuid is not correct. Please ask firebot on irc for a uuid.

::: dom/alarm/nsIDOMAlarm.idl
@@ +32,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

...

@@ +41,5 @@
> +interface nsIDOMDOMRequest;
> +interface nsIScriptableDateFormat;
> +interface nsIDOMEvent;
> +
> +[scriptable, builtinclass, uuid(8a8e667a-95bf-11e1-bf60-0050c2490048)]

Wrong uuid.

@@ +44,5 @@
> +
> +[scriptable, builtinclass, uuid(8a8e667a-95bf-11e1-bf60-0050c2490048)]
> +interface nsIDOMMozAlarm : nsISupports
> +{
> +    readonly attribute long id;

unsigned long

::: dom/alarm/nsIDOMAlarmsManager.idl
@@ +32,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

...

@@ +40,5 @@
> +interface nsIDOMDOMRequest;
> +interface nsIDOMEventListener;
> +interface nsIDOMMozAlarm;
> +
> +[scriptable, builtinclass, uuid(8a8e667a-95bf-11e1-bf62-0050c2490048)]

Wrong uuid.

::: dom/alarm/test/Makefile.in
@@ +31,5 @@
> +# and other provisions required by the GPL or the LGPL. If you do not delete
> +# the provisions above, a recipient may use your version of this file under
> +# the terms of any one of the MPL, the GPL or the LGPL.
> +#
> +# ***** END LICENSE BLOCK *****

...

::: dom/base/Navigator.cpp
@@ +993,5 @@
> +  if (!mAlarmsManager) {
> +    mAlarmsManager = new alarm::AlarmsManager();
> +  }
> +
> +  NS_ADDREF(*aAlarms = mAlarmsManager);

Use forget()

::: dom/base/nsDOMClassInfo.cpp
@@ +1444,5 @@
> +  NS_DEFINE_CLASSINFO_DATA(MozAlarmsManager, nsDOMGenericSH,
> +                           DOM_DEFAULT_SCRIPTABLE_FLAGS)
> +                           
> +  NS_DEFINE_CLASSINFO_DATA(MozAlarm, nsDOMGenericSH,
> +						   DOM_DEFAULT_SCRIPTABLE_FLAGS)

Too much indent.

::: dom/base/nsDOMClassInfoClasses.h
@@ +429,5 @@
>  DOMCI_CLASS(MozPowerManager)
>  DOMCI_CLASS(MozWakeLock)
>  
> +DOMCI_CLASS(MozAlarmsManager)
> +DOMCI_CLASS(MozAlarm)

Make sure your declaration order matches the one in nsDOMClassInfo.cpp

::: dom/interfaces/base/nsIDOMNavigator.idl
@@ +59,5 @@
> +  readonly attribute boolean               	onLine;
> +  readonly attribute DOMString             	buildID;
> +  readonly attribute DOMString             	doNotTrack;
> +  readonly attribute nsIDOMMozPowerManager 	mozPower;
> +  readonly attribute nsIDOMMozAlarmsManager	mozAlarms;

Every time you change a interface you have to update uuid too.

Comment 7

[Gene Lian (I already quit Mozilla)]

Attachment: WIP Patch v.2

Kanru,

I've modified codes by following your review (thank you!). Please see the new patch when you have a chance. Also, I created a new event suggested by Mounir as below, which can be used to be fired toward the alarm event handler.

interface nsIDOMMozAlarmEvent : nsIDOMEvent
{
  readonly attribute nsIDOMMozAlarm alarm;
};

Thanks,
Gene

Comment 8

[Gene Lian (I already quit Mozilla)]

Attachment: WIP Patch v.3

Hi guys,

I have pretty much done on the Alarm APIs, which can support the following features based on our previous conclusions :) Please see the latest WIP patch and feel free to let me know if you have any other concerns.

1. Web app developer can arbitrarily set multiple alarms and get a unique ID for each alarm to remove it later.

2. Web app developer can also set some customized data regarding to each alarm.

3. All the alarms that have been set can be automatically restored even if a device reboot is encountered since the back-end service is maintained by an indexedDB database.

4. An alarm message will be fired when the system goes to the specified alarm time (we still need the system intent architecture to send the message back to app; so far I'm just dumping some info to make sure alarms are able to go off successfully). Afterwards, the system will automatically be reset to the next valid alarm.

5. Most of the alarm behaviors are done by async codes and multiple threads, which means users should be able to keep doing other things when the alarm is concurrently being added, removed, queried, listened and gone off.

I'm drafting an diagram to illustrate the architecture design and will be uploading it later, which can support the reviewers do the code reviews.

Thanks,
Gene

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 625908 [details] [diff] [review]
WIP Patch v.3

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

::: dom/alarm/AlarmHalService.cpp
@@ +10,5 @@
> +namespace alarm {
> +
> +NS_IMPL_ISUPPORTS1(AlarmHalService, nsIAlarmHalService)
> +
> +int AlarmHalService::sAlarmFd = -1;

Why do we still have this file descriptor at this layer? We need a more abstract HAL API.

Comment 10

[Gene Lian (I already quit Mozilla)]

(In reply to Kan-Ru Chen [:kanru] from comment #9)
> Comment on attachment 625908 [details] [diff] [review]
> WIP Patch v.3
> 
> Review of attachment 625908 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/alarm/AlarmHalService.cpp
> @@ +10,5 @@
> > +namespace alarm {
> > +
> > +NS_IMPL_ISUPPORTS1(AlarmHalService, nsIAlarmHalService)
> > +
> > +int AlarmHalService::sAlarmFd = -1;
> 
> Why do we still have this file descriptor at this layer? We need a more
> abstract HAL API.

Because it seems to me the GonkHal.cpp is a collection of HAL function calls, I originally thought we need an AlarmHalService object to utilize these functions. This object is responsible for opening the dev/alarm file, saving the file descriptor and automatically closing it in the destructor. 

I think I can still hide the file descriptor in the GonkHal.cpp as a static variable. As you suggested, in other platforms like Windows, they might not use a similar file descriptor to control the alarm settings. I'll have this change in the next patch.

Thanks,
Gene

Comment 11

[Gene Lian (I already quit Mozilla)]

Attachment: Alarm API Architecture

For the convenience of code reviews, I provide a PDF to illustrate the overview of Alarm API architecture.

Gene

Comment 12

[[:fabrice] Fabrice Desré]

Gene,

Do you mind splitting up your patch in different parts? That would help reviews, since different people will likely review different parts. For instance you could do:
- IDL
- hal / gonk
- js parts

Comment 13

[Gene Lian (I already quit Mozilla)]

Fabrice,

My pleasure :) I'll try to split it up in the next patch. Please stay tuned. Thanks for your suggestions.

Gene

Comment 14

[Gene Lian (I already quit Mozilla)]

I'll have this done tomorrow. I'm having some building errors with the latest Gecko merge.

Gene

Comment 15

[Gene Lian (I already quit Mozilla)]

Attachment: Part1, IDL and dummy DOM

Jonas,

I'd like to invite you to review the IDL part, where the DOM interface is defined by our previous conclusions. In addition, I also designed a nslAlarmHalService.idl which plays an role of IDL interface between the upper-layer codes (.js) and the hal/gonk codes (.cpp). You can also find an architecture overview in the attachment for your convenience of code review.

Thanks,
Gene

Comment 16

[Gene Lian (I already quit Mozilla)]

Attachment: Part2, Hal/Gonk

Chris,

I'd like to invite you to review the Hal/Gonk part which provides necessary HAL APIs for upper layers to initialize/set/wait/close alarms in the system. You can find more details in the comments of Hal.h to understand how the clients are able to utilize these APIs. Also, you can find an architecture overview in the attachment for your convenience of code review.

Thanks,
Gene

Comment 17

[Gene Lian (I already quit Mozilla)]

Attachment: Part3, AlarmHalService

Mounir,

I'd like to invite you to review the implementation of AlarmHalService part which plays a role of IDL interface between upper-layer codes (.js) and the hal/gonk codes (.cpp). This part is in charge of managing the resources for setting and listening alarms by hiding some details to upper layers. You can also find the architecture overview in the attachment for your convenience of code review.

Thanks,
Gene

Comment 18

[Gene Lian (I already quit Mozilla)]

Attachment: Part4, AlarmDB

Vivien and Mounir,

I'd like to invite you to review the AlarmDB part. This is an independent JS module for accessing the alarm database powered by the indexedDB as a back end. We need such an off-line database because the alarms have to be restored whenever the device is rebooted. You can also find the architecture overview in the attachment for your convenience of code review.

Thanks,
Gene

Comment 19

[Gene Lian (I already quit Mozilla)]

Attachment: Part5, Alarm DOM

Vivien and Mounir,

I'd like to invite you to review the Alarm DOM implementation part. This is the core part utilizing all the functions and modules defined in the previous patches. Basically, this part provides an asynchronizing communication between multiple AlarmsManager and the central AlarmService, where the former provides DOM implementation exposed to web apps and the latter provides a back-end service for accessing AlarmDB and Hal/Gonk. AlarmService is also in charge of performing the alarm resetting whenever an alarm had been fired, added or removed. Also, you can find the architecture overview in the attachment for your convenience of code review.

Thanks,
Gene

Comment 20

[Gene Lian (I already quit Mozilla)]

Attachment: Alarm API Architecture

Comment 21

[Mounir Lamouri (:mounir)]

Comment on attachment 627135 [details] [diff] [review]
Part4, AlarmDB

I'm no super-reviewer. Redirecting that to Jonas.

Comment 22

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 627135 [details] [diff] [review]
Part4, AlarmDB

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

::: dom/alarm/AlarmDB.jsm
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const EXPORTED_SYMBOLS = ["AlarmDB", "ALARMDB_NAME", "ALARMSTORE_NAME"];

No need to expose ALARMDB_NAME nor ALARMSTORE_NAME

@@ +6,5 @@
> +
> +const EXPORTED_SYMBOLS = ["AlarmDB", "ALARMDB_NAME", "ALARMSTORE_NAME"];
> +
> +/* static functions */
> +const DEBUG = true;

make it false by default

@@ +12,5 @@
> +if (DEBUG) {
> +  debug = function (s) { dump("AlarmDB: " + s + "\n"); }
> +} else {
> +  debug = function (s) {}
> +}

function debug() {
  if (DEBUG)
    dump("AlaramDB: " + s + "\n");
}

No need to overcomplexify that. Also 'debug' is not declared in the previous code.

@@ +104,5 @@
> +        if (!txn.result) {
> +          txn.result = {};
> +        }
> +        
> +        store.getAll().onsuccess = function (event) {

Add a name to the anonynous function.
Also it seems like there is a mix of arguments naming conventions. Sometimes they are prefixed with 'a', sometimes not. Let's choose one :)

@@ +128,5 @@
> +      "readonly", 
> +      function (txn, store) {
> +        txn.result = 0;
> +
> +        store.openCursor(null, "prev").onsuccess = function (event) {

nit: add a name to the anonymous function.

@@ +134,5 @@
> +          if (!cursor) {
> +            debug("Could not get the last key from database. Probably empty database");
> +            txn.result = 0;
> +          }
> +          else {

At the beginning of the file, you were using

if () {
} else {
}

here you use

if () {
}
else {
}

let's choose one.

Comment 23

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 627140 [details] [diff] [review]
Part5, Alarm DOM

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

Sounds good but there is a lot of confusions on the JS side between bind/this/self and this needs to be cleaned up. 

Quick question: what happen if there is an alarm set and the current time of the device is changed to be in the future. Does the code will be stuck on this alarm until the next reboot?

::: dom/alarm/AlarmService.jsm
@@ +10,5 @@
> +if (DEBUG) {
> +  debug = function(s) { dump("AlarmService: " + s + "\n"); };
> +} else {
> +  debug = function(s) {};
> +}

See my comment for AlarmsManager.js

@@ +42,5 @@
> +    messages.forEach(
> +      function (msgName) {
> +        self._ppmm.addMessageListener(msgName, this);
> +      }.bind(this)
> +    );

messages.forEach(function addListener(name) {
  this._ppmm.addMessageListener(msgName, this);
}.bind(this));

If you're using bind, there is no need for declaring 'let self = this' above.

@@ +51,5 @@
> +    this._db = new AlarmDB(myGlobal);
> +    this._db.init(myGlobal);
> +    
> +    // variable to save the current set alarm in system
> +    this._curSetAlarm = null;

_curSetAlarm -> _currentAlarm

@@ +59,5 @@
> +    this._db.getLastId(
> +      function getLastIdSuccessCb(aLastId) {
> +        debug("Callback after getting the last ID from database: " + aLastId);
> +        self._lastId = aLastId;
> +      }.bind(this),

self -> this

@@ +62,5 @@
> +        self._lastId = aLastId;
> +      }.bind(this),
> +      function getLastIdErrorCb(aErrorMsg) {
> +        throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
> +      }.bind(this)

nit: there is no reason to use bind here.

@@ +66,5 @@
> +      }.bind(this)
> +    );
> +                       
> +    // variable to restore the alarms from database
> +    this._alarmQueue = [];

let alarmQueue = this._alarmQueue = [];

 and use it in your callbacks instead of self/bind.

@@ +71,5 @@
> +    this._db.get(
> +      function getSuccessCb(aAlarms) {
> +        debug("Callback after getting alarms from database: " + JSON.stringify(aAlarms));
> +        // only add the alarm that is valid
> +        let curSysTime = (new Date()).getTime();

Date.now()

@@ +78,5 @@
> +          function (aAlarm) {
> +            if (new Date(aAlarm.date).getTime() > curSysTime) {
> +              self._alarmQueue.push(aAlarm);
> +            }
> +          }.bind(this)

aAlarms.forEach(function add(aAlarm) {
  alarmQueue.push(aAlarm);
});

@@ +99,5 @@
> +
> +      }.bind(this),
> +      function getErrorCb(aErrorMsg) {
> +        throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
> +      }.bind(this)

nit: no needs for bind here.

@@ +111,5 @@
> +      
> +    debug("receiveMessage(): " + aMessage.name);
> +    
> +    let self = this;
> +    let msg = aMessage.json;

msg -> json

@@ +125,5 @@
> +            ); 
> +          }.bind(this),
> +          function getErrorCb(aErrorMsg) {
> +            this._ppmm.sendAsyncMessage(
> +              "AlarmsManager:Get:Return:NO", 

NO -> KO (this is a convention used in some APIs)

@@ +159,5 @@
> +              // if there is no alarm being set in system, set the new alarm
> +              self._curSetAlarm = newAlarm;
> +              if (!self._alarmHalService.setAlarm(newAlarmTime / 1000, 0)) {
> +                throw Components.results.NS_ERROR_FAILURE;
> +              }

Add return; and avoid the else

@@ +168,5 @@
> +              if (newAlarmTime < curSetAlarmTime) {
> +                // the new alarm is earlier than the current set alarm
> +                // swap them and push the previous set alarm in the queue
> +                self._alarmQueue.push(self._curSetAlarm);
> +                self._alarmQueue.sort(self._sortAlarmQueueFunc);

alarmQueue.unshift(self._curSetAlarm); should be enough is the array is always sorted, right?

@@ +187,5 @@
> +
> +          }.bind(this), 
> +          function addErrorCb(aErrorMsg) {
> +            throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
> +          }.bind(this)

nit: no need for bind.

@@ +231,5 @@
> +                if (self._alarmQueue[i].id == msg.id) {
> +                  self._alarmQueue.splice(i, 1);
> +                  break;
> +                }
> +              }

Move this block before (self._curSetAlarm.id == msg.id) under the condition (self._curSetAlarm.id !== msg.id) and do an early return once it is executed to avoid the else.

@@ +240,5 @@
> +            
> +          }.bind(this), 
> +          function removeErrorCb(aErrorMsg) {
> +            throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
> +          }.bind(this)

nit: no need for bind

@@ +255,5 @@
> +    debug("observe(): " + topic);
> +    
> +    switch (topic) {
> +      case "alarm-fired":
> +        let curSysTime = (new Date()).getTime();

Date.now()

@@ +261,5 @@
> +        if (this._curSetAlarm) {
> +          debug("Fire system intent: " + JSON.stringify(this._curSetAlarm));
> +          {
> +            // fire system intent: this._curSetAlarm
> +          }

nit: fire system intent -> TODO Fire a system message, see bug 755245 
nit: remove the {}

@@ +265,5 @@
> +          }
> +          this._curSetAlarm = null;
> +        }
> +
> +        // reset the next alarm from the queue

let alarmQueue = this._alarmQueue;
while (alarmQueue.length > 0) {
  let nextAlarm = alarmQueue.shift();
  ...
}

@@ +276,5 @@
> +          if (nextAlarmTime <= curSysTime) {
> +            debug("Fire system intent: " + JSON.stringify(nextAlarm));
> +            {
> +              // fire system intent: nextAlarm
> +            }

nit: fire system intent -> TODO Fire a system message, see bug 755245 
nit: remove the {}

@@ +286,5 @@
> +            }
> +            break;
> +          }
> +        }
> +        

} else {

::: dom/alarm/AlarmsManager.js
@@ +10,5 @@
> +if (DEBUG) {
> +  debug = function (s) { dump("AlarmsManager: " + s + "\n"); }
> +} else {
> +  debug = function (s) {}
> +}

const DEBUG = false;

function debug(s) {
  if (DEBUG)
    dump("AlarmsManager: " + s + "\n");
}

@@ +62,5 @@
> +
> +    return this._cpmm.sendSyncMessage(
> +      "AlarmsManager:Add", 
> +      { date: aDate, ignoreTimezone: isIgnoreTimezone, data: aData }
> +    );

The idl says 'unsigned long add' but here you will return an array. You likely want to return the first value only.

@@ +88,5 @@
>  
> +  receiveMessage: function receiveMessage(aMessage) {
> +    debug("receiveMessage(): " + aMessage.name);
> +
> +    let msg = aMessage.json;

msg -> json

@@ +89,5 @@
> +  receiveMessage: function receiveMessage(aMessage) {
> +    debug("receiveMessage(): " + aMessage.name);
> +
> +    let msg = aMessage.json;
> +    let request = this.getRequest(msg.requestID);

Seems like you can do an early return if there is no request. The code above does nothing in this case.

::: dom/alarm/Makefile.in
@@ +22,4 @@
>  
>  EXTRA_COMPONENTS =       \
>    AlarmsManager.js       \
>    AlarmsManager.manifest \

You also want to add this file to b2g/installer/package.manifest otherwise the component will not be pushed on the device.

Comment 24

[Gene Lian (I already quit Mozilla)]

(In reply to Vivien Nicolas (:vingtetun) from comment #23)

First of all, thanks for your detailed review! Really appreciate :) And sorry for these improper coding conventions (actually, I'm just a one-month freshman working for Mozilla and this is my first big task though).

> Comment on attachment 627140 [details] [diff] [review]
> Part5, Alarm DOM
> 
> Review of attachment 627140 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sounds good but there is a lot of confusions on the JS side between
> bind/this/self and this needs to be cleaned up. 
> 
> Quick question: what happen if there is an alarm set and the current time of
> the device is changed to be in the future. Does the code will be stuck on
> this alarm until the next reboot?

This is a very interesting question that I didn't consider ;) I'll practically give it a try tomorrow to see if we need to deal with this case. My first guessing is it should be fine since the block IO for waiting alarm would return immediately when we set an old-time alarm, thus firing a system message. However, it would be better to test the case of directly changing system time to be in the future which is a kind of opposite way.

> @@ +62,5 @@
> > +
> > +    return this._cpmm.sendSyncMessage(
> > +      "AlarmsManager:Add", 
> > +      { date: aDate, ignoreTimezone: isIgnoreTimezone, data: aData }
> > +    );
> 
> The idl says 'unsigned long add' but here you will return an array. You
> likely want to return the first value only.

Please see the case dealing with "AlarmsManager:Add" in the AlarmService.jsm. Doesn't it return the new ID at the bottom of the case? In the JS console, it can also return me and display the correct new ID. Please let me know if I wrongly use the sendSyncMessage(). :)

      case "AlarmsManager:Add":
        // generate a unique ID to specity the new alarm
        let curDate = new Date();
        let newId = (++this._lastId);

        // prepare a record for the new alarm
        let newAlarm = { id: newId, date: msg.date, ignoreTimezone: msg.ignoreTimezone, data: msg.data };

        this._db.add(
        ...
        );

        return newAlarm.id;
        break;

> ::: dom/alarm/Makefile.in
> @@ +22,4 @@
> >  
> >  EXTRA_COMPONENTS =       \
> >    AlarmsManager.js       \
> >    AlarmsManager.manifest \
> 
> You also want to add this file to b2g/installer/package.manifest otherwise
> the component will not be pushed on the device.

I've already had them in my Part 1 patch for IDL and dummy DOM. Sorry for the misunderstanding.

@BINPATH@/components/AlarmsManager.js
@BINPATH@/components/AlarmsManager.manifest


I should be able to have all the changes based on your suggestions tomorrow. Please stay tuned :)

Thanks,
Gene

Comment 25

[Mounir Lamouri (:mounir)]

Comment on attachment 627127 [details] [diff] [review]
Part1, IDL and dummy DOM

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

Implementing nsIDOMAlarmsManager.idl in JS seems to be a bad design: you need to call hal:: methods from that implementation which can't be called in JS so you need that service in C++. Implementing the manager in C++ would prevent creating that service.

Comment 26

[Mounir Lamouri (:mounir)]

Comment on attachment 627131 [details] [diff] [review]
Part3, AlarmHalService

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

In addition of the few commetns below: the class name is wrong, it shouldn't be AlarmHalService but AlarmService.
Anyhow, you should implement nsIDOMAlarmManager in C++ so you will not have to use that service and directly call the hal methods from the native implementation.

::: dom/alarm/AlarmHalService.cpp
@@ +8,5 @@
> +#if (defined(DEBUG) && defined(MOZ_WIDGET_GONK))
> +  #define debug(args...) __android_log_print(ANDROID_LOG_INFO, "AlarmHalService", args)
> +#else
> +  #define debug(args...) {}
> +#endif

Why can't you use NS_ERROR and NS_WARNING as everywhere else in Gecko?

@@ +18,5 @@
>  NS_IMPL_ISUPPORTS1(AlarmHalService, nsIAlarmHalService)
>  
>  AlarmHalService::AlarmHalService()
>  {
> +  debug("AlarmHalService()");

Remove that.

@@ +23,5 @@
> +  
> +  mAlarmInitialized = hal::InitAlarm();
> +  
> +  if (mAlarmInitialized) {
> +    NS_NewThread(getter_AddRefs(mAlarmFireWatcherThread), new AlarmFireWatcher());

Do you ever kill that thread?

@@ +25,5 @@
> +  
> +  if (mAlarmInitialized) {
> +    NS_NewThread(getter_AddRefs(mAlarmFireWatcherThread), new AlarmFireWatcher());
> +  } else {
> +    debug("Unable to initialize alarm.");

This should be NS_WARNING or NS_ERROR.

@@ +31,5 @@
>  }
>  
>  /* virtual */ AlarmHalService::~AlarmHalService() 
>  {
> +  debug("~AlarmHalService()");

Remove that.

@@ +35,5 @@
> +  debug("~AlarmHalService()");
> +  
> +  if (mAlarmInitialized) {
> +    if (!hal::CloseAlarm()) {
> +      debug("Unable to close alarm.");

NS_ERROR

@@ +44,5 @@
>  
>  /* static */ already_AddRefed<nsIAlarmHalService>
>  AlarmHalService::GetInstance()
>  {
> +  debug("GetInstance()");

Remove that.

@@ +53,5 @@
>  
>  NS_IMETHODIMP
>  AlarmHalService::SetAlarm(PRInt32 aSeconds, PRInt32 aNanoseconds, bool *aStatus)
>  {
> +  debug("SetAlarm()");

... and that.
... and all the following debug().

@@ +119,5 @@
> +AlarmFireWatcher::Run()
> +{
> +  while (true) {
> +    if (hal::WaitForAlarm()) {
> +      NS_DispatchToMainThread(mAlarmFireNotifier);

Can't you just instantiate the Runnable here before dispatching it?

::: dom/alarm/AlarmHalService.h
@@ +32,5 @@
> +  bool mAlarmInitialized;
> +  nsCOMPtr<nsIThread> mAlarmFireWatcherThread;
> +};
> +
> +class AlarmFireNotifier : public nsRunnable 

Please, move this inside the previous class.

@@ +39,5 @@
> +  AlarmFireNotifier();
> +  NS_IMETHOD Run();
> +};
> +
> +class AlarmFireWatcher : public nsRunnable 

ditto

@@ +46,5 @@
> +  AlarmFireWatcher();
> +  NS_IMETHOD Run();
> +  
> +private:
> +  nsRefPtr<AlarmFireNotifier> mAlarmFireNotifier;

You don't need that.

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Does this API work from content processes?

Will try to review soon.

Comment 28

[Gene Lian (I already quit Mozilla)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #26)
> Comment on attachment 627131 [details] [diff] [review]
> Part3, AlarmHalService
> 
> Review of attachment 627131 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In addition of the few commetns below: the class name is wrong, it shouldn't
> be AlarmHalService but AlarmService.
> Anyhow, you should implement nsIDOMAlarmManager in C++ so you will not have
> to use that service and directly call the hal methods from the native
> implementation.

Thanks for your review and comment! Really appreciate :) 

Regarding this concern, it's a trade-off between C++ and JS for implementing the AlarmService, since the AlarmService highly relies on the indexedDB as a back-end database which seems more easily to be used with JS codes (you may refer to the Part4 patch to see how the AlarmDB.jsm uses the IndexedDBHelper). That's why the main AlarmsManager and AlarmService are written by JS (please see the Part5 patch). 

To communicate with Hal/Gonk, another AlarmHalService is created, which not only in charge of the C++/JS interface but also internally initializing, setting and creating thread for waiting on alarm. Therefore, the AlarmHalService seems to be a more independent component separate from the AlarmService. That is, the AlarmHalService can be even reused by any other callers in addition to the AlarmService if needed in the future. 

May I have your compromise to keep the current design? :) I think I can change the AlarmHalService to another better name to avoid misunderstanding (like "AlarmHal" simply?), since the core "service" for AlarmsManager is actually maintained by the existing AlarmService already in Part5 patch.

Hopefully the above concerns sound good to you. :) The other comments sound reasonable to me and I should be able to have these changes in a couple of days.

Thanks for your review again!

Gene

Comment 29

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] from comment #28)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #26)
> > Comment on attachment 627131 [details] [diff] [review]
> > Part3, AlarmHalService
> > 
> > Review of attachment 627131 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > In addition of the few commetns below: the class name is wrong, it shouldn't
> > be AlarmHalService but AlarmService.
> > Anyhow, you should implement nsIDOMAlarmManager in C++ so you will not have
> > to use that service and directly call the hal methods from the native
> > implementation.
> 
> Thanks for your review and comment! Really appreciate :) 
> 
> Regarding this concern, it's a trade-off between C++ and JS for implementing
> the AlarmService, since the AlarmService highly relies on the indexedDB as a
> back-end database which seems more easily to be used with JS codes (you may
> refer to the Part4 patch to see how the AlarmDB.jsm uses the
> IndexedDBHelper). That's why the main AlarmsManager and AlarmService are
> written by JS (please see the Part5 patch). 
> 
> To communicate with Hal/Gonk, another AlarmHalService is created, which not
> only in charge of the C++/JS interface but also internally initializing,
> setting and creating thread for waiting on alarm. Therefore, the
> AlarmHalService seems to be a more independent component separate from the
> AlarmService. That is, the AlarmHalService can be even reused by any other
> callers in addition to the AlarmService if needed in the future. 
> 
> May I have your compromise to keep the current design? :) I think I can
> change the AlarmHalService to another better name to avoid misunderstanding
> (like "AlarmHal" simply?), since the core "service" for AlarmsManager is
> actually maintained by the existing AlarmService already in Part5 patch.
> 
> Hopefully the above concerns sound good to you. :) The other comments sound
> reasonable to me and I should be able to have these changes in a couple of
> days.
> 
> Thanks for your review again!
> 
> Gene

Btw, another trade-off is it seems much easier to pass the JSON object data by using JS codes, not only between AlarmsManager and AlarmService, but also between AlarmService and AlarmDB. That's why I eventually chose JS codes.

Thanks,
Gene

Comment 30

[Mounir Lamouri (:mounir)]

Can't you have the entire implementation in C++ except AlarmDB.jsm, assuming this part really needs to be implemented in JS.
You seems like you *want* to use JS but you *have* to you C++ for the thread handling. That makes the entire architecture of that code completely non-understandable and I'm pretty sure you can convert AlarmService.jsm in C++. AlarmsManager.js would be trivial to move in C++.

Comment 31

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #30)
> Can't you have the entire implementation in C++ except AlarmDB.jsm, assuming
> this part really needs to be implemented in JS.

Only the operating system interfacing part (HAL) has to be in C/++, sadly we cannot use that directly in JS.

> You seems like you *want* to use JS but you *have* to you C++ for the thread
> handling. That makes the entire architecture of that code completely
> non-understandable and I'm pretty sure you can convert AlarmService.jsm in
> C++. AlarmsManager.js would be trivial to move in C++.

Wouldn't that generating a lot of code in order to support multiple process?

Comment 32

[Mounir Lamouri (:mounir)]

(In reply to Kan-Ru Chen [:kanru] from comment #31)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #30)
> > Can't you have the entire implementation in C++ except AlarmDB.jsm, assuming
> > this part really needs to be implemented in JS.
> 
> Only the operating system interfacing part (HAL) has to be in C/++, sadly we
> cannot use that directly in JS.

This is exactly why I'm saying you should implement this in C++: given that you have a part that needs to be in C++, better to maximize the code in C++. Having features half implemented in C++ and half in JS is a pain and make everything hard to read.

> > You seems like you *want* to use JS but you *have* to you C++ for the thread
> > handling. That makes the entire architecture of that code completely
> > non-understandable and I'm pretty sure you can convert AlarmService.jsm in
> > C++. AlarmsManager.js would be trivial to move in C++.
> 
> Wouldn't that generating a lot of code in order to support multiple process?

I don't know the specifics of AlarmsManager but hal should handle the multi-process hassle for you. Look at BatteryManager: the implementation is supporting multi-process, it is in C++ and it is yet simple to read and understand. I'm not saying AlarmsManager is as simple as that but multi-process shouldn't be an issue unless there is a specific problem that needs to be handled.

Comment 33

[Thinker Li [:sinker]]

Comment on attachment 627131 [details] [diff] [review]
Part3, AlarmHalService

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

::: dom/alarm/AlarmHalService.cpp
@@ +109,5 @@
> +  return NS_OK;
> +}
> +
> +AlarmFireWatcher::AlarmFireWatcher() :
> +  mAlarmFireNotifier(new AlarmFireNotifier())

You may want NS_NewRunnableMethod().

@@ +118,5 @@
> +NS_IMETHODIMP
> +AlarmFireWatcher::Run()
> +{
> +  while (true) {
> +    if (hal::WaitForAlarm()) {

Only main thread can call hal::WaitForAlarm(), right!?

::: dom/alarm/AlarmHalService.h
@@ +46,5 @@
> +  AlarmFireWatcher();
> +  NS_IMETHOD Run();
> +  
> +private:
> +  nsRefPtr<AlarmFireNotifier> mAlarmFireNotifier;

You can merge AlarmFireNotifier and AlarmFireWatcher into one class, and use NS_NewRunnableMethod() to dispatch methods.

Comment 34

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Hi Gene,

Most of this approach looks OK, nice job!  Thanks for the good
documentation.

There are some high-level issues with the implementation though.

The first is with the hal layer.  Since the alarm is "global", across
all Gecko processes, we should only allow the master process to
program it.  It doesn't make sense for a content process to directly
program the alarm, for example, because it can't know the deadlines of
all the other Gecko processes.

The second issue is with the threading model in the hal layer.  You're
exposing a blocking hal:: function directly, which is a little
confusing --- all the other hal:: functions only run on the main
thread.  We don't want to allow the main thread to block on a timer
callback.  This also "leaks abstractions" out of hal --- the most
efficient way to await an alarm notification might be asynchronous
(e.g. a signal for an itimer).  We should hide this in the hal layer
behind an asynchronous interface.  This will mean your timer-waiting
thread should live in GonkHal.

Since we're not using any timer interface except the RTC wakeup timer,
I would recommend not exposing any other through hal.  That will
simplify the code.  If we need to add the other types later, we can.

Let me know if this makes sense.  Thanks!

Comment 35

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Sorry, one more issue I forgot to mention:

With an asynchronous timer-notification interface, we'll need a way to notify timer-watcher(s) on the main thread.  We can re-use the existing IObserver mechnisms for this in hal, although they're somewhat overkill for this case since there should only be one listener at any time.

Comment 36

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 627130 [details] [diff] [review]
Part2, Hal/Gonk

(Clearing per higher-level issues above.)

Comment 37

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I haven't followed the entire C++/JS discussion above, but hal can't handle the multi-processing of this API.  It definitely makes sense to write the DB component in JS.  The last part of the system that talks to hal:: in the master process has to be C++, obviously.

The code that glues the DOM interface, multi-process layer, DB interaction, and hal calls together can be JS or C++.  What Gene has implemented here (mostly JS) looks OK to me.

Comment 38

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

Comment on attachment 627127 [details] [diff] [review]
Part1, IDL and dummy DOM

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

The AlarmManager idl file looks fine. I don't know the multi processing stuff well enough to have an opinion on if JS vs. C++ is the best way to do it, so I'm happy to leave that to others.

Comment 39

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

Comment on attachment 627135 [details] [diff] [review]
Part4, AlarmDB

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

Clearing these until there's an r+

Comment 40

[Gene Lian (I already quit Mozilla)]

Attachment: Part1, IDL and dummy DOM

The AlarmsManager IDL exposed to Web App is still the same. Just changed the AlarmHalService IDL a little bit for internal uses.

Gene

Comment 41

[Gene Lian (I already quit Mozilla)]

Attachment: Part2, Hal/Gonk

Cjones and Mounir,

I've had the following changes for the Hal/Gonk part:

1. Don't expose the WaitForAlarm() to clients, which doesn't make sense to design an hal:: function that will block the main thread.

2. Hide the thread of waiting on alarm fired in the Hal/Gonk, which will automatically be created when calling InitAlarm().

3. Since we only support RTC wakeup alarm type so far, don't expose the parameter for setting various alarm types to clients unless needed in the future.

4. The CloseAlarm() is able to terminate the alarm blocking thread by setting a dummy alarm, which can return the blocking IO immediately.

5. Regarding the "global alarm" issue pointed out by Cjones, I've already designed an alarm queue in the central AlarmService, which can collect the alarms requested by different content processes and uniformly reset the alarm by popping one by one from the queue.

Thanks,
Gene

Comment 42

[Gene Lian (I already quit Mozilla)]

Attachment: Part3, AlarmHalService

Mounir and Cjones,

AlarmHalService now becomes a very tiny interface which is simply in charge of passing alarm setting time from AlarmService to Hal/Gonk.

Thanks,
Gene

Comment 43

[Gene Lian (I already quit Mozilla)]

Attachment: Part4, AlarmDB

Vivien and Jonas,

I've had all the changes based on the following rules:

1. Give proper names for all the anonymous functions.

2. Disable the debug functions.

3. Use uniform parameter naming convention (i.e. aXxxx). 

4. Use uniform if/else block alignment.

Thanks,
Gene

Comment 44

[Gene Lian (I already quit Mozilla)]

Attachment: Part5, Alarm DOM

Vivien and Jonas,

In addition to the changes mentioned in the AlarmDB.jsm, I've also had the following changes in this patch:

1. If we use .bind(this), don't use self.xxx.

2. Try NOT to use .bind(this) unless necessary.

3. Change some naming things like: _curSetAlarm -> _currentAlarm, curSysTime -> nowTime, (new Date()).getTime() -> Date.now(), msg -> json and NO -> KO.

4. Access this._alarmQueue by alarmQueue for simpleness.

5. Use early return to avoid nested if/else blocks that could be too hard to read.

6. I've already checked the case of manually changing the system time after the current setting alarm time. It works well as expected (immediately fired)!

7. Regarding the issues you pointed out in the previous patch for the sendSyncMessage() return and the component reference, they should be good to go (please see my clarification in the previous comment).

Thanks,
Gene

Comment 45

[Gene Lian (I already quit Mozilla)]

Attachment: Alarm API Architecture

Uploaded the new architecture diagram.

Gene

Comment 46

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Suggested changes for Part 2

Hi Gene,

Your Part2 patch here is mostly ok, but there were a few things that needed to be fixed
 - we shouldn't expose the hal alarm API to content processes
 - prefer using pure-C++ notification (ObserverList<T>) to the XPCOM observer service.  hal is "lower level" than XPCOM so XPCOM shouldn't be used there unless necessary.
 - there were some fairly subtle threading bugs in the GonkHal implementation

There were a few stylistic changes here too, but not so important.

In my experience, describing threading bugs takes a long time, so I thought it would be easier to write suggested fixes instead :).

Note, I haven't compiled or tested this suggested patch.

Please feel free to ping me if you have more questions.

Comment 47

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 629128 [details] [diff] [review]
Part2, Hal/Gonk

This patch is better, but there are some issues I'd like to see fixed.  Please see the suggested patch.

Thanks!

Comment 48

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 629129 [details] [diff] [review]
Part3, AlarmHalService

This implementation will need to change based on the changes I suggested to the hal:: API, but those are trivial.  This is an OK way to talk to hal:: from JS.

Comment 49

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 629453 [details] [diff] [review]
Suggested changes for Part 2

>diff --git a/hal/gonk/GonkHal.cpp b/hal/gonk/GonkHal.cpp

>+void
>+DisableAlarm()
>+{
>+  MOZ_ASSERT(sAlarmData);

>+bool
>+SetAlarm(PRTime long aSeconds, long aNanoseconds)
>+{
>+  MOZ_ASSERT(sAlarmData);

Sorry, I realized while reviewing Part3 that both of these should be,

  if (!sAlarmData) {
    return [false];
  }

instead of assertions.

Comment 50

[Gene Lian (I already quit Mozilla)]

Attachment: Part1, IDL and dummy DOM

1. Rename the get() to be getAll() since we're getting all the alarms instead of a single one.

2. Removed the AlarmHalService IDL part to another patch, since it's not really related to the DOM interfaces.

Gene

Comment 51

[Gene Lian (I already quit Mozilla)]

Attachment: Part2, Hal/Gonk

Cjones.

I've already had all the changes based on your suggested patch. Note that some additional changes were added to make it compilable. Also, I replaced the assertion things with true/false returns (please let me know if you don't like these changes).

One big issue is it seems the Android bionic doesn't support pthread_cancel(). Do you have any suggested alternatives? I'll also try to work out a possible solution for this tonight. Please take a look on the other parts and hope they're all following your ideas.

Thanks,
Gene

Comment 52

[Gene Lian (I already quit Mozilla)]

Attachment: Part3, AlarmHalService

Cjones,

I've modified the AlarmHalService by using hal::RegisterTheOneAlarmObserver() and hal::UnregisterTheOneAlarmObserver(). Also, to make sure the alarm watching thread can be terminated successfully, I utilized the ClearOnShutdown() to make sure the AlarmHalService will correctly call its destructor and thus unregistering the alarm observer when encountering a shut-down event.

Thanks,
Gene

Comment 53

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Gene Lian [:gene] from comment #51)
> Created attachment 629745 [details] [diff] [review]
> Part2, Hal/Gonk
> 
> One big issue is it seems the Android bionic doesn't support
> pthread_cancel(). Do you have any suggested alternatives? I'll also try to
> work out a possible solution for this tonight. Please take a look on the
> other parts and hope they're all following your ideas.
> 

Arrrggghh!!  bionic'd again :(.

I think we'll need to emulate it by using pthread_sigmask() to process maybe SIGUSR2 on the watcher thread, and then pthread_kill() the watcher thread and set a "shutdown" bit in the AlarmData, then check that bit in the outer while() loop and in the inner do {} while() loop in the thread.

Comment 54

[Mounir Lamouri (:mounir)]

Comment on attachment 629745 [details] [diff] [review]
Part2, Hal/Gonk

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

No need for r+sr here. I will let Chris review this. He seems more appropriate than me.

Comment 55

[Mounir Lamouri (:mounir)]

Comment on attachment 629746 [details] [diff] [review]
Part3, AlarmHalService

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

IMO, you should have the DOM implementation in C++ so that class would be useless. I would feel bad r+'ing a patch with a architecture/design I consider to be over-complex. Given that r+sr isn't needed here, I'm going to defer the review to cjones who seems to consider the architecture sane enough.

Comment 56

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

Comment on attachment 629743 [details] [diff] [review]
Part1, IDL and dummy DOM

I think we'll only want to expose this API to installed apps, right? Since for non-installed web pages we can't open the page if it isn't open at the time when the alarm fires. So essentially the API wouldn't provide any functionality above setTimeout.


So please add some security check for now which makes .mozAlarms return null except for installed apps.

r=me with that fixed.

Comment 57

[Gene Lian (I already quit Mozilla)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #55)
> Comment on attachment 629746 [details] [diff] [review]
> Part3, AlarmHalService
> 
> Review of attachment 629746 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> IMO, you should have the DOM implementation in C++ so that class would be
> useless. I would feel bad r+'ing a patch with a architecture/design I
> consider to be over-complex. Given that r+sr isn't needed here, I'm going to
> defer the review to cjones who seems to consider the architecture sane
> enough.

Mounir,

I think I can totally understand your concern. It's definitely my fault that I should have more abundant discussions with you before starting the implementation. :)

Honestly, I used to try to change everything to C++ when you suggested, however, it's kind of painful for me to do so when we've already had quite a few logic in the core JS codes for multi-processes, indexedDB interactions and Date object calculations ^^a (I know it's not really a good excuse from an architecture design view...).

Really appreciate your suggestions. They are definitely valuable! :) I really learned a lot from this lesson and will try to be very careful next time when we encounter any similar cases again.

Gene

Comment 58

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 629131 [details] [diff] [review]
Part4, AlarmDB

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

::: dom/alarm/AlarmDB.jsm
@@ +32,5 @@
> +AlarmDB.prototype = {
> +  __proto__: IndexedDBHelper.prototype,
> +
> +  upgradeSchema: function upgradeSchema(aTransaction, aDb, aOldVersion, aNewVersion) {
> +  

Remove this line break.

Comment 59

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 629135 [details] [diff] [review]
Part5, Alarm DOM

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

Very close but I would like to see a new patch.

::: dom/alarm/AlarmService.jsm
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict"

nit: "use strict"; - it miss a ';'

@@ +12,5 @@
> +    dump("AlarmService: " + aStr + "\n");
> +  }
> +}
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;

nit: There is some places where you add a space after and before brackets and some where you don't. Let's add it always.

@@ +25,5 @@
> +
> +let AlarmService = {
> +
> +  init: function init() {
> +

nit: remove the line break

@@ +31,5 @@
> +
> +    // set the alarmHalService which communicates with hal/gonk
> +    this._alarmHalService = Cc["@mozilla.org/alarmHalService;1"].getService(Ci.nsIAlarmHalService);
> +
> +    // set the parent process message manager

This comment and the one before does not seems useful to me - basically what you describe is said in the name of the component.

@@ +37,5 @@
> +
> +    // add the messages to be listened
> +    const messages = ["AlarmsManager:Get", "AlarmsManager:Add", "AlarmsManager:Remove"];
> +    messages.forEach(
> +      function addMessage(msgName) {

messages.forEach(function addMessage(msgName) {
  ...
}.bind(this));

keep the callback on the same line.

@@ +69,5 @@
> +    this._db.get(
> +      function getSuccessCb(aAlarms) {
> +        debug("Callback after getting alarms from database: " + JSON.stringify(aAlarms));
> +        
> +        // access this._alarmQueue by alarmQueue for simpleness

nit: remove the comment

@@ +75,5 @@
> +
> +        // only add the alarm that is valid
> +        let nowTime = Date.now();
> +        aAlarms.forEach(
> +          function addAlarm(aAlarm) {

nit: aAlarms.forEach(function addAlarm(aAlarm) {
  ...
});

@@ +83,5 @@
> +          }
> +        );
> +       
> +        // sort the alarms by time stamps
> +        alarmQueue.sort(this._sortAlarmQueueFunc);

What about renaming this._sortAlarmQueueFunc -> this._sortAlarmByTimeStamps and remove the comment?

@@ +85,5 @@
> +       
> +        // sort the alarms by time stamps
> +        alarmQueue.sort(this._sortAlarmQueueFunc);
> +
> +        if (alarmQueue.length) {

You can include the sorting in this if condition too.

@@ +88,5 @@
> +
> +        if (alarmQueue.length) {
> +          // set the next alarm from queue
> +          this._currentAlarm = alarmQueue.shift();
> +          let nextAlarmTime = (new Date(this._currentAlarm.date)).getTime();

The more I read this kind of conversion, the more I wonder why alarm.date is not a timestamp itself?

@@ +108,5 @@
> +    Services.obs.addObserver(this, "alarm-fired", false);
> +  },
> +
> +  receiveMessage: function receiveMessage(aMessage) {
> +      

nit: no need for this line break

@@ +146,5 @@
> +            debug("Callback after adding alarm in database.");
> +            let nowTime = Date.now();
> +            let newAlarmTime = (new Date(newAlarm.date)).getTime();
> +            
> +            // access this._alarmQueue by alarmQueue for simpleness

nit: remove this comment

@@ +207,5 @@
> +          json.id,
> +          function removeSuccessCb() {
> +            debug("Callback after removing alarm from database.");
> +            
> +            // access this._alarmQueue by alarmQueue for simpleness

nit: remove this comment

@@ +267,5 @@
> +        break;
> +    }
> +  },
> +  
> +  observe: function observe(subject, topic, data) {

nit: aSubject, aTopic, aData

@@ +282,5 @@
> +
> +          this._currentAlarm = null;
> +        }
> +
> +        // access this._alarmQueue by alarmQueue for simpleness

nit: remove this comment

@@ +301,5 @@
> +          } else {
> +            this._currentAlarm = nextAlarm;
> +            if (!this._alarmHalService.setAlarm(nextAlarmTime / 1000, 0)) {
> +              throw Components.results.NS_ERROR_FAILURE;
> +            }

Everytime this._currentAlarm is set, it is followed by code that set the alarm on hal and that checks if that works before throwing. Make this._currentAlarm a getter/setter and move this code there to not have to repeat it everywhere.

::: dom/alarm/AlarmsManager.js
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict"

nit: "use strict"; - it miss a ';'

@@ +53,5 @@
> +        isIgnoreTimezone = true;
> +        break;
> +        
> +      case "honorTimezone":
> +      case null:

case null?

@@ +134,2 @@
>      // add the valid messages to be listened
> +    this.initHelper(aWindow, ["AlarmsManager:Get:Return:OK", "AlarmsManager:Get:Return:NO"]);

NO -> KO

Comment 60

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 629745 [details] [diff] [review]
Part2, Hal/Gonk

I would prefer to review this after we find a workaround for pthread_cancel, since that's critical to the correctness of this patch.

Comment 61

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 629746 [details] [diff] [review]
Part3, AlarmHalService

>diff --git a/dom/alarm/AlarmHalService.cpp b/dom/alarm/AlarmHalService.cpp

>+void
>+AlarmHalService::Notify(const mozilla::void_t& aVoid)
>+{
>+  nsCOMPtr<nsIObserverService> observerService = mozilla::services::GetObserverService();
>+  if (observerService) {
>+    observerService->NotifyObservers(nsnull, "alarm-fired", nsnull);
>+  }

As we discussed on IRC, is there an (easy) way to implement this functionality using a callback function instead of a global observer-service broadcast?  If there's not an easy way, then this implementation looks OK, but please let me know.

It's a little sad that it requires this much boilerplate to call a few C++ functions from JS, but that's OK.  It makes sense to have the rest of your implementation written in JS.

(Clearing review request pending answer to question above.)

Comment 62

[Gene Lian (I already quit Mozilla)]

Attachment: Part1, IDL and dummy DOM, V2

Notes:

1. Change the repectTimezone to be a required argument.

2. Redesign the add() as a DOMRequest return.

Gene

Comment 63

[Gene Lian (I already quit Mozilla)]

Attachment: Part2, Hal/Gonk, V2

Cjones,

I've already added pthread_kill() part, which is well-tested and everything works well (the alarm watcher can be successfully killed and the resources can be correctly released).

Thanks,
Gene

Comment 64

[Gene Lian (I already quit Mozilla)]

Attachment: Part3, AlarmHalService, V2

Cjones,

I added the following 2 callback functions which can be registered to send the alarm-fired event and the timezone-changed event as follows:

AlarmHalService::SetAlarmFiredCb(nsIAlarmEventCb* aAlarmFiredCb);
AlarmHalService::SetTimezoneChangedCb(nsIAlarmEventCb* aTimeZoneChangedCb);

Notice the SetTimezoneChangedCb() still needs the support of bug 714358 which is under construction and seems to be available very soon :) I just reserved some dummy functions here. We need this function to dynamically adjust the alarm time respect to the correct timezone where user is located. For example, supposing an alarm is set at 7:00pm at Tokyo, it must be gone off at 7:00pm respect to Paris' local time when the user is located at Paris. Therefore, knowing the user's timezone is needed.

Thanks,
Gene

Comment 65

[Gene Lian (I already quit Mozilla)]

Attachment: Part4, AlarmDB, V2

I just did some renaming things in the new patch like get() -> getAll() and some minor code clean-up. So I directly specify this patch with + since Vivien has already reviewed through these codes.

Carrying forward r=vivien.

Gene

Comment 66

[Gene Lian (I already quit Mozilla)]

Attachment: Part5, Alarm DOM, V2

Vivien,

Thanks for your detailed reviews, especially the suggestion of getter/setter which can magically simplify the codes :) In the new patch, in addition to your review comments, I've also had the following big changes:

1. Based on Cjones' suggestions, instead of using the "alarm-fired" observer, I use this._alarmHalService.setAlarmFiredCb() to register a callback which will be directly called whenever an alarm-fired event is detected, which seems more straightforward.

2. Another similar this._alarmHalService.setTimezoneChangedCb() is designed to detect the timezone-changed event. For an alarm specified with "ignoreTimezone", it must be gone off respect to the user's timezone. Supposing an alarm is set at 7:00pm at Tokyo, it must be gone off at 7:00pm respect to Paris' local time when the user is located at Paris. Therefore, we need to dynamically adjust the alarm UTC time whenever the user's current timezone is changed.

3. Whenever the timezone is changed, all the alarms being set or in the queue will be totally cleared. They will be restored and programmed again from the database based on the current timezone. Please see AlarmService._restoreAlarmsFromDb().

4. All the alarm setting time will now be adjusted by calculating the difference of the original timezone and the current timezone, when it is specified with a "ignoreTimezone" type. Please see AlarmService._getAlarmTime().

5. I change the AlarmsManager.add() to be a DOMRequest-return one and rename the AlarmsManager.get() to be AlarmsManager.getAll() based on Jonas' suggestions.

6. Regarding your question about (new Date(alarm.date)).getTime(), because the alarm.date is saved as a general JSON object, we need a way to cast it to be a Date object. Or do we have any other better ways?

Thanks,
Gene

Comment 67

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 630512 [details] [diff] [review]
Part2, Hal/Gonk, V2

Hi Gene,

In general, I think these changes introduce more returned error codes
than clients of the APIs really need to care about.  Error codes
should be used as little as possible, only when you really concretely
expect the client do make use of the returned code.  But this is an
API with a very specific purpose and only one client currently, so
things are OK the way they are now :).

(POSIX APIs are a *very* bad example because UNIX signals force pretty
much every function to return an error code, and the client almost
always needs to care about the codes.)

>diff --git a/hal/Hal.cpp b/hal/Hal.cpp
 
>+static AlarmObserver* sAlarmObserver = NULL;

| = NULL| is unnecessary for static data, please remove.

>+
>+bool
>+RegisterTheOneAlarmObserver(AlarmObserver* aObserver)
>+{
>+  if (InSandbox()) {
>+    HAL_LOG(("Alarms can't be programmed from sandboxed contexts.  Yet."));
>+    return false;
>+  }

This needs to be a strong assertion.  Please restore that.

>+
>+  if (sAlarmObserver) {
>+    HAL_LOG(("Currently, there can only be 0 or 1 alarm observers."));
>+    return false;
>+  }

This should also be a strong assertion.  Please restore that.

>+  return hal_impl::EnableAlarm();

Please leave this as RETURN_PROXY_IF_SANDBOXED(EnableAlarm()).  We may
allow content processes to program alarms in the future, but we don't
now.  The assertions above already check the current invariant, that
it's not allowed.

>+bool
>+UnregisterTheOneAlarmObserver()
>+{

>+  if (InSandbox()) {
>+    HAL_LOG(("Alarms can't be programmed from sandboxed contexts.  Yet."));
>+    return false;
>+  }

It doesn't matter if content processes call this function, because we
don't allow them to register observers above.  So no need to check
here.

>+  if (sAlarmObserver) {
>+    sAlarmObserver = NULL;
>+    return hal_impl::DisableAlarm();

Per reasoning above, please restore RETURN_PROXY_IF_SANDBOXED.

>+  return true;

This function can't fail so there's no need to return true/false.

>+bool
>+SetAlarm(PRTime long aSeconds, long aNanoseconds)

Oops, |long aSeconds|.

>+{
>+  if (InSandbox()) {
>+    HAL_LOG(("Alarms can't be programmed from sandboxed contexts.  Yet."));
>+    return false;
>+  }
>+
>+  if (!sAlarmObserver) {
>+    HAL_LOG(("It's pointless to program an alarm nothing is going to observe."));
>+    return false;
>+  }

Please restore the previous assertions here too.

>+  return hal_impl::SetAlarm(aSeconds, aNanoseconds);

Please restore RETURN_PROXY_IF_SANDBOXED.

>diff --git a/hal/gonk/GonkHal.cpp b/hal/gonk/GonkHal.cpp

>+static void* 
>+WaitForAlarm(void* aData)
>+{

>+  pthread_cleanup_pop(1);
>+  return NULL;

Oops, my fault ... this |return NULL;| should have been
|pthread_exit(NULL)|, which automatically pops the cleanup stack.

>+bool
>+EnableAlarm()
>+{
>+  if (sAlarmData) {
>+    HAL_LOG(("We should enable the alarm only one time."));
>+    return false;
>+  }

The Hal.cpp code protects this invariant.  No need to check here.
Please restore the previous assertion.

>+bool
>+DisableAlarm()
>+{
>+  if (!sAlarmData) {
>+    HAL_LOG(("We should have enabled the alarm."));
>+    return false;

If we return false here, what do you expect the client to do with that
return value?

>+  if (pthread_kill(sAlarmFireWatcherThread, SIGUSR2)) {

There's no way this pthread_kill() can fail in this case, so this
error check should probably be an assertion.  Try

  DebugOnly<int> err = pthread_kill(...);
  MOZ_ASSERT(!err);

>+  return true;

I'm not sure this return value is useful but don't feel too strongly
about it.

>+bool
>+SetAlarm(PRTime long aSeconds, long aNanoseconds)
>+{

Oops, my fault again --- should be |long aSeconds|.

OK!  r=me with those changes.

Comment 68

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 630514 [details] [diff] [review]
Part3, AlarmHalService, V2

>diff --git a/dom/alarm/AlarmHalService.cpp b/dom/alarm/AlarmHalService.cpp

>+void
>+AlarmHalService::Init()
>+{
>+  mAlarmEnabled = hal::RegisterTheOneAlarmObserver(this);
>+  

|using namespace hal;| please.

>+  if (!mAlarmEnabled) {

Having to track this state here doesn't seem very useful to me.  This
is a problem with returning error codes, it can "trickle out" to a lot
of places where it's not necessarily useful.

>+/* virtual */ AlarmHalService::~AlarmHalService() 
>+{

>+    mAlarmEnabled = false;

There's no need to set this in the destructor.

>+NS_IMETHODIMP
>+AlarmHalService::SetAlarm(PRInt32 aSeconds, PRInt32 aNanoseconds, bool *aStatus)
>+{

>+  *aStatus = hal::SetAlarm(aSeconds, aNanoseconds);
>+

The XPCOM convention is to not set out-parameters unless the return
code is NS_OK.  It's not something I like, but we need to follow that.

>diff --git a/dom/alarm/nsIAlarmHalService.idl b/dom/alarm/nsIAlarmHalService.idl

>+  void setTimezoneChangedCb(in nsIAlarmEventCb aTimezoneChangedCb);

At the moment, I don't see any reason for this to be part of the alarm
API.  The use cases are quite different.  Please remove for now.

r=me with those changes.

Comment 69

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #68)
> Comment on attachment 630514 [details] [diff] [review]
> Part3, AlarmHalService, V2
> 
> >+  void setTimezoneChangedCb(in nsIAlarmEventCb aTimezoneChangedCb);
> 
> At the moment, I don't see any reason for this to be part of the alarm
> API.  The use cases are quite different.  Please remove for now.

Gene explained this to me.  Carry on!

Comment 70

[Gene Lian (I already quit Mozilla)]

Attachment: Part2, Hal/Gonk, V2.1

Cjones,

Really appreciate your review again. I really learned a lot form the suggestions :) Please take a look on the new patch when you're available. Please feel free to let me know if there is any part I can do it better.

Gene

Comment 71

[Gene Lian (I already quit Mozilla)]

Attachment: Part3, AlarmHalService, V2.1

Cjones,

As discussed in the IRC, I keep the SetTimezoneChangedCb() since the upper layer (AlarmService) has lots of logic done to deal with the timezone change (by some hard-code test). Once the bug 714358 is done, I believe this function can be enabled soon ;)

Thanks,
Gene

Comment 72

[Gene Lian (I already quit Mozilla)]

Attachment: Part3, AlarmHalService, V2.1

Comment 73

[Gene Lian (I already quit Mozilla)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #67)
> Comment on attachment 630512 [details] [diff] [review]
> Part2, Hal/Gonk, V2
> >diff --git a/hal/gonk/GonkHal.cpp b/hal/gonk/GonkHal.cpp
> 
> >+static void* 
> >+WaitForAlarm(void* aData)
> >+{
> 
> >+  pthread_cleanup_pop(1);
> >+  return NULL;
> 
> Oops, my fault ... this |return NULL;| should have been
> |pthread_exit(NULL)|, which automatically pops the cleanup stack.

Btw, one thing I don't quite understand here. Do you mean I need to replace the return |return NULL;| with |pthread_exit(NULL)|? But the compiler doesn't allow me to do this because we need to return something for this non-void function. In addition, isn't pthread_cleanup_pop(1) already in charge of popping the cleanup stack?

Also, I had difficulty finding the null issue (sAlarmState) you mentioned in the IRC. Everything looks good to me.

Oops.. You've already had review+ when I post this ;)

Thanks,
Gene

Comment 74

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Yes, smarter compilers don't make you do this, but

 pthread_exit(NULL);
 return NULL; // not reached

is the right way to write that.  Please make that change :).

(You're correct that pthread_cleanup_pop() is functionally equivalent in this case, but
 - it's a distributed invariant (that there's only 1 handler to pop.  Distributed invariants are hard.
 - it implies that pthread_cleanup_pop() is the only way the cleanup handler is called, which isn't true; it's always called when the thread exits.
)

Comment 75

[Gene Lian (I already quit Mozilla)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #74)
> Yes, smarter compilers don't make you do this, but
> 
>  pthread_exit(NULL);
>  return NULL; // not reached
> 
> is the right way to write that.  Please make that change :).
> 
> (You're correct that pthread_cleanup_pop() is functionally equivalent in
> this case, but
>  - it's a distributed invariant (that there's only 1 handler to pop. 
> Distributed invariants are hard.
>  - it implies that pthread_cleanup_pop() is the only way the cleanup handler
> is called, which isn't true; it's always called when the thread exits.
> )

After some discussions with Cjones on the IRC, we decided to keep using the pair of pthread_cleanup_push() and pthread_cleanup_pop(), instead of pthread_exit()..

Gene

Comment 76

[Gene Lian (I already quit Mozilla)]

Attachment: Part1, IDL and dummy DOM, V2.1

Jonas,

I've already had the following changes based on our previous discussions. Please have a review when you're available :)

1. Change the repectTimezone to be a required argument.
2. Redesign the add() as a DOMRequest return.
3. Add security/permission checks for the APIs.
4. navigator.mozAlarms = null when uninstalled app is using it.

Thanks,
Gene

Comment 77

[Gene Lian (I already quit Mozilla)]

Attachment: Part4, AlarmDB, V2.1

Just did some minor code cleanup and renaming things in the new patch, so I directly specify this patch with + since Vivien has already reviewed through these codes.

Gene

Comment 78

[Gene Lian (I already quit Mozilla)]

Attachment: Part5, Alarm DOM, V2.1

Vivien,

Just rebase the patch again because I added some security/permission checks in the Part1, IDL and dummy DOM, V2.1. Please refer to comment #66 for what I've done for this new patch.

Thanks,
Gene

Comment 79

[Mounir Lamouri (:mounir)]

Gene, all Makefiles you are adding should be added to $MAKEFILES_dom in toolkit/toolkit-makefiles.sh.

Comment 80

[Mounir Lamouri (:mounir)]

Comment on attachment 631301 [details] [diff] [review]
Part1, IDL and dummy DOM, V2.1

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

::: dom/alarm/test/test_alarm_basics.html
@@ +13,5 @@
> +<script type="application/javascript">
> +
> +/** Test for Alarm API **/
> +
> +ok('mozAlarms' in navigator, "navigator.mozAlarms should exist");

Do we want non-installed applications to access the alarm object?!

Comment 81

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

Comment on attachment 631301 [details] [diff] [review]
Part1, IDL and dummy DOM, V2.1

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

::: dom/alarm/AlarmsManager.js
@@ +38,5 @@
> +      throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
> +    }
> +
> +    let request = this.createRequest();
> +    return request;

You can just do |return this.createRequest();|. Though I suspect this code will change in later patches, so fine to keep as-is if so.

Same in getAll

::: dom/alarm/test/test_alarm_basics.html
@@ +13,5 @@
> +<script type="application/javascript">
> +
> +/** Test for Alarm API **/
> +
> +ok('mozAlarms' in navigator, "navigator.mozAlarms should exist");

Please test that the property exists but is null. We don't want to expose the object to pages that don't have access to it.

Comment 82

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 631305 [details] [diff] [review]
Part5, Alarm DOM, V2.1

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

That's close! I would like to understand the 'autoincrement' vs this._lastID story.

::: dom/alarm/AlarmService.jsm
@@ +30,5 @@
> +    this._currentTimezoneOffset = (new Date()).getTimezoneOffset();
> +
> +    this._alarmHalService = Cc["@mozilla.org/alarmHalService;1"].getService(Ci.nsIAlarmHalService);
> +    this._setAlarmFiredCb();
> +    this._setTimezoneChangedCb();

Rewrite a little bit your code ans save some levels of indirections.

let alarmHalService = this._alarmHalService = Cc["@mozilla.org/alarmHalService;1"].getService(Ci.nsIAlarmHalService);
alarmHalService.setAlarmFiredCb(this._onAlarmFired.bind(this));
alarmHalService.setTimezoneChangedCb(this._onTimezoneChanged.bind(this));

where onAlarmFired is the content of alarmFiredCb and onTimezoneChanged is the content of timezoneChangedCb

@@ +32,5 @@
> +    this._alarmHalService = Cc["@mozilla.org/alarmHalService;1"].getService(Ci.nsIAlarmHalService);
> +    this._setAlarmFiredCb();
> +    this._setTimezoneChangedCb();
> +
> +    this._ppmm = Cc["@mozilla.org/parentprocessmessagemanager;1"].getService(Ci.nsIFrameMessageManager);

Not that I'm a big fan of Global variables but in a few files ppmm is defined on |this| with a lazy getter. Let's follow this pattern and avoid to use bind where you don't need to anymore because of that.

References:
 - http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#22
 - http://mxr.mozilla.org/mozilla-central/source/dom/contacts/fallback/ContactService.jsm#23
 - http://mxr.mozilla.org/mozilla-central/source/dom/contacts/fallback/ContactService.jsm#23

@@ +57,5 @@
> +      }
> +      if (!this._alarmHalService.setAlarm(this._getAlarmTime(aAlarm) / 1000, 0)) {
> +        throw Components.results.NS_ERROR_FAILURE;
> +      }
> +    }.bind(this));

Simply do:

      _alarm: null,
      get _currentAlarm() {
        return this._alarm;
      },
      set _currentAlarm(aAlarm) {
        this._alarm = aAlarm;
        if (!aAlarm)
          return;
        
        if (!this._alarmHalService.setAlarm(this._getAlarmTime(aAlarm) / 1000, 0))
          throw Components.results.NS_ERROR_FAILURE;
      }

as if you add methods to the AlarmService singleton.

@@ +72,5 @@
> +      }.bind(this),
> +      function getLastIdErrorCb(aErrorMsg) {
> +        throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
> +      }
> +    );

Sorry to not have asked that before but can you explain the rationale behind getLastID? Why can't you use the 'autoincrement' paramerer of a store? https://developer.mozilla.org/en/IndexedDB/IDBDatabase#createObjectStore%28%29

@@ +215,5 @@
> +  },
> +
> +  observe: function observe(aSubject, aTopic, aData) {
> +    debug("observe(): " + aTopic);
> +  },

This code does nothing.

@@ +303,5 @@
> +
> +  _getAlarmTime: function _getAlarmTime(aAlarm) {
> +    let alarmTime = (new Date(aAlarm.date)).getTime();
> +    
> +    // For an alarm is specified with "ignoreTimezone",

remove 'is'

@@ +310,5 @@
> +    // it must be gone off at 7:00pm respect to Paris' 
> +    // local time when the user is located at Paris.
> +    // We can adjust the alarm UTC time by calculating
> +    // the difference of the orginal timezone and the 
> +    // curren timezone.

curren -> current

Comment 83

[Gene Lian (I already quit Mozilla)]

(In reply to Vivien Nicolas (:vingtetun) from comment #82)
> Comment on attachment 631305 [details] [diff] [review]
> Part5, Alarm DOM, V2.1
> 
> Review of attachment 631305 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That's close! I would like to understand the 'autoincrement' vs this._lastID
> story.
> @@ +72,5 @@
> > +      }.bind(this),
> > +      function getLastIdErrorCb(aErrorMsg) {
> > +        throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
> > +      }
> > +    );
> 
> Sorry to not have asked that before but can you explain the rationale behind
> getLastID? Why can't you use the 'autoincrement' parameter of a store?
> https://developer.mozilla.org/en/IndexedDB/
> IDBDatabase#createObjectStore%28%29

This is a very good point! :) The main reason why I used .getLastId() was because the .add() wasn't a DOMRequest function before. Therefore, the AlarmService needs to internally pre-determine the next ID so that it can be returned earlier without waiting the record-adding process is finished in the indexedDB. Now that the .add() becomes a DOMRequest one, I believe it's more reasonable to use the autoincrement now.

The only benefit of using the self-defined .getLastId() is: whenever the alarm records were totally clear, after a reboot it can return an ID restarting from 0. However, the autoincrement always returns a bigger ID even if the database used to be empty. That could be a problem if the ID number is exploding although it's almost impossible.

All in all, it makes sense to use the autoincrement now. I'll have this change in the next patch.

Thanks,
Gene

Comment 84

[Gene Lian (I already quit Mozilla)]

Attachment: Part1, IDL and dummy DOM, V3

Jonas and Mounir,

Thanks for your reviews and comments. I've done the following changes in this patch:

1. Add 2 testing pages: test_alarm_permitted_app.html and test_alarm_non_permitted_app.html to test the installed and non-installed applications respectively, where the latter will get a null when accessing navigator.mozAlarms.

2. Use return this.createRequest();

3. Other minor code clean-up.

Please have a review when you're available and feel free to let me know if I can do anything better :)

Gene

Comment 85

[Gene Lian (I already quit Mozilla)]

Attachment: Part4, AlarmDB, V3

Vivien,

To take use of the autoIncrement of indexedDB, we need to have some changes for the AlarmDB database schema. Also, the AlarmDB.add() needs to return the ID automatically generated by the indexedDB.

Please also see my response for why using .getLastId() before at comment #83.

Thanks,
Gene

Comment 86

[Gene Lian (I already quit Mozilla)]

Attachment: Part5, Alarm DOM, V3

Vivien,

Thanks for your review again. :) I've already had the changes based on your review comments. This biggest one is I removed the usage of .getLastId() and use the native autoIncrement of indexedDB (please see why at comment #83). The others are some minor code clean-up.

Please feel free to let me know if I can do anything better. I'm glad to do any changes even if they're minor ones. :)

Thanks,
Gene

Comment 87

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 633481 [details] [diff] [review]
Part5, Alarm DOM, V3

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

Thanks for all the hard work!

Comment 88

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 633480 [details] [diff] [review]
Part4, AlarmDB, V3

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

::: dom/alarm/AlarmDB.jsm
@@ +119,5 @@
> +  init: function init(aGlobal) {
> +    debug("init()");
> +
> +    this.initDBHelper(ALARMDB_NAME, ALARMDB_VERSION, ALARMSTORE_NAME, aGlobal);
> +  }

Personally I would put the init on top of the prototype.

Comment 89

[Mounir Lamouri (:mounir)]

Comment on attachment 633478 [details] [diff] [review]
Part1, IDL and dummy DOM, V3

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

I feel like I'm missing something: where did you add the mozAlarms attribute to Navigator?

::: b2g/chrome/content/shell.js
@@ +54,5 @@
>    let permissions = [
>      'indexedDB', 'indexedDB-unlimited', 'webapps-manage', 'offline-app', 'pin-app',
>      'websettings-read', 'websettings-readwrite',
>      'content-camera', 'webcontacts-manage', 'wifi-manage', 'desktop-notification',
> +    'geolocation', 'device-storage', 'webalarms-manage'

Why not simply calling the permission "alarms"?

::: dom/alarm/test/test_alarm_non_permitted_app.html
@@ +18,5 @@
> +
> +var mozAlarms = navigator.mozAlarms;
> +
> +ok('mozAlarms' in navigator, "navigator.mozAlarms should exist");
> +is(mozAlarms, null, "navigator.mozAlarms should return null");

Can't you merge this test with the test_alarm_basics.html?

::: dom/alarm/test/test_alarm_permitted_app.html
@@ +20,5 @@
> +
> +ok('mozAlarms' in navigator, "navigator.mozAlarms should exist");
> +ok(mozAlarms, "navigator.mozAlarms should return an object");
> +ok(mozAlarms instanceof Components.interfaces.nsIDOMAlarmsManager, 
> +  "navigator.mozAlarms should be an nsIDOMAlarmsManager object");

ditto

Comment 90

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Mounir Lamouri (:mounir) from comment #89)
> Comment on attachment 633478 [details] [diff] [review]
> Part1, IDL and dummy DOM, V3
> 
> Review of attachment 633478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I feel like I'm missing something: where did you add the mozAlarms attribute
> to Navigator?
>

It is in Part 1, IDL and dummy DOM

diff --git a/dom/alarm/AlarmsManager.manifest b/dom/alarm/AlarmsManager.manifest
new file mode 100644
--- /dev/null
+++ b/dom/alarm/AlarmsManager.manifest
@@ -0,0 +1,3 @@
+component {fea1e884-9b05-11e1-9b64-87a7016c3860} AlarmsManager.js
+contract @mozilla.org/alarmsManager;1 {fea1e884-9b05-11e1-9b64-87a7016c3860}
+category JavaScript-navigator-property mozAlarms @mozilla.org/alarmsManager;1
 
> ::: b2g/chrome/content/shell.js
> @@ +54,5 @@
> >    let permissions = [
> >      'indexedDB', 'indexedDB-unlimited', 'webapps-manage', 'offline-app', 'pin-app',
> >      'websettings-read', 'websettings-readwrite',
> >      'content-camera', 'webcontacts-manage', 'wifi-manage', 'desktop-notification',
> > +    'geolocation', 'device-storage', 'webalarms-manage'
> 
> Why not simply calling the permission "alarms"?
> 
> ::: dom/alarm/test/test_alarm_non_permitted_app.html
> @@ +18,5 @@
> > +
> > +var mozAlarms = navigator.mozAlarms;
> > +
> > +ok('mozAlarms' in navigator, "navigator.mozAlarms should exist");
> > +is(mozAlarms, null, "navigator.mozAlarms should return null");
> 
> Can't you merge this test with the test_alarm_basics.html?
> 
> ::: dom/alarm/test/test_alarm_permitted_app.html
> @@ +20,5 @@
> > +
> > +ok('mozAlarms' in navigator, "navigator.mozAlarms should exist");
> > +ok(mozAlarms, "navigator.mozAlarms should return an object");
> > +ok(mozAlarms instanceof Components.interfaces.nsIDOMAlarmsManager, 
> > +  "navigator.mozAlarms should be an nsIDOMAlarmsManager object");
> 
> ditto

Comment 91

[Gene Lian (I already quit Mozilla)]

(In reply to Mounir Lamouri (:mounir) from comment #89)

Thanks Mounir for the review!

> Comment on attachment 633478 [details] [diff] [review]
> Part1, IDL and dummy DOM, V3
> 
> Review of attachment 633478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I feel like I'm missing something: where did you add the mozAlarms attribute
> to Navigator?

It's in AlarmsManager.manifest and activated by:

category JavaScript-navigator-property mozAlarms @mozilla.org/alarmsManager;1

(oops! Vivien has pointed it out when I responded this)

> 
> ::: b2g/chrome/content/shell.js
> @@ +54,5 @@
> >    let permissions = [
> >      'indexedDB', 'indexedDB-unlimited', 'webapps-manage', 'offline-app', 'pin-app',
> >      'websettings-read', 'websettings-readwrite',
> >      'content-camera', 'webcontacts-manage', 'wifi-manage', 'desktop-notification',
> > +    'geolocation', 'device-storage', 'webalarms-manage'
> 
> Why not simply calling the permission "alarms"?

Sounds good! I was wondering this one also. I'll have this change in the next patch.

> 
> ::: dom/alarm/test/test_alarm_non_permitted_app.html
> @@ +18,5 @@
> > +
> > +var mozAlarms = navigator.mozAlarms;
> > +
> > +ok('mozAlarms' in navigator, "navigator.mozAlarms should exist");
> > +is(mozAlarms, null, "navigator.mozAlarms should return null");
> 
> Can't you merge this test with the test_alarm_basics.html?

Actually, what I'm doing now for those non-permitted apps is to directly disable them in the AlarmsManager.init() (at line 72-73), which can only be executed one time when the navigator.mozAlarms is used in the first place. Under this circumstance, it's infeasible to test permission and non-permission within the single page since one page cannot do the initialization more than one time though. 

My personal thought is whether the app is permitted or not should be immediately decided when the app is launched (initialized). That's why I edited 2 different pages to test the permissions. Please let me know if it doesn't sound reasonable to you and then I'll try to modify the permission mechanism :)

Thanks,
Gene

Comment 92

[Gene Lian (I already quit Mozilla)]

Attachment: Part1, IDL and dummy DOM, V3.1

Please see the responses in comment #91.

Thanks,
Gene

Comment 93

[Gene Lian (I already quit Mozilla)]

Attachment: Part2, Hal/Gonk, V3

Cjones,

I didn't change anything from the previous patch (V2.1) except for adding a new and simple Hal/Gonk function as below:

int GetTimezoneOffset(bool aIgnoreDST);

This is needed because the Alarm API needs a way to know the current timezone offset. Actually, I originally used (new Date()).getTimezoneOffset() (JS codes) to do this. However, it cannot correctly return the updated timezone offset when the timezone is changed (bug 285615). Under this circumstances, we have to refresh the page or reboot the device to get the new timezone, which doesn't make sense at all to a B2G app.

In addition, I believe we must need a more native way to directly get the system timezone offset from Hal/Gonk, because some C++ codes in middleware will need this to do some timezone-related adjustments or calculations (at least for Alarm API). Nevertheless, the current SetTimezone() cannot support the getter and the property_set("persist.sys.timezone", ...); can only return a non-calculable timezone string like "Asis/Taipei" or "America/Chicago"... etc.

May I have your review on this? Really sorry for bothering you again. You can search "timezoneOffset" to quickly see all the changes for this.

Thanks,
Gene

Comment 94

[Gene Lian (I already quit Mozilla)]

Attachment: Part3, AlarmHalService, V3

Cjones,

Just having some simple changes corresponding to the future use of GetTimezoneOffset() in Hal/Gonk. After the bug 714358 is done, the expected codes to call the OnTimezoneChanged() callback will be like:

void
AlarmHalService::Notify(...)
{
  if (mTimezoneChangedCb) {
    mTimezoneChangedCb->OnTimezoneChanged(GetTimezoneOffset(false));
  }
}

Please feel free to let me know if anything seems uncomfortable to you.

Thanks,
Gene

Comment 95

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Since gecko totally controls the system clock for b2g/gonk, we should just fix bug 285615 instead of working around the bug.  Have you investigated what that would take?

Comment 96

[Gene Lian (I already quit Mozilla)]

Attachment: Part2, Hal/Gonk, V3

Don't need to expose a new hal:: function for getting timezone offset, since the Gecko is able to directly use the native libc. Recover the patch to the previous one, which has been reviewed through by Cjones, so just setting review+.

Gene

Comment 97

[Gene Lian (I already quit Mozilla)]

Attachment: Part3, AlarmHalService, V3

Cjones,

Since Gecko should be able to use the native cross-platform C library, I designed the following function in this layer:

int AlarmHalService::GetTimezoneOffset(bool aIgnoreDST);

which can be used to calculate and return the current timezone offset. Please also find comment #94 to see how we'll call it whenever the timezone change is notified.

Sorry for letting you misunderstand this issue. The JS Date object bug is totally another separate issue.

Thanks,
Gene

Comment 98

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] from comment #97)
> Created attachment 635178 [details] [diff] [review]
> Part3, AlarmHalService, V3
> 
> Cjones,
> 
> Since Gecko should be able to use the native cross-platform C library, I
> designed the following function in this layer:
> 
> int AlarmHalService::GetTimezoneOffset(bool aIgnoreDST);

Oops, discover a potential bug when crossing dates. I'll fix it tomorrow and try to use NSPR as we can. Please stay tuned.

Thanks,
Gene

Comment 99

[Gene Lian (I already quit Mozilla)]

Attachment: Part3, AlarmHalService, V3

Cjones,

Eventually, I used a set of existing NSPR functions (prtime.h) to get the system timezone offset. Please find the following function:

PRInt32 AlarmHalService::GetTimezoneOffset(bool aIgnoreDST);

Please also find comment #94 to see how we'll call it whenever the timezone change is notified.

Sorry for letting you misunderstand this issue. The JS Date object bug is totally another separate issue in JS engine.

Thanks,
Gene

Comment 100

[Mounir Lamouri (:mounir)]

Comment on attachment 634835 [details] [diff] [review]
Part1, IDL and dummy DOM, V3.1

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

I'm not really convinced that it is a good idea to never allow alarms for the app if the first request failed. It's no big deal for the moment because it seems that we are going to have an implicit authorization model for alarms.

BTW, what about not showing 'mozAlarms' on non-B2G targets? Jonas, what do you think of that?

::: dom/alarm/AlarmsManager.js
@@ +34,5 @@
> +                                      flags: nsIClassInfo.DOM_OBJECT }),
> +
> +  add: function add(aDate, aRespectTimezone, aData) {
> +    if (!this.hasPrivileges)
> +      throw Components.results.NS_ERROR_NOT_IMPLEMENTED;

You don't want to return this error here because the method is actually implemented.
Usually, we assume that if the app was able to get the object, it should just be able to use it so you should not even check what you are checking.

@@ +36,5 @@
> +  add: function add(aDate, aRespectTimezone, aData) {
> +    if (!this.hasPrivileges)
> +      throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
> +
> +    return this.createRequest();

Please, do |return null;| instead. No need to return a request we would leak.

@@ +41,5 @@
> +  },
> +
> +  remove: function remove(aId) {
> +    if (!this.hasPrivileges)
> +      throw Components.results.NS_ERROR_NOT_IMPLEMENTED;

ditto

@@ +48,5 @@
> +  },
> +
> +  getAll: function getAll() {
> +    if (!this.hasPrivileges)
> +      throw Components.results.NS_ERROR_NOT_IMPLEMENTED;

ditto

@@ +50,5 @@
> +  getAll: function getAll() {
> +    if (!this.hasPrivileges)
> +      throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
> +
> +    return this.createRequest();

Please, do |return null;| instead. No need to return a request we would leak.

@@ +65,5 @@
> +
> +    let perm = principal == secMan.getSystemPrincipal() ? 
> +      Ci.nsIPermissionManager.ALLOW_ACTION : Services.perms.testExactPermission(principal.URI, "alarms");
> +
> +    //only pages with perm set can use the alarms

nit: // Only pages [...] alarms.

::: dom/alarm/Makefile.in
@@ +29,5 @@
> +endif
> +
> +# Add VPATH to LOCAL_INCLUDES so we are going to include the correct backend
> +# subdirectory (and the ipc one).
> +LOCAL_INCLUDES += $(VPATH:%=-I%)

You don't need that.

@@ +32,5 @@
> +# subdirectory (and the ipc one).
> +LOCAL_INCLUDES += $(VPATH:%=-I%)
> +
> +include $(topsrcdir)/config/config.mk
> +include $(topsrcdir)/ipc/chromium/chromium-config.mk

I doubt you need that line too.

@@ +35,5 @@
> +include $(topsrcdir)/config/config.mk
> +include $(topsrcdir)/ipc/chromium/chromium-config.mk
> +include $(topsrcdir)/config/rules.mk
> +
> +DEFINES += -D_IMPL_NS_LAYOUT

... and also that one.

::: dom/alarm/test/test_alarm_non_permitted_app.html
@@ +12,5 @@
> +<script type="application/javascript">
> +
> +"use strict";
> +
> +SpecialPowers.setBoolPref("dom.mozAlarms.enabled", true);

Please use pushPrefEnv instead. So, we are sure the pref is pushed back to its previous value.

::: dom/alarm/test/test_alarm_permitted_app.html
@@ +12,5 @@
> +<script type="application/javascript">
> +
> +"use strict";
> +
> +SpecialPowers.setBoolPref("dom.mozAlarms.enabled", true);

Please use pushPrefEnv instead. So, we are sure the pref is pushed back to its previous value.

@@ +13,5 @@
> +
> +"use strict";
> +
> +SpecialPowers.setBoolPref("dom.mozAlarms.enabled", true);
> +SpecialPowers.addPermission("alarms", true, document);

Make sure to remove the permission at the end of the test.

Comment 101

[Gene Lian (I already quit Mozilla)]

Attachment: Part3, AlarmHalService, V3

Just do some rebase things. Please see comment #99 for the main change.

Gene

Comment 102

[Gene Lian (I already quit Mozilla)]

Attachment: Part1, IDL and dummy DOM, V3.2

Thanks Mounir for the reviews :) This new patch addresses the comment #100 as follows:

1. Removed the permission checks for each method, since users should have no chance to call these methods when the navigator.mozAlarms has already been null.

2. Returns null instead of "this.createRequest();" for .add() and .getAll().

3. Changed all the comments to be [upper-case-letter][...][.].

4. Removed "LOCAL_INCLUDES += $(VPATH:%=-I%)" which is unnecessary.

5. Removed "DEFINES += -D_IMPL_NS_LAYOUT" which is unnecessary.

6. Removed "include $(topsrcdir)/ipc/chromium/chromium-config.mk" in this patch (but it will be added later when there is an attempt to use hal:: functions).

7. Used "pushPrefEnv" instead in the test pages to restore the environment.

8. In test_alarm_permitted_app.html, removed the permission at the end of the test.

9. After discussing with Jonas, we still decided to make navigator.mozAlarm visible (but null) to non-B2G apps, which follows the same rule of all the current designs like mozContacts and mozSettings... etc.

Thanks,
Gene

Comment 103

[Mounir Lamouri (:mounir)]

Comment on attachment 636742 [details] [diff] [review]
Part1, IDL and dummy DOM, V3.2

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

r=me with the changes applied.

::: dom/alarm/Makefile.in
@@ +9,5 @@
> +
> +include $(DEPTH)/config/autoconf.mk
> +
> +MODULE              = dom
> +LIBRARY_NAME        = jsdomalarm_s

Name this 'domalarm_s'

@@ +10,5 @@
> +include $(DEPTH)/config/autoconf.mk
> +
> +MODULE              = dom
> +LIBRARY_NAME        = jsdomalarm_s
> +LIBXUL_LIBRARY      = 1

I think you are missing the XPIDL_MODULE.

::: dom/alarm/nsIDOMAlarmsManager.idl
@@ +6,5 @@
> +
> +interface nsIDOMDOMRequest;
> +
> +[scriptable, uuid(fea1e884-9b05-11e1-9b64-87a7016c3860)]
> +interface nsIDOMAlarmsManager : nsISupports

The interface should be prefixed by Moz: nsIDOMMozAlarmsManager.

::: layout/build/Makefile.in
@@ +61,5 @@
>  	$(DEPTH)/view/src/$(LIB_PREFIX)gkview_s.$(LIB_SUFFIX) \
>  	$(DEPTH)/dom/base/$(LIB_PREFIX)jsdombase_s.$(LIB_SUFFIX) \
>  	$(DEPTH)/dom/battery/$(LIB_PREFIX)dom_battery_s.$(LIB_SUFFIX) \
>  	$(DEPTH)/dom/contacts/$(LIB_PREFIX)jsdomcontacts_s.$(LIB_SUFFIX) \
> +	$(DEPTH)/dom/alarm/$(LIB_PREFIX)jsdomalarm_s.$(LIB_SUFFIX) \

s/jsdomalarm/domalarm/

@@ +245,5 @@
>  		   -I$(topsrcdir)/dom/src/storage \
>  		   -I$(topsrcdir)/dom/src/offline \
>  		   -I$(topsrcdir)/dom/src/geolocation \
>  		   -I$(topsrcdir)/dom/contacts \
> +		   -I$(topsrcdir)/dom/alarm \

Why?

Comment 104

[Gene Lian (I already quit Mozilla)]

Attachment: Part1, IDL and dummy DOM, V3.3

Thanks Mounir for the quick review! You must be very busy recently. This new patch addresses the comment #103.

1. Changed nsIDOMAlarmsManager to be nsIDOMMozAlarmsManager.
2. Added "XPIDL_MODULE = dom_alarm" (sorry it's my fault).
3. Used "domalarm_s" instead of "jsdomalarm_s" for the LIBRARY_NAME.
4. Removed "-I$(topsrcdir)/dom/alarm" which seems unnecessary in /layout/build/Makefile.in
5. I found out it'd be better to to add what we've done in the b2g/installer/package-manifest.in to browser/installer/package-manifest.in as well, which follows most of the applications like contacts, settings... etc.

It's close!

Thanks,
Gene

Comment 105

[Gene Lian (I already quit Mozilla)]

Attachment: Part3, AlarmHalService, V3

Cjones,

Just do some rebase things because of the Part1 change. Please see comment #99 for the main change.

Thanks,
Gene

Comment 106

[Mounir Lamouri (:mounir)]

Comment on attachment 637139 [details] [diff] [review]
Part1, IDL and dummy DOM, V3.3

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

BTW, you could remove test_alarm_basics.html, doesn't seem really needed given the check is done in the two other files. If you change that, you don't need to re-ask for a review.

However, you should ask a superreview to Jonas.

Comment 107

[Gene Lian (I already quit Mozilla)]

Attachment: Part1, IDL and dummy DOM, V3.4

Jonas,

All the Alarm API behaviors seem in agreement now :) Need your final super-review on the IDL part (Mounir has already had review+ on this). Thank you!

Gene

Comment 108

[Gene Lian (I already quit Mozilla)]

Attachment: Part1, IDL and dummy DOM, Version for Check-In

Mounir and Jonas have already had reviewed+ on this patch. Created a final patch for check-in, which is merged with the latest codes.

Gene

Comment 109

[Gene Lian (I already quit Mozilla)]

Attachment: Part2, Hal/Gonk, Version for Check-In

Cjones has already had reviewed+ on this patch. Created a final patch for check-in, which is merged with the latest codes.

Gene

Comment 110

[Gene Lian (I already quit Mozilla)]

Attachment: Part3, AlarmHalService, Version for Check-In

Cjones has already had reviewed+ on this patch. Created a final patch for check-in, which is merged with the latest codes.

Gene

Comment 111

[Gene Lian (I already quit Mozilla)]

Attachment: Part4, AlarmDB, Version for Check-In

Vivien has already had reviewed+ on this patch. Created a final patch for check-in, which is merged with the latest codes.

Gene

Comment 112

[Gene Lian (I already quit Mozilla)]

Attachment: Part5, Alarm DOM, Version for Check-In

Vivien has already had reviewed+ on this patch. Created a final patch for check-in, which is merged with the latest codes.

Gene

Comment 113

[Mounir Lamouri (:mounir)]

Pushed in m-i.

Comment 114

[Ed Morley [:emorley]]

Backed out for M3 failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1dba66cfad9a

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

Comment 115

[Gene Lian (I already quit Mozilla)]

Attachment: Part6, System Message Integration, V1

Vivien,

Since the system message mechanism has been done and checked in yesterday, I integrated it into Alarm API logic as well. Some notes:

1. AlarmsManager uses nsIDOMWindowUtils to get the manifest URL of the App.
2. AlarmDB needs to save the manifest URL to be restored later.
3. Use the defineLazyGetter() to get the system message service.
4. When the alarm is fired, use sendMessage() to send an "alarm" message to the manifest URL of that app which has added this alarm.

Could you please have a review on this when you're available?

Thanks,
Gene

Comment 116

[Gene Lian (I already quit Mozilla)]

Attachment: Part1, IDL and dummy DOM, Version for Check-In

Jonas and Mounir have already had reviewed+ on this patch. Created a final patch for check-in, which is merged with the latest codes.

Gene

Comment 117

[Gene Lian (I already quit Mozilla)]

Attachment: Part2, Hal/Gonk, Version for Check-In

Cjones has already had reviewed+ on this patch. Created a final patch for check-in, which is merged with the latest codes.

Gene

Comment 118

[Gene Lian (I already quit Mozilla)]

Attachment: Part3, AlarmHalService, Version for Check-In

Cjones has already had reviewed+ on this patch. Created a final patch for check-in, which is merged with the latest codes.

Gene

Comment 119

[Gene Lian (I already quit Mozilla)]

Attachment: Part4, AlarmDB, Version for Check-In

Vivien has already had reviewed+ on this patch. Created a final patch for check-in, which is merged with the latest codes.

Gene

Comment 120

[Gene Lian (I already quit Mozilla)]

Attachment: Part5, Alarm DOM, Version for Check-In

Vivien has already had reviewed+ on this patch. Created a final patch for check-in, which is merged with the latest codes.

Gene

Comment 121

[Gene Lian (I already quit Mozilla)]

Attachment: Part6, System Message Integration, Version for Check-In

Vivien has already had reviewed+ on this patch. Created a final patch for check-in, which is merged with the latest codes.

Gene

Comment 122

[Kan-Ru Chen [:kanru] (UTC+9)]

wait try result.

Comment 123

[Gene Lian (I already quit Mozilla)]

(In reply to Kan-Ru Chen [:kanru] from comment #122)
> wait try result.

Please see https://tbpl.mozilla.org/?tree=Try&rev=5d95b18b3a27 for the try server result.

Thanks,
Gene

Comment 124

[[:fabrice] Fabrice Desré]

Comment on attachment 639255 [details] [diff] [review]
Part6, System Message Integration, Version for Check-In

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

::: dom/alarm/AlarmsManager.js
@@ +62,5 @@
>  
>      let request = this.createRequest();
>      this._cpmm.sendAsyncMessage(
>        "AlarmsManager:Add", 
> +      { requestID: this.getRequestId(request), date: aDate, ignoreTimezone: isIgnoreTimezone, data: aData, manifestURL: this._manifestURL }

Why do you ever allow this call if manifestURL is null? You're still setting the alarms on the backend, but don't dispatch them. I think we should just return NS_ERROR_FAILURE in this case.

Comment 125

[Gene Lian (I already quit Mozilla)]

(In reply to Fabrice Desré [:fabrice] from comment #124)
> Comment on attachment 639255 [details] [diff] [review]
> Part6, System Message Integration, Version for Check-In
> 
> Review of attachment 639255 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/alarm/AlarmsManager.js
> @@ +62,5 @@
> >  
> >      let request = this.createRequest();
> >      this._cpmm.sendAsyncMessage(
> >        "AlarmsManager:Add", 
> > +      { requestID: this.getRequestId(request), date: aDate, ignoreTimezone: isIgnoreTimezone, data: aData, manifestURL: this._manifestURL }
> 
> Why do you ever allow this call if manifestURL is null? You're still setting
> the alarms on the backend, but don't dispatch them. I think we should just
> return NS_ERROR_FAILURE in this case.

Hi Fabrice! Good point! Actually, I was intentionally doing this because alarms could also be set from the JS console for the convenience of testing purposes. Under this circumstance, it's manifestURL is null. For these cases, what I want to do is simply to observe if the alarms can be set and fired correctly on the backend and don't attempt to send messages for them since they don't have real apps.

Please let me know if it doesn't sound reasonable to you.

Thanks,
Gene

Comment 126

[Gene Lian (I already quit Mozilla)]

Attachment: Part1, IDL and dummy DOM, Version for Check-In

Jonas and Mounir have already had reviewed+ on this patch. Created a final patch for check-in, which is merged with the latest codes.

Gene

Comment 127

[Gene Lian (I already quit Mozilla)]

Could anyone please help me push the patches into mozilla-inbound? I don't have privilege to do so. The try-server result now looks good, which has fixed the previous Mochitest failures (M3 cases).

Thanks,
Gene

Comment 128

[[:fabrice] Fabrice Desré]

(In reply to Gene Lian [:gene] from comment #125)
> (In reply to Fabrice Desré [:fabrice] from comment #124)
> >
> > Why do you ever allow this call if manifestURL is null? You're still setting
> > the alarms on the backend, but don't dispatch them. I think we should just
> > return NS_ERROR_FAILURE in this case.
> 
> Hi Fabrice! Good point! Actually, I was intentionally doing this because
> alarms could also be set from the JS console for the convenience of testing
> purposes. Under this circumstance, it's manifestURL is null. For these
> cases, what I want to do is simply to observe if the alarms can be set and
> fired correctly on the backend and don't attempt to send messages for them
> since they don't have real apps.
> 
> Please let me know if it doesn't sound reasonable to you.

I don't like that to be honest. We'll soon have support for webapps in the test profile so we won't need any such hacks to test properly. Can you update your patches or file a follow-up?

Comment 129

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b3d4127dd8ce
https://hg.mozilla.org/mozilla-central/rev/7b992fc42d0a
https://hg.mozilla.org/mozilla-central/rev/7ddee2b0e868
https://hg.mozilla.org/mozilla-central/rev/09aed6c9f4a0
https://hg.mozilla.org/mozilla-central/rev/6474c913b05f
https://hg.mozilla.org/mozilla-central/rev/c28e5bf6cc40

Comment 130

[Gene Lian (I already quit Mozilla)]

(In reply to Fabrice Desré [:fabrice] from comment #128)
> (In reply to Gene Lian [:gene] from comment #125)
> > (In reply to Fabrice Desré [:fabrice] from comment #124)
> > >
> > > Why do you ever allow this call if manifestURL is null? You're still setting
> > > the alarms on the backend, but don't dispatch them. I think we should just
> > > return NS_ERROR_FAILURE in this case.
> > 
> > Hi Fabrice! Good point! Actually, I was intentionally doing this because
> > alarms could also be set from the JS console for the convenience of testing
> > purposes. Under this circumstance, it's manifestURL is null. For these
> > cases, what I want to do is simply to observe if the alarms can be set and
> > fired correctly on the backend and don't attempt to send messages for them
> > since they don't have real apps.
> > 
> > Please let me know if it doesn't sound reasonable to you.
> 
> I don't like that to be honest. We'll soon have support for webapps in the
> test profile so we won't need any such hacks to test properly. Can you
> update your patches or file a follow-up?

Fabrice,

Thanks for your solutions. That would be indeed better if we could use such a testing mechanism. I'll file another follow-up later to check and avoid adding alarms from JS console. However, don't worry about the current checked-in codes, which should have no harm at all I believe. ;)

Thanks,
Gene

Comment 131

[Jeremie Patonnier :Jeremie]

Documentation available here:
https://developer.mozilla.org/en-US/docs/tag/alarm