superreview requested: [Bug 749551] Alarm API : [Attachment 627135] Part4, AlarmDB

[bugzill...@mozilla.org]

Gene Lian [:gene] <cl...@mozilla.com> has asked Mounir Lamouri (:volkmar)
(:mounir) <mou...@lamouri.fr> for superreview:
Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 627135: Part4, AlarmDB
https://bugzilla.mozilla.org/attachment.cgi?id=627135&action=edit


------- Additional Comments from Gene Lian [:gene] <cl...@mozilla.com>
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

[bugzill...@mozilla.org]

Gene Lian [:gene] <cl...@mozilla.com> has asked Mounir Lamouri (:volkmar)
(:mounir) <mou...@lamouri.fr> for superreview:
Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 627140: Part5, Alarm DOM
https://bugzilla.mozilla.org/attachment.cgi?id=627140&action=edit

------- Additional Comments from Gene Lian [:gene] <cl...@mozilla.com>
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

[bugzill...@mozilla.org]

Mounir Lamouri (:volkmar) (:mounir) <mou...@lamouri.fr> has reassigned Gene
Lian [:gene] <cl...@mozilla.com>'s request for superreview to Jonas Sicking
(:sicking) <jo...@sicking.cc>:

Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 627135: Part4, AlarmDB
https://bugzilla.mozilla.org/attachment.cgi?id=627135&action=edit

------- Additional Comments from Mounir Lamouri (:volkmar) (:mounir)
<mou...@lamouri.fr>
I'm no super-reviewer. Redirecting that to Jonas.

[bugzill...@mozilla.org]

Mounir Lamouri (:volkmar) (:mounir) <mou...@lamouri.fr> has reassigned Gene
Lian [:gene] <cl...@mozilla.com>'s request for superreview to Jonas Sicking
(:sicking) <jo...@sicking.cc>:
Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

[bugzill...@mozilla.org]

Jonas Sicking (:sicking) <jo...@sicking.cc> has canceled Gene Lian [:gene]
<cl...@mozilla.com>'s request for superreview:

Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 627135: Part4, AlarmDB
https://bugzilla.mozilla.org/attachment.cgi?id=627135&action=edit

------- Additional Comments from Jonas Sicking (:sicking) <jo...@sicking.cc>
Review of attachment 627135:
-----------------------------------------------------------------

Clearing these until there's an r+

[bugzill...@mozilla.org]

Jonas Sicking (:sicking) <jo...@sicking.cc> has canceled Gene Lian [:gene]
<cl...@mozilla.com>'s request for superreview:
Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

[bugzill...@mozilla.org]

Gene Lian [:gene] <cl...@mozilla.com> has asked Chris Jones [:cjones]
[:warhammer] <jones....@gmail.com> for superreview:

Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 629128: Part2, Hal/Gonk
https://bugzilla.mozilla.org/attachment.cgi?id=629128&action=edit

------- Additional Comments from Gene Lian [:gene] <cl...@mozilla.com>

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

[bugzill...@mozilla.org]

Gene Lian [:gene] <cl...@mozilla.com> has asked Chris Jones [:cjones]
[:warhammer] <jones....@gmail.com> for superreview:
Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 629129: Part3, AlarmHalService
https://bugzilla.mozilla.org/attachment.cgi?id=629129&action=edit

------- Additional Comments from Gene Lian [:gene] <cl...@mozilla.com>

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

[bugzill...@mozilla.org]

Gene Lian [:gene] <cl...@mozilla.com> has asked Jonas Sicking (:sicking)
<jo...@sicking.cc> for superreview:

Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 629131: Part4, AlarmDB
https://bugzilla.mozilla.org/attachment.cgi?id=629131&action=edit

------- Additional Comments from Gene Lian [:gene] <cl...@mozilla.com>

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

[bugzill...@mozilla.org]

Gene Lian [:gene] <cl...@mozilla.com> has asked Jonas Sicking (:sicking)
<jo...@sicking.cc> for superreview:
Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 629135: Part5, Alarm DOM
https://bugzilla.mozilla.org/attachment.cgi?id=629135&action=edit

