Closed
Bug 772369
Opened 12 years ago
Closed 12 years ago
Alarm API - Follow-Up Fix for System Message Integration
Categories
(Core :: DOM: Device Interfaces, defect)
Core
DOM: Device Interfaces
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: airpingu, Assigned: airpingu)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
fabrice
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•12 years ago
|
Summary: Alarm API - Avoid Adding Alarms when App's ManifestURL is Null → Alarm API - Follow-Up Fix for System Message Integration
Assignee | ||
Comment 1•12 years ago
|
||
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
Attachment #642849 -
Flags: review?(fabrice)
Assignee | ||
Updated•12 years ago
|
Attachment #642849 -
Flags: review?(fabrice)
Assignee | ||
Comment 2•12 years ago
|
||
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
Attachment #642849 -
Attachment is obsolete: true
Attachment #642892 -
Flags: review?(fabrice)
Assignee | ||
Comment 3•12 years ago
|
||
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
Attachment #642892 -
Flags: review?(fabrice)
Assignee | ||
Comment 4•12 years ago
|
||
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
Attachment #642892 -
Attachment is obsolete: true
Attachment #642900 -
Flags: review?(fabrice)
Updated•12 years ago
|
Attachment #642900 -
Flags: review?(fabrice) → review+
Assignee | ||
Updated•12 years ago
|
Flags: in-testsuite-
Keywords: checkin-needed
Whiteboard: Try-server passed: https://tbpl.mozilla.org/?tree=Try&rev=978817c3c6ee
Keywords: checkin-needed
Target Milestone: --- → mozilla17
Comment 6•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: Try-server passed: https://tbpl.mozilla.org/?tree=Try&rev=978817c3c6ee → Try-server passed: https://tbpl.mozilla.org/?tree=Try&rev=978817c3c6ee
Assignee | ||
Updated•12 years ago
|
Whiteboard: Try-server passed: https://tbpl.mozilla.org/?tree=Try&rev=978817c3c6ee
You need to log in
before you can comment on or make changes to this bug.
Description
•