Last Comment Bug 772369 - Alarm API - Follow-Up Fix for System Message Integration
: Alarm API - Follow-Up Fix for System Message Integration
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Gene Lian [:gene] (I already quit Mozilla)
:
Mentors:
Depends on:
Blocks: alarm-api
  Show dependency treegraph
 
Reported: 2012-07-10 02:24 PDT by Gene Lian [:gene] (I already quit Mozilla)
Modified: 2012-07-25 18:54 PDT (History)
5 users (show)
airpingu: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP Patch (5.41 KB, patch)
2012-07-16 20:19 PDT, Gene Lian [:gene] (I already quit Mozilla)
no flags Details | Diff | Splinter Review
Patch (3.09 KB, patch)
2012-07-17 01:52 PDT, Gene Lian [:gene] (I already quit Mozilla)
no flags Details | Diff | Splinter Review
Patch, V2 (3.68 KB, patch)
2012-07-17 03:44 PDT, Gene Lian [:gene] (I already quit Mozilla)
fabrice: review+
Details | Diff | Splinter Review

Description Gene Lian [:gene] (I already quit Mozilla) 2012-07-10 02:24:50 PDT
Need to add sanity checks to avoid adding alarms when the app's manifestURL is null. Please see Fabrice's suggestion at https://bugzilla.mozilla.org/show_bug.cgi?id=749551#c130.
Comment 1 Gene Lian [:gene] (I already quit Mozilla) 2012-07-16 20:19:23 PDT
Created attachment 642849 [details] [diff] [review]
WIP Patch

Fabrice,

May I invite you to be the reviewer of the System Message integration for Alarm APIs? This following summarizes what I've changed in this patch:

1. In the content process (AlarmsManager.js), the aWindow.document.nodePrincipal.URI.spec is saved and sent to parent process when adding alarms.

2. In the parent process (AlarmService.jsm), when alarm is fired, I used Services.io.newURI() to create a new manifestURI for sending system messages.

3. Add a sanity check to avoid adding alarms from a page without valid URI.

4. s/manifestURL/URISpec


Questions:

1. As discussed in the e-mail, it seems the the aManifestURI.spec passed into SystemMessageInternal.sendMessage() is still http://clock.gaiamobile.org/, instead of http://clock.gaiamobile.org/manifest.webapp as expected. Is it correct to use aWindow.document.nodePrincipal.URI.spec to get URI?

2. Although the aManifestURI.spec could be wrong, it seems the SystemMessageManager has never been started because the debug messages within the SystemMessageManager.init() didn't show (debug is already turned on). That's why SystemMessageManager.receiveMessage() cannot receive any messages sent from ppmm.sendAsyncMessage("SystemMessageManager:Message", ...). How should we correctly launch the SystemMessageManager?

Thanks,
Gene
Comment 2 Gene Lian [:gene] (I already quit Mozilla) 2012-07-17 01:52:47 PDT
Created attachment 642892 [details] [diff] [review]
Patch

Fabrice,

May I invite you to be the reviewer of the System Message integration for Alarm APIs? The following summarizes what I've changed in this patch:

1. In the content process (AlarmsManager.js), the |utils.getApp().manifestURL| is saved and sent to parent process when adding alarms.

2. In the parent process (AlarmService.jsm), when alarm is fired, I used Services.io.newURI() to create a new manifestURI for sending system messages. Don't need to check if the manifestURL is null or not because of item #3.

3. Disable the Alarm APIs (i.e. set navigator.mozAlarms to null) when the page has no valid manifestURL, so that clients won't have chances to add an alarm that had no manifestURL.

Thanks,
Gene
Comment 3 Gene Lian [:gene] (I already quit Mozilla) 2012-07-17 03:20:08 PDT
Comment on attachment 642892 [details] [diff] [review]
Patch

Oops... This one cannot be passed by Mochitest which doesn't always use installed apps for testing. I'll upload another solution later. Sorry for bugging.

Gene
Comment 4 Gene Lian [:gene] (I already quit Mozilla) 2012-07-17 03:44:43 PDT
Created attachment 642900 [details] [diff] [review]
Patch, V2

Fabrice,

May I invite you to be the reviewer of the System Message integration for Alarm APIs? The following summarizes what I've changed in this patch:

1. In the content process (AlarmsManager.js), the |utils.getApp().manifestURL| is saved and sent to parent process when adding alarms.

2. In the parent process (AlarmService.jsm), when alarm is fired, I used Services.io.newURI() to create a new manifestURI for sending system messages. Don't need to check if the manifestURL is null or not because of item #3.

3. Add an sanity check to avoid adding alarms for an non-installed app which can ensure all the added alarms are associated with an valid manifestURL for sending system messages.

Thanks,
Gene
Comment 5 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-07-24 02:07:03 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/44e8a79e4be3
Comment 6 Ed Morley [:emorley] 2012-07-25 08:13:44 PDT
https://hg.mozilla.org/mozilla-central/rev/44e8a79e4be3

Note You need to log in before you can comment on or make changes to this bug.