------- Additional Comments from Gene Lian [:gene] <cl...@mozilla.com>
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

[bugzill...@mozilla.org]

Chris Jones [:cjones] [:warhammer] <jones....@gmail.com> has not granted
Gene Lian [:gene] <cl...@mozilla.com>'s request for superreview:

Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 629128: Part2, Hal/Gonk
https://bugzilla.mozilla.org/attachment.cgi?id=629128&action=edit

------- Additional Comments from Chris Jones [:cjones] [:warhammer]
<jones....@gmail.com>
This patch is better, but there are some issues I'd like to see fixed. Please
see the suggested patch.

Thanks!

[bugzill...@mozilla.org]

Chris Jones [:cjones] [:warhammer] <jones....@gmail.com> has granted Gene

Lian [:gene] <cl...@mozilla.com>'s request for superreview:
Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 629129: Part3, AlarmHalService
https://bugzilla.mozilla.org/attachment.cgi?id=629129&action=edit

------- Additional Comments from Chris Jones [:cjones] [:warhammer]
<jones....@gmail.com>

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.

[bugzill...@mozilla.org]

Gene Lian [:gene] <cl...@mozilla.com> has asked Chris Jones [:cjones]
[:warhammer] <jones....@gmail.com> for superreview:

Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 629745: Part2, Hal/Gonk
https://bugzilla.mozilla.org/attachment.cgi?id=629745&action=edit

------- Additional Comments from Gene Lian [:gene] <cl...@mozilla.com>

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

[bugzill...@mozilla.org]

Gene Lian [:gene] <cl...@mozilla.com> has asked Chris Jones [:cjones]
[:warhammer] <jones....@gmail.com> for superreview:
Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 629746: Part3, AlarmHalService
https://bugzilla.mozilla.org/attachment.cgi?id=629746&action=edit

------- Additional Comments from Gene Lian [:gene] <cl...@mozilla.com>

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

[bugzill...@mozilla.org]

Mounir Lamouri (:volkmar) (:mounir) <mou...@lamouri.fr> has canceled Gene Lian
[:gene] <cl...@mozilla.com>'s request for superreview:

Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 629745: Part2, Hal/Gonk
https://bugzilla.mozilla.org/attachment.cgi?id=629745&action=edit

------- Additional Comments from Mounir Lamouri (:volkmar) (:mounir)
<mou...@lamouri.fr>
Review of attachment 629745:
-----------------------------------------------------------------

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

[bugzill...@mozilla.org]

Mounir Lamouri (:volkmar) (:mounir) <mou...@lamouri.fr> has canceled Gene Lian
[:gene] <cl...@mozilla.com>'s request for superreview:
Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 629746: Part3, AlarmHalService
https://bugzilla.mozilla.org/attachment.cgi?id=629746&action=edit

------- Additional Comments from Mounir Lamouri (:volkmar) (:mounir)
<mou...@lamouri.fr>

Review of attachment 629746:
-----------------------------------------------------------------

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.

[bugzill...@mozilla.org]

Gene Lian [:gene] <cl...@mozilla.com> has canceled Gene Lian [:gene]

<cl...@mozilla.com>'s request for superreview:
Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 629131: Part4, AlarmDB
https://bugzilla.mozilla.org/attachment.cgi?id=629131&action=edit

------- Additional Comments from Gene Lian [:gene] <cl...@mozilla.com>

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

[bugzill...@mozilla.org]

Gene Lian [:gene] <cl...@mozilla.com> has asked Jonas Sicking (:sicking)

<jo...@sicking.cc> for superreview:

Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 630517: Part4, AlarmDB, V2
https://bugzilla.mozilla.org/attachment.cgi?id=630517&action=edit

[bugzill...@mozilla.org]

Gene Lian [:gene] <cl...@mozilla.com> has canceled Gene Lian [:gene]

<cl...@mozilla.com>'s request for superreview:

Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 629135: Part5, Alarm DOM
https://bugzilla.mozilla.org/attachment.cgi?id=629135&action=edit

------- Additional Comments from Gene Lian [:gene] <cl...@mozilla.com>

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

[bugzill...@mozilla.org]

Gene Lian [:gene] <cl...@mozilla.com> has asked Jonas Sicking (:sicking)

<jo...@sicking.cc> for superreview:

Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 630527: Part5, Alarm DOM, V2
https://bugzilla.mozilla.org/attachment.cgi?id=630527&action=edit

[bugzill...@mozilla.org]

Gene Lian [:gene] <cl...@mozilla.com> has canceled Gene Lian [:gene]

<cl...@mozilla.com>'s request for superreview:

Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 630517: Part4, AlarmDB, V2
https://bugzilla.mozilla.org/attachment.cgi?id=630517&action=edit

------- Additional Comments from Gene Lian [:gene] <cl...@mozilla.com>

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

[bugzill...@mozilla.org]

Gene Lian [:gene] <cl...@mozilla.com> has canceled Gene Lian [:gene]
<cl...@mozilla.com>'s request for superreview:
Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 630527: Part5, Alarm DOM, V2
https://bugzilla.mozilla.org/attachment.cgi?id=630527&action=edit

------- Additional Comments from Gene Lian [:gene] <cl...@mozilla.com>

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

[bugzill...@mozilla.org]

Gene Lian [:gene] <cl...@mozilla.com> has asked Jonas Sicking (:sicking)

Vacation June 11-22 <jo...@sicking.cc> for superreview:

Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 634835: Part1, IDL and dummy DOM, V3.1
https://bugzilla.mozilla.org/attachment.cgi?id=634835&action=edit

------- Additional Comments from Gene Lian [:gene] <cl...@mozilla.com>

Please see the responses in comment #91.

Thanks,
Gene

[bugzill...@mozilla.org]

Mounir Lamouri (:mounir) <mou...@lamouri.fr> has canceled Gene Lian [:gene]
<cl...@mozilla.com>'s request for superreview:

Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 634835: Part1, IDL and dummy DOM, V3.1
https://bugzilla.mozilla.org/attachment.cgi?id=634835&action=edit

------- Additional Comments from Mounir Lamouri (:mounir) <mou...@lamouri.fr>
Review of attachment 634835:
-----------------------------------------------------------------

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.

[bugzill...@mozilla.org]

Gene Lian [:gene] <cl...@mozilla.com> has asked Jonas Sicking (:sicking)

Vacation June 11-22 <jo...@sicking.cc> for superreview:

Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 636742: Part1, IDL and dummy DOM, V3.2
https://bugzilla.mozilla.org/attachment.cgi?id=636742&action=edit

------- Additional Comments from Gene Lian [:gene] <cl...@mozilla.com>

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

[bugzill...@mozilla.org]

Gene Lian [:gene] <cl...@mozilla.com> has canceled Gene Lian [:gene]
<cl...@mozilla.com>'s request for superreview:

Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 636742: Part1, IDL and dummy DOM, V3.2
https://bugzilla.mozilla.org/attachment.cgi?id=636742&action=edit


------- Additional Comments from Gene Lian [:gene] <cl...@mozilla.com>

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

[bugzill...@mozilla.org]

Gene Lian [:gene] <cl...@mozilla.com> has asked Jonas Sicking (:sicking)

Vacation June 11-22 <jo...@sicking.cc> for superreview:

Bug 749551: Alarm API
https://bugzilla.mozilla.org/show_bug.cgi?id=749551

Attachment 637409: Part1, IDL and dummy DOM, V3.4
https://bugzilla.mozilla.org/attachment.cgi?id=637409&action=edit

------- Additional Comments from Gene Lian [:gene] <cl...@mozilla.com>

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

[bugzill...@mozilla.org]

Jonas Sicking (:sicking) <jo...@sicking.cc> has granted Gene Lian [:gene]
<cl...@mozilla.com>'s request for superreview: