Closed
Bug 1018320
Opened 11 years ago
Closed 10 years ago
Implement RequestSync API for FirefoxOS
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
People
(Reporter: jedp, Assigned: baku)
References
Details
(Keywords: dev-doc-needed)
Attachments
(8 files, 30 obsolete files)
(deleted),
patch
|
fabrice
:
review+
bajaj
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bajaj
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ehsan.akhgari
:
review+
bajaj
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bajaj
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bajaj
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ehsan.akhgari
:
review+
bajaj
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ehsan.akhgari
:
review+
bajaj
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
To provide a background sync scheduler service for FirefoxOS. More about the proposed architecture can be found on the wiki [1].
A full implementation of the BackgroundSync API [2] will require ServiceWorkers (bug 903441). In the interim, we ought to be able to move ahead with an implementation that uses system messages in the manner of the Alarm API.
[1] https://wiki.mozilla.org/Cloud_Services/FirefoxOS_Sync
[2] https://github.com/slightlyoff/BackgroundSync/blob/master/explainer.md
Comment 1•11 years ago
|
||
I think this warrants an intent to implement thread... Thanks!
Reporter | ||
Comment 2•11 years ago
|
||
Thanks, Ehsan. I'll begin a thread on this.
Reporter | ||
Comment 3•11 years ago
|
||
Initial sketch (wip - doesn't actually work :)
Reporter | ||
Comment 4•11 years ago
|
||
Attachment #8431786 -
Attachment is obsolete: true
Reporter | ||
Comment 5•11 years ago
|
||
Attachment #8431852 -
Attachment is obsolete: true
Reporter | ||
Comment 6•11 years ago
|
||
Attachment #8431878 -
Attachment is obsolete: true
Reporter | ||
Comment 7•11 years ago
|
||
Carlos and I are working on this together in his gecko-dev git repo:
https://github.com/carlosdp/gecko-dev/tree/request-sync
Reporter | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Updated•11 years ago
|
Keywords: dev-doc-needed
Reporter | ||
Comment 8•11 years ago
|
||
wip
confused as to why this loads the DOM module successfully on desktop firefox, but when we try in b2g, we crash when the DOM module tries to touch ppmm. DOM jsm is not process-aware, so it's getting instantiated in child process? But if so, why doesn't this explode on desktop firefox, too?
Attachment #8431895 -
Attachment is obsolete: true
Reporter | ||
Comment 9•11 years ago
|
||
Problem in comment 8 solved by :carlosdp
This patch is a squash of our working tree, which is here:
https://github.com/carlosdp/gecko-dev/tree/request-sync
Provides basic functionality for a gaia app:
- app calls navigator.syncScheduler.requestSync("foo");
- scheduler emits a "sync" system message back to the app
We're working on the scheduler now, but in this rudimentary form, it should already help unblock apps that want to test this sync workflow.
Here's a dev app for testing:
https://github.com/jedp/gaia/tree/sync-thing
Attachment #8437180 -
Attachment is obsolete: true
Reporter | ||
Updated•10 years ago
|
Summary: Implement navigator.requestSync and .unregisterSync → Implement BackgroundSync API for FirefoxOS
Reporter | ||
Comment 10•10 years ago
|
||
Current working branch is now Carlos's sync-db, not request-sync
Reporter | ||
Comment 11•10 years ago
|
||
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #1)
> I think this warrants an intent to implement thread... Thanks!
Intent-to-Implement thread was opened on June 11:
https://groups.google.com/d/msg/mozilla.dev.webapi/3LbVbgJgaIQ/OHjIwUhc3yQJ
Sorry I neglected to attach the link sooner.
Comment 12•10 years ago
|
||
Hey Jonas,
This is the WIP sync scheduler implementation for the requestSync spec. There are a lot of rough edges still in error handling and scheduling is very naive, but we wanted to submit it for feedback at this mid-stage to make sure we are on a good path.
Let me know what you think of where we are so far!
Thanks,
Carlos
Assignee: jed+bmo → cpadron
Attachment #8437342 -
Attachment is obsolete: true
Attachment #8452013 -
Flags: feedback?(jonas)
Comment on attachment 8452013 [details] [diff] [review]
SyncScheduler (WIP)
Review of attachment 8452013 [details] [diff] [review]:
-----------------------------------------------------------------
I think the general direction here is great. I haven't looked too much at the actual implementation though.
::: dom/webidl/SyncScheduler.webidl
@@ +14,5 @@
> + IntervalOption interval;
> + boolean repeating;
> + short minInterval;
> + boolean wifiOnly;
> + any data;
This seems unnecessarily different from the options mentioned in:
https://github.com/slightlyoff/BackgroundSync/blob/master/explainer.md
I do like the 'wifiOnly' option though. As long as it's considered just a hint and that it's ok if we expose UI which allows the user to override.
I don't understand the purpose of 'interval'. Seems like a less flexible way of describing what's in 'minInterval'?
Attachment #8452013 -
Flags: feedback?(jonas)
Updated•10 years ago
|
Component: FxA → DOM
Product: Firefox OS → Core
Assignee | ||
Updated•10 years ago
|
Assignee: cpadron → amarchesini
Assignee | ||
Comment 14•10 years ago
|
||
I'm working on this API. Here a draft of the implementation that does all the webIDL interfaces, plus a test to check them.
Jonas, I would like to have a feedback about the webIDL part. Thanks.
Attachment #8452013 -
Attachment is obsolete: true
Attachment #8513537 -
Flags: feedback?(jopsen)
Assignee | ||
Updated•10 years ago
|
Attachment #8513537 -
Flags: feedback?(jopsen) → feedback?(jonas)
Comment on attachment 8513537 [details] [diff] [review]
WIP
We covered this over vidyo this morning.
Attachment #8513537 -
Flags: feedback?(jonas)
Assignee | ||
Comment 16•10 years ago
|
||
This first patch does:
1. expose the BackgroundSyncScheduler interface as navigator.sync - behind prefs just for certifiedApps
2. expose the BackgroundSyncManager interface as navigator.syncManager - behind prefs and with backgroundsync-manager permission
3. the syncManager interface allows just to retrieve all the registations (I needs it for debug reasons) but not editing.
4. Dispatching system messages respecting the minInterval and other stuffs but not wifiOnly.
5. tests for all these 4 points.
In the next patches I'll do wifiOnly and more of the syncManager API.
Attachment #8513537 -
Attachment is obsolete: true
Attachment #8517558 -
Flags: review?(jonas)
Assignee | ||
Comment 17•10 years ago
|
||
Writing a test for this is tricky because I have to play with preferences but I cannot test if the hal layer is sending the correct notifications.
Attachment #8518206 -
Flags: review?(jonas)
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8519012 -
Flags: review?(jonas)
Assignee | ||
Comment 20•10 years ago
|
||
BackgroundSyncAPI => RequestSyncAPI
Attachment #8517558 -
Attachment is obsolete: true
Attachment #8517558 -
Flags: review?(jonas)
Attachment #8522286 -
Flags: review?(jonas)
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8518206 -
Attachment is obsolete: true
Attachment #8518206 -
Flags: review?(jonas)
Attachment #8522288 -
Flags: review?(jonas)
Assignee | ||
Updated•10 years ago
|
Summary: Implement BackgroundSync API for FirefoxOS → Implement RequestSync API for FirefoxOS
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8522286 -
Attachment is obsolete: true
Attachment #8522286 -
Flags: review?(jonas)
Attachment #8522325 -
Flags: review?(jonas)
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8522288 -
Attachment is obsolete: true
Attachment #8522288 -
Flags: review?(jonas)
Attachment #8522326 -
Flags: review?(jonas)
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8522329 -
Flags: review?(jonas)
Assignee | ||
Comment 25•10 years ago
|
||
no dynamic message name.
Attachment #8522325 -
Attachment is obsolete: true
Attachment #8522325 -
Flags: review?(jonas)
Attachment #8523504 -
Flags: review?(jonas)
Assignee | ||
Comment 26•10 years ago
|
||
rebased
Attachment #8522326 -
Attachment is obsolete: true
Attachment #8522326 -
Flags: review?(jonas)
Attachment #8523505 -
Flags: review?(jonas)
Assignee | ||
Comment 27•10 years ago
|
||
rebased
Attachment #8519012 -
Attachment is obsolete: true
Attachment #8519012 -
Flags: review?(jonas)
Attachment #8523506 -
Flags: review?(jonas)
Assignee | ||
Comment 28•10 years ago
|
||
Attachment #8522329 -
Attachment is obsolete: true
Attachment #8522329 -
Flags: review?(jonas)
Attachment #8523507 -
Flags: review?(jonas)
Assignee | ||
Comment 29•10 years ago
|
||
This is the last patch for this API.
Attachment #8523509 -
Flags: review?(jonas)
Assignee | ||
Comment 30•10 years ago
|
||
Actually I forgot the 'setPolicy' method in the RequestSyncManager. Here the patch.
Attachment #8523906 -
Flags: review?(jonas)
Updated•10 years ago
|
feature-b2g: --- → 2.2?
Comment on attachment 8523506 [details] [diff] [review]
patch 3 - promise in sendMessage()
Review of attachment 8523506 [details] [diff] [review]:
-----------------------------------------------------------------
Fabrice should do this one
Attachment #8523506 -
Flags: review?(jonas) → review?(fabrice)
Comment on attachment 8523509 [details] [diff] [review]
patch 5 - mozSetMessageHandlerPromise
Review of attachment 8523509 [details] [diff] [review]:
-----------------------------------------------------------------
Fabrice needs to review this one too. I don't know this code well enough sadly.
::: dom/messages/SystemMessageManager.js
@@ +118,5 @@
> + debug("No promise set, sending the response immediately");
> + sendResponse();
> + } else {
> + debug("Using the promise to postpone the response.");
> + this._promise.then(sendResponse, sendResponse);
If the promise is rejected, we should reject the promise that was returned from nsISystemMessagesInternal.sendMessage(). That doesn't seem to happen here?
We likely should also reject if calling the message message handler throws an exception. I'm not sure if earlier patches did that?
Attachment #8523509 -
Flags: review?(jonas) → review?(fabrice)
Assignee | ||
Comment 33•10 years ago
|
||
Here what Jonas suggested.
Attachment #8526698 -
Flags: review?(fabrice)
Assignee | ||
Comment 34•10 years ago
|
||
A small bugfix in the unregistering of the task: the timer has to be canceled and the object has to be removed from the queue of pending tasks.
Attachment #8523507 -
Attachment is obsolete: true
Attachment #8523507 -
Flags: review?(jonas)
Attachment #8526709 -
Flags: review?(jonas)
Assignee | ||
Updated•10 years ago
|
Attachment #8526709 -
Attachment description: use.patch → patch 4 - use of promise
Comment 35•10 years ago
|
||
Comment on attachment 8523506 [details] [diff] [review]
patch 3 - promise in sendMessage()
Review of attachment 8523506 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/messages/SystemMessageInternal.js
@@ +84,5 @@
> this._cpuWakeLocks = {};
>
> this._configurators = {};
>
> + this._pendingPromises = {};
Use a Map instead.
Attachment #8523506 -
Flags: review?(fabrice) → feedback+
Assignee | ||
Comment 36•10 years ago
|
||
Attachment #8523506 -
Attachment is obsolete: true
Attachment #8527159 -
Flags: review?(fabrice)
Assignee | ||
Comment 37•10 years ago
|
||
Attachment #8526709 -
Attachment is obsolete: true
Attachment #8526709 -
Flags: review?(jonas)
Attachment #8527160 -
Flags: review?(jonas)
Assignee | ||
Comment 38•10 years ago
|
||
Attachment #8526698 -
Attachment is obsolete: true
Attachment #8526698 -
Flags: review?(fabrice)
Attachment #8527161 -
Flags: review?(fabrice)
Updated•10 years ago
|
QA Whiteboard: [2.2-feature-qa+]
Assignee | ||
Comment 39•10 years ago
|
||
Ehsan, do you mind to help Jonas to review this API?
Flags: needinfo?(ehsan.akhgari)
Assignee | ||
Updated•10 years ago
|
Attachment #8523504 -
Flags: review?(jonas) → review?(ehsan.akhgari)
Assignee | ||
Updated•10 years ago
|
Attachment #8523505 -
Flags: review?(jonas) → review?(ehsan.akhgari)
Assignee | ||
Updated•10 years ago
|
Attachment #8523906 -
Flags: review?(jonas) → review?(ehsan.akhgari)
Assignee | ||
Updated•10 years ago
|
Attachment #8527160 -
Flags: review?(jonas) → review?(ehsan.akhgari)
Updated•10 years ago
|
Flags: needinfo?(ehsan.akhgari)
Comment 40•10 years ago
|
||
Comment on attachment 8523509 [details] [diff] [review]
patch 5 - mozSetMessageHandlerPromise
Review of attachment 8523509 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with Jonas's questions addressed
Attachment #8523509 -
Flags: review?(fabrice) → review+
Comment 41•10 years ago
|
||
Comment on attachment 8527159 [details] [diff] [review]
patch 3 - promise in sendMessage()
Review of attachment 8527159 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/messages/SystemMessageInternal.js
@@ +179,5 @@
> },
>
> sendMessage: function(aType, aMessage, aPageURI, aManifestURI, aExtra) {
> + let self = this;
> + return new Promise(function(aResolve, aReject) {
nit: (aResolve, aReject) => { } so you don't have to use self = this
Attachment #8527159 -
Flags: review?(fabrice) → review+
Updated•10 years ago
|
Attachment #8527161 -
Flags: review?(fabrice) → review+
Assignee | ||
Comment 42•10 years ago
|
||
Attachment #8527159 -
Attachment is obsolete: true
Comment 43•10 years ago
|
||
Comment on attachment 8523504 [details] [diff] [review]
patch 1 - basic API
Review of attachment 8523504 [details] [diff] [review]:
-----------------------------------------------------------------
Sorry for the delay. Please submit interdiffs for next iterations. Thanks!
::: dom/requestsync/RequestSyncService.jsm
@@ +89,5 @@
> + debug("initialization done");
> + },
> + function() {
> + dump("ERROR!! RequestSyncService - Failed to retrieve data from the database.\n");
> + });
Shouldn't we also do things such as rescheduling timers after the initialization is done, to keep this working across device restarts, for example?
@@ +137,5 @@
> + clearData: function(aData) {
> + debug('clearData');
> +
> + let params =
> + aData.QueryInterface(Ci.mozIApplicationClearPrivateDataParams);
Perhaps you should null check aData too.
@@ +228,5 @@
> + if (this._registrations[aKey][aTaskName].timer) {
> + this._registrations[aKey][aTaskName].timer.cancel();
> + }
> +
> + delete this._registrations[aKey][aTaskName];
Not sure if this makes a difference, but should you remove _registrations[aKey] as well if it's empty?
@@ +251,5 @@
> + let uri = Services.io.newURI(aMessage.principal.origin, null, null);
> + let principal = secMan.getAppCodebasePrincipal(uri,
> + aMessage.principal.appId, aMessage.principal.isInBrowserElement);
> + if (!principal) {
> + return;
If this fails, it will throw, so you should handle that here.
@@ +296,5 @@
> +
> + let minInterval = BSYNC_MIN_INTERVAL;
> + try {
> + let tmp = Services.prefs.getIntPref("dom.requestSync.minInterval");
> + minInterval = tmp;
Why not assign to minInterval directly?
@@ +326,5 @@
> + this.removeRegistrationInternal(aData.task, key);
> + }
> +
> + // This creates a RequestTaskFull object.
> + aData.params.task = aData.task;
What guarantees that aData.params is an object at this point?
::: dom/requestsync/tests/mochitest.ini
@@ +1,2 @@
> +[DEFAULT]
> +skip-if = e10s
What makes this not work with e10s, out of curiosity?
@@ +6,5 @@
> + file_basic_app.html
> + common_app.js
> + common_basic.js
> +
> +[test_interfaces.html]
Nit: can you please call this something else?
::: dom/requestsync/tests/test_wakeUp.html
@@ +84,5 @@
> + }, genericError);
> + }
> +
> + function test_wait() {
> + // nothing to do here.
So what's the point? ;-)
::: dom/webidl/RequestSyncManager.webidl
@@ +15,5 @@
> +dictionary RequestTaskFull : RequestTask {
> + RequestTaskApp app;
> +};
> +
> +[NoInterfaceObject, NavigatorProperty="syncManager",
Please add a comment that this is only going to be used in the system app.
::: dom/webidl/RequestSyncScheduler.webidl
@@ +18,5 @@
> +dictionary RequestTask : RequestTaskParams {
> + ScalarValueString task = "";
> +
> + // Last synchonization date.. maybe it's useful to know.
> + Date lastSync;
Please use DOMTimeStamp instead.
@@ +21,5 @@
> + // Last synchonization date.. maybe it's useful to know.
> + Date lastSync;
> +};
> +
> +[NoInterfaceObject, NavigatorProperty="sync",
Please remove [NoInterfaceObject].
Attachment #8523504 -
Flags: review?(ehsan.akhgari) → review-
Comment 44•10 years ago
|
||
Comment on attachment 8523505 [details] [diff] [review]
patch 2 - wifi only
Review of attachment 8523505 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/requestsync/RequestSyncService.jsm
@@ +644,5 @@
> + });
> + return;
> + }
> +
> + // Enable all the tasks.
Why are you doing this for all tasks and not just wifi only ones?
::: dom/requestsync/nsIRequestSyncWifiService.idl
@@ +21,5 @@
> +
> +[builtinclass, scriptable, uuid(6db65d27-e0a4-4e22-b3fb-246039cd657f)]
> +interface nsIRequestSyncWifiService : nsISupports
> +{
> + void setCallback(in nsIRequestSyncWifiCallback aCallback);
I'm not sure if it's worth adding these APIs. Can't you just dispatch an observer notification or something? That should help simplify this code.
Attachment #8523505 -
Flags: review?(ehsan.akhgari) → review-
Comment 45•10 years ago
|
||
Comment on attachment 8527160 [details] [diff] [review]
patch 4 - use of promise from sendMessage()
Review of attachment 8527160 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/requestsync/RequestSyncService.jsm
@@ +245,5 @@
> + this._queuedTasks.splice(pos, 1);
> + }
> +
> + // It can be that this object is already in scheduled, or in the queue of a
> + // iDB transacation. In order to avoid it's scheduling, we must disable it.
Nit: rescheduling it.
@@ +548,5 @@
> }
>
> + // We don't want to run more than 1 task at the same time. We do this using
> + // the promise created by sendMessage(). But if the task takes more than
> + // BSYNC_OPERATION_TIMEOUT millisecs, we has to ignore the promise and
Nit: have to
@@ +555,5 @@
> + let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +
> + let done = false;
> + let self = this;
> + function operationCompleted() {
Nit: please give this a different name than the member function operationCompleted.
@@ +595,5 @@
> + dump("ERROR!! RequestSyncService - OperationCompleted called without an active task\n");
> + return;
> + }
> +
> + this._activeTask.data.lastSync = new Date();
I think you should remove this line.
@@ +723,5 @@
> + // It can be that this task has been already schedulated.
> + let pos = self._queuedTasks.indexOf(aObj);
> + if (pos != -1) {
> + self._queuedTasks.splice(pos, 1);
> + }
Perhaps refactor this into a little helper function?
Attachment #8527160 -
Flags: review?(ehsan.akhgari) → review+
Comment 46•10 years ago
|
||
Comment on attachment 8523906 [details] [diff] [review]
patch 6 - setPolicy
Review of attachment 8523906 [details] [diff] [review]:
-----------------------------------------------------------------
(I haven't looked at the patch in detail yet mostly because I disagree with the API design here. The implementation is mostly fine after a cursory look.)
::: dom/webidl/RequestSyncManager.webidl
@@ +35,5 @@
> + Promise<void> setPolicy(ScalarValueString task,
> + ScalarValueString origin,
> + ScalarValueString manifestURL,
> + boolean isInBrowserElement,
> + optional BackgroundTaskPolicy policy);
I'm not a fan of this design. Why won't we make registrations() return a proper interface that can have a setPolicy method that only accepts the state and the min interval?
Also, why is |policy| here optional? Can you please explain the design here a little bit?
Attachment #8523906 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 47•10 years ago
|
||
> > + function() {
> > + dump("ERROR!! RequestSyncService - Failed to retrieve data from the database.\n");
> > + });
>
> Shouldn't we also do things such as rescheduling timers after the
> initialization is done, to keep this working across device restarts, for
> example?
If the device restarts this service restarts as well... right?
> @@ +326,5 @@
> > + this.removeRegistrationInternal(aData.task, key);
> > + }
> > +
> > + // This creates a RequestTaskFull object.
> > + aData.params.task = aData.task;
>
> What guarantees that aData.params is an object at this point?
validateRegistrationParam().
> ::: dom/requestsync/tests/mochitest.ini
> @@ +1,2 @@
> > +[DEFAULT]
> > +skip-if = e10s
>
> What makes this not work with e10s, out of curiosity?
I don't remember. I have to test it again. I'll let you know in a comment.
> ::: dom/requestsync/tests/test_wakeUp.html
> @@ +84,5 @@
> > + }, genericError);
> > + }
> > +
> > + function test_wait() {
> > + // nothing to do here.
>
> So what's the point? ;-)
I need a function that doesn't call runTest. RunTest() will be called when the event is received.
The interdiff will be partial because several comments had been applied already during the workweek.
Such as data check in clearData and NoInterface.
Assignee | ||
Comment 48•10 years ago
|
||
Attachment #8523504 -
Attachment is obsolete: true
Attachment #8537708 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 49•10 years ago
|
||
Attachment #8537709 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 50•10 years ago
|
||
Attachment #8537722 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 51•10 years ago
|
||
Attachment #8523505 -
Attachment is obsolete: true
Attachment #8537724 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 52•10 years ago
|
||
> > + // Enable all the tasks.
>
> Why are you doing this for all tasks and not just wifi only ones?
Because in theory the only ones who can be disabled are the wifionly. I can add a check for this.
Something like:
if (!aObj.data.wifiOnly) {
dump("ERROR - Found a disabled task that is not wifiOnly.");
}
Assignee | ||
Comment 53•10 years ago
|
||
Attachment #8527160 -
Attachment is obsolete: true
Assignee | ||
Comment 55•10 years ago
|
||
> ::: dom/webidl/RequestSyncManager.webidl
> @@ +35,5 @@
> > + Promise<void> setPolicy(ScalarValueString task,
> > + ScalarValueString origin,
> > + ScalarValueString manifestURL,
> > + boolean isInBrowserElement,
> > + optional BackgroundTaskPolicy policy);
>
> I'm not a fan of this design. Why won't we make registrations() return a
> proper interface that can have a setPolicy method that only accepts the
> state and the min interval?
What you suggest is actually a nicer approach.
It's also true that this API is used by the system app only.
> Also, why is |policy| here optional? Can you please explain the design here
> a little bit?
This is webIDL. Dictionaries as params (last params I guess) must be optional.
This is the reason why I have a check in the code to avoid a null policy obj.
Assignee | ||
Comment 56•10 years ago
|
||
> What you suggest is actually a nicer approach.
> It's also true that this API is used by the system app only.
The problem I see is that, in order to return an object, instead of a dictionary, we have to duplicate all the attributes into the object because I cannot reuse the dictionaries as attributes.
Probably having a setPolicy() method is cleaner. Or maybe we can change it to:
Promise<void> setPolicy(USVString taskName,
RequestTaskApp app,
RequestTaskPolicy policy);
Comment 57•10 years ago
|
||
Candice, can you follow up this topic?
Thanks.
feature-b2g: 2.2? → ---
Flags: needinfo?(cserran)
Comment 58•10 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #47)
> > > + function() {
> > > + dump("ERROR!! RequestSyncService - Failed to retrieve data from the database.\n");
> > > + });
> >
> > Shouldn't we also do things such as rescheduling timers after the
> > initialization is done, to keep this working across device restarts, for
> > example?
>
> If the device restarts this service restarts as well... right?
Don't know what I was thinking. You're right!
> > @@ +326,5 @@
> > > + this.removeRegistrationInternal(aData.task, key);
> > > + }
> > > +
> > > + // This creates a RequestTaskFull object.
> > > + aData.params.task = aData.task;
> >
> > What guarantees that aData.params is an object at this point?
>
> validateRegistrationParam().
Please add a comment to that effect.
> > ::: dom/requestsync/tests/mochitest.ini
> > @@ +1,2 @@
> > > +[DEFAULT]
> > > +skip-if = e10s
> >
> > What makes this not work with e10s, out of curiosity?
>
> I don't remember. I have to test it again. I'll let you know in a comment.
Thanks!
> > ::: dom/requestsync/tests/test_wakeUp.html
> > @@ +84,5 @@
> > > + }, genericError);
> > > + }
> > > +
> > > + function test_wait() {
> > > + // nothing to do here.
> >
> > So what's the point? ;-)
>
> I need a function that doesn't call runTest. RunTest() will be called when
> the event is received.
Makes sense. But you should put this in the code comments for the next reader to not get confused. :-)
Comment 59•10 years ago
|
||
Comment on attachment 8537709 [details] [diff] [review]
patch 1 - interdiff
Review of attachment 8537709 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/requestsync/RequestSyncService.jsm
@@ +269,1 @@
> if (!principal) {
Isn't let supposed to be bound to the lexical scope? If yes, I don't see how this can ever work! Please double check and fix if necessary. If this was indeed a bug, you should probably add a test case that would have caught it.
Attachment #8537709 -
Flags: review?(ehsan.akhgari) → review+
Comment 60•10 years ago
|
||
Comment on attachment 8537708 [details] [diff] [review]
patch 1 - basic API
Review of attachment 8537708 [details] [diff] [review]:
-----------------------------------------------------------------
I forgot to say this earlier, but you need to add these new interfaces to test_interfaces.html too.
Attachment #8537708 -
Flags: review?(ehsan.akhgari) → review+
Comment 61•10 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #52)
> > > + // Enable all the tasks.
> >
> > Why are you doing this for all tasks and not just wifi only ones?
>
> Because in theory the only ones who can be disabled are the wifionly. I can
> add a check for this.
> Something like:
>
> if (!aObj.data.wifiOnly) {
> dump("ERROR - Found a disabled task that is not wifiOnly.");
> }
Yes, please do!
Comment 62•10 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #55)
> > ::: dom/webidl/RequestSyncManager.webidl
> > @@ +35,5 @@
> > > + Promise<void> setPolicy(ScalarValueString task,
> > > + ScalarValueString origin,
> > > + ScalarValueString manifestURL,
> > > + boolean isInBrowserElement,
> > > + optional BackgroundTaskPolicy policy);
> >
> > I'm not a fan of this design. Why won't we make registrations() return a
> > proper interface that can have a setPolicy method that only accepts the
> > state and the min interval?
>
> What you suggest is actually a nicer approach.
> It's also true that this API is used by the system app only.
Even just for the system app as a consumer, I think this is a bit too sloppy. I doubt it's too much work to fix this before landing, and I'd like for that to happen. Sorry for pushing on this, but we can do much better! :-)
> > Also, why is |policy| here optional? Can you please explain the design here
> > a little bit?
>
> This is webIDL. Dictionaries as params (last params I guess) must be
> optional.
> This is the reason why I have a check in the code to avoid a null policy obj.
Yes, but I think this is more of a by-product of the design here. My question was more at the conceptual level, as in: "Why does it make sense to have a setPolicy method than takes an optional policy argument?"
Comment 63•10 years ago
|
||
Comment on attachment 8537722 [details] [diff] [review]
patch 2 - interdiff
Review of attachment 8537722 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/requestsync/RequestSyncWifiService.cpp
@@ +49,5 @@
> + nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> + if (obs) {
> + obs->NotifyObservers(nullptr, "wifi-state-changed",
> + mIsWifi ? NS_LITERAL_STRING("enabled").get() :
> + NS_LITERAL_STRING("disabled").get());
It might be a good idea to file a follow-up bug in order to move the NetInfo API to use this under the hood as well!
Also, nit: please use MOZ_UTF16 instead.
::: dom/requestsync/nsIRequestSyncWifiService.idl
@@ -22,2 @@
> [builtinclass, scriptable, uuid(6db65d27-e0a4-4e22-b3fb-246039cd657f)]
> interface nsIRequestSyncWifiService : nsISupports
At this point, this interface is useless. Please remove it, just instantiate the service using nsISupports, get rid of the IDL file and define the macros above in a new .h file.
Attachment #8537722 -
Flags: review?(ehsan.akhgari) → review-
Comment 64•10 years ago
|
||
Comment on attachment 8537724 [details] [diff] [review]
patch 2 - wifi only
I just reviewed the interdiff.
Attachment #8537724 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 65•10 years ago
|
||
I don't know if this is what you meant.
Attachment #8537722 -
Attachment is obsolete: true
Attachment #8537724 -
Attachment is obsolete: true
Attachment #8540119 -
Flags: review?(ehsan.akhgari)
Comment 66•10 years ago
|
||
Comment on attachment 8540119 [details] [diff] [review]
patch 2 - wifi only
Review of attachment 8540119 [details] [diff] [review]:
-----------------------------------------------------------------
Yes, this is exactly what I meant. Thanks!
Attachment #8540119 -
Flags: review?(ehsan.akhgari) → review+
Assignee | ||
Comment 67•10 years ago
|
||
Here we have an object instead a dictionary.
Attachment #8523906 -
Attachment is obsolete: true
Attachment #8540552 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 68•10 years ago
|
||
Attachment #8540552 -
Attachment is obsolete: true
Attachment #8540552 -
Flags: review?(ehsan.akhgari)
Attachment #8540683 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 69•10 years ago
|
||
Comment 70•10 years ago
|
||
This review needs to wait on bug 940273 being reviewed first. I may not get to it this week...
Comment 71•10 years ago
|
||
Comment on attachment 8540683 [details] [diff] [review]
patch 6 - setPolicy
Review of attachment 8540683 [details] [diff] [review]:
-----------------------------------------------------------------
Sorry for the delay!
::: dom/requestsync/RequestSyncService.jsm
@@ +376,4 @@
> this.removeRegistrationInternal(aData.task, key);
> }
>
> // This creates a RequestTaskFull object.
RequestTaskFull doesn't exist any more.
@@ +516,5 @@
> +
> + aObj.data.overwrittenMinInterval = aData.overwrittenMinInterval;
> + aObj.data.state = aData.state;
> +
> + toSave = aObj;
Should we check to make sure that toSave is null here? i.e., that we have only one matching registration?
::: dom/requestsync/RequestSyncTaskApp.jsm
@@ +19,5 @@
> +
> + let keys = [ 'origin', 'manifestURL', 'isInBrowserElement' ];
> + for (let i = 0; i < keys.length; ++i) {
> + if (!(keys[i] in aData)) {
> + dump("ERROR - RequestSyncTaskApp must receive a fully app object: " + keys[i] + " missing.");
Nit: full
@@ +33,5 @@
> + classID: Components.ID('{5a0b64db-a2be-4f08-a6c5-8bf2e3ae0c57}'),
> + contractID: '@mozilla.org/dom/request-sync-manager;1',
> + QueryInterface: XPCOMUtils.generateQI([Ci.nsISupportsWeakReference,
> + Ci.nsIObserver,
> + Ci.nsIDOMGlobalPropertyInitializer]),
None of these interfaces seem to be necessary here.
::: dom/requestsync/RequestSyncTaskManager.jsm
@@ +37,5 @@
> + classID: Components.ID('{a1e1c9c6-ce42-49d4-b8b4-fbd686d8fdd9}'),
> + contractID: '@mozilla.org/dom/request-sync-manager;1',
> + QueryInterface: XPCOMUtils.generateQI([Ci.nsISupportsWeakReference,
> + Ci.nsIObserver,
> + Ci.nsIDOMGlobalPropertyInitializer]),
These are unneeded as well.
@@ +39,5 @@
> + QueryInterface: XPCOMUtils.generateQI([Ci.nsISupportsWeakReference,
> + Ci.nsIObserver,
> + Ci.nsIDOMGlobalPropertyInitializer]),
> +
> + init: function(aManager, aData) {
Who calls this function? I think it should be redundant.
@@ +97,5 @@
> + let self = this;
> + p.then(function() {
> + self._state = aState;
> + self._overwrittenMinInterval = aOverwrittenMinInterval;
> + });
As far as I know, nothing guarantees for this resolve handler to be called before the resolve handler that the user sets on the promise. If that is correct (and please double check), then this code is incorrect, and we need to create a new promise, return that, and in this resolve handler, resolve the newly created promise to ensure that the user's resolve handler will always see the updated state and overwrittenMinInterval properties.
::: dom/webidl/RequestSyncManager.webidl
@@ +7,5 @@
> +[AvailableIn=CertifiedApps,
> + Pref="dom.requestSync.enabled",
> + CheckPermissions="requestsync-manager",
> + JSImplementation="@mozilla.org/dom/request-sync-task-app;1"]
> +interface RequestSyncTaskApp {
Why not just call it RequestSyncApp?
@@ +20,5 @@
> +[AvailableIn=CertifiedApps,
> + Pref="dom.requestSync.enabled",
> + CheckPermissions="requestsync-manager",
> + JSImplementation="@mozilla.org/dom/request-sync-task-manager;1"]
> +interface RequestSyncTaskManager {
I think RequestSyncTask is probbaly a better name for this.
@@ +24,5 @@
> +interface RequestSyncTaskManager {
> + readonly attribute RequestSyncTaskApp app;
> +
> + readonly attribute RequestSyncTaskPolicyState state;
> + readonly attribute long overwrittenMinInterval;
Please add some comments describing all of these fields.
@@ +31,5 @@
> + readonly attribute DOMTimeStamp lastSync;
> + readonly attribute USVString wakeUpPage;
> + readonly attribute boolean oneShot;
> + readonly attribute long minInterval;
> + readonly attribute boolean wifiOnly;
How is this different with |state| being "wifiOnly"? I think this property can be removed.
@@ +35,5 @@
> + readonly attribute boolean wifiOnly;
> + readonly attribute any data;
> +
> + Promise<void> setPolicy(RequestSyncTaskPolicyState aState,
> + long ovewrittenMinInterval);
Don't we want to make the second argument optional? Since I don't think we should expect the callers to always want to provide it.
Attachment #8540683 -
Flags: review?(ehsan) → review-
Assignee | ||
Comment 72•10 years ago
|
||
> @@ +31,5 @@
> > + readonly attribute DOMTimeStamp lastSync;
> > + readonly attribute USVString wakeUpPage;
> > + readonly attribute boolean oneShot;
> > + readonly attribute long minInterval;
> > + readonly attribute boolean wifiOnly;
>
> How is this different with |state| being "wifiOnly"? I think this property
> can be removed.
wifiOnly is what the app sets when the task is created. state is what the setting app overwrites.
I think it's important to have these 2 separate values instead merging them otherwise we don't know if the wifiOnly is wanted by the user of by the app.
For the rest, a new patch is coming.
Assignee | ||
Comment 73•10 years ago
|
||
Attachment #8540683 -
Attachment is obsolete: true
Attachment #8543172 -
Flags: review?(ehsan)
Comment 74•10 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #72)
> > @@ +31,5 @@
> > > + readonly attribute DOMTimeStamp lastSync;
> > > + readonly attribute USVString wakeUpPage;
> > > + readonly attribute boolean oneShot;
> > > + readonly attribute long minInterval;
> > > + readonly attribute boolean wifiOnly;
> >
> > How is this different with |state| being "wifiOnly"? I think this property
> > can be removed.
>
> wifiOnly is what the app sets when the task is created. state is what the
> setting app overwrites.
> I think it's important to have these 2 separate values instead merging them
> otherwise we don't know if the wifiOnly is wanted by the user of by the app.
Makes sense!
Comment 75•10 years ago
|
||
Comment on attachment 8543172 [details] [diff] [review]
patch 6 - setPolicy
Review of attachment 8543172 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/requestsync/RequestSyncService.jsm
@@ +513,5 @@
> + (!app && aData.manifestURL != "")) {
> + return;
> + }
> +
> + if (aData.overwrittenMinInterval) {
This won't detect passing 0 here. You should test |"overwrittenMinInterval" in aData|.
Attachment #8543172 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 76•10 years ago
|
||
let's push it to try before landing it:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=01590634ca3a
Assignee | ||
Comment 77•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5e26604cc6e0
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c6fcdd1c681f
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/edf5737d6e0e
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/46353577ef7a
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8c01c134e40f
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/bce9ed290ddd
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2ef1c26d77d3
Fixed the mochitest issues for b2g. On Monday I'll write an email on dev-b2g about this API.
Comment 78•10 years ago
|
||
You apparently didn't fix the opt M7/debug M15 "test_interfaces.html | uncaught exception - NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref] at chrome://specialpowers/content/specialpowersAPI.js:153" issue, since that's still happening. Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/636498d041b5
Comment 79•10 years ago
|
||
The debug M12 test_info_leak.html "events received in wrong order" is you, too.
Assignee | ||
Comment 80•10 years ago
|
||
Got it. I just pushed a fix to try server:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f00cdac4b782
If green, I'll land this patch.
Assignee | ||
Comment 81•10 years ago
|
||
Assignee | ||
Comment 82•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/012373bb552b
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3e2e4d298e98
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/daf456b0a23a
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2df23c44ef44
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6063a2463d6c
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1aac4d23ccd2
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f60d4ad64070
Comment 83•10 years ago
|
||
(In reply to Phil Ringnalda (:philor) from comment #79)
> The debug M12 test_info_leak.html "events received in wrong order" is you,
> too.
It's still you. Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c3569fd75d4e
Comment 84•10 years ago
|
||
this is a platform bug and they are landing in 2.2
feature-b2g: --- → 2.2+
Flags: needinfo?(cserran)
Assignee | ||
Comment 85•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7778319fce32
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/210391087148
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b729cdfd5a8c
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9706b535ae64
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d23d32851ae8
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/71263e63af0b
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f15ef701bdf2
https://hg.mozilla.org/mozilla-central/rev/7778319fce32
https://hg.mozilla.org/mozilla-central/rev/210391087148
https://hg.mozilla.org/mozilla-central/rev/b729cdfd5a8c
https://hg.mozilla.org/mozilla-central/rev/9706b535ae64
https://hg.mozilla.org/mozilla-central/rev/d23d32851ae8
https://hg.mozilla.org/mozilla-central/rev/71263e63af0b
https://hg.mozilla.org/mozilla-central/rev/f15ef701bdf2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment 87•10 years ago
|
||
With regard to docs, we have triaged this and as it is a stop-gap feature, non-standard and available only to some apps, we will not put a staff writer on it (at least for now).
Nevertheless we will help anybody wanted to document it.
If you want to document, you need to create the two interfaces docs:
https://developer.mozilla.org/en-US/docs/Web/API/RequestSyncManager
https://developer.mozilla.org/en-US/docs/Web/API/RequestSyncScheduler
(and their subpages)
Info about how to do it: https://developer.mozilla.org/en-US/docs/MDN/Contribute/Howto/Write_an_API_reference
Updated•10 years ago
|
Flags: sec-review?(ptheriault)
Comment 88•10 years ago
|
||
This bug is marked as 2.2+ for b2g, but the branch for that release is mozilla-b2g37_v2_2, using gecko 37.
I mistakenly thought this was already on that branch, and email is attempting to use navigator.sync (bug 1098289) in the gaia 2.2 branch (soon to be backed out).
Will this be in the 2.2 release? It is my understanding that this is the last week for feature changes for the 2.2 branches.
ni? :arog only because he was the one that initially contacted me about requestsync use in email.
Flags: needinfo?(arogers)
Comment 89•10 years ago
|
||
As far as I know stage 1 for this should be landing in 37 and should be in the 2.2 branch. It's possible that this has been backed out or undergone a change in plan.
Bahvana, Jonas, can you comment on this uplift?
I have no issues backing out the email/calendar changes if we were unable to land this patch for some reason... but obviously, I'd prefer not.
Flags: needinfo?(jonas)
Flags: needinfo?(bbajaj)
Flags: needinfo?(arogers)
Comment 90•10 years ago
|
||
(In reply to Adam Rogers (:arog) from comment #89)
> As far as I know stage 1 for this should be landing in 37 and should be in
> the 2.2 branch. It's possible that this has been backed out or undergone a
> change in plan.
>
> Bahvana, Jonas, can you comment on this uplift?
>
> I have no issues backing out the email/calendar changes if we were unable to
> land this patch for some reason... but obviously, I'd prefer not.
If :baku/ehsan, can comment on regarding the risk being manageable here, its fine to uplift given this is baked well on m-c. Just fill-in the apprvoal request form asap for b2g37 that already covers this question. Also would be useful to understand if there is any targeted testing needed here once this lands.
Flags: needinfo?(bbajaj) → needinfo?(amarchesini)
Assignee | ||
Comment 91•10 years ago
|
||
Comment on attachment 8537708 [details] [diff] [review]
patch 1 - basic API
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: RequestSync API will not be available for 2.2
Testing completed: tbpl
Risk to taking this patch (and alternatives if risky): This is a new API. I think we want this for 2.2 and I don't see big risks to accept these patches. We have several mochitests covering all features of this API.
String or UUID changes made by this patch: none
Flags: needinfo?(amarchesini)
Attachment #8537708 -
Flags: approval-mozilla-b2g37?
Assignee | ||
Updated•10 years ago
|
Attachment #8527161 -
Flags: approval-mozilla-b2g37?
Assignee | ||
Updated•10 years ago
|
Attachment #8535556 -
Flags: approval-mozilla-b2g37?
Assignee | ||
Updated•10 years ago
|
Attachment #8537735 -
Flags: approval-mozilla-b2g37?
Assignee | ||
Updated•10 years ago
|
Attachment #8537739 -
Flags: approval-mozilla-b2g37?
Assignee | ||
Updated•10 years ago
|
Attachment #8540119 -
Flags: approval-mozilla-b2g37?
Assignee | ||
Comment 92•10 years ago
|
||
Comment on attachment 8543172 [details] [diff] [review]
patch 6 - setPolicy
I guess I didn't need to mark each patch... but now it's too late.
Attachment #8543172 -
Flags: approval-mozilla-b2g37?
Assignee | ||
Comment 93•10 years ago
|
||
In case you decide to uplift the patches, please take what is landed on m-c because I didn't apply all the comments to the attached patches in this bug. Or if you want, I can give a new set of patches.
Comment 94•10 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #93)
> In case you decide to uplift the patches, please take what is landed on m-c
> because I didn't apply all the comments to the attached patches in this bug.
> Or if you want, I can give a new set of patches.
Standard practice is to export what landed on m-c unless there are branch patches attached :)
Updated•10 years ago
|
Attachment #8527161 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Updated•10 years ago
|
Attachment #8535556 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Updated•10 years ago
|
Attachment #8537708 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Updated•10 years ago
|
Attachment #8537735 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Updated•10 years ago
|
Attachment #8537739 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Updated•10 years ago
|
Attachment #8540119 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Updated•10 years ago
|
Attachment #8543172 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Flags: needinfo?(jonas)
Comment 95•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7ea79cc34e61
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/32d904a7d04c
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/0fd5710ce15e
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/dda5de2f1e9f
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/bfc99f8c73b6
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/d283ce0f0996
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/addef534a15b
status-b2g-v2.2:
--- → fixed
status-b2g-master:
--- → fixed
status-firefox36:
--- → wontfix
status-firefox37:
--- → wontfix
status-firefox38:
--- → fixed
Flags: in-testsuite+
Comment 96•10 years ago
|
||
I couldn't find anywhere the doc of setMessageHandlerPromise and it's relationship with the request-sync API. To be honest, the comment in Navigator.webidl only makes me more confused; when does the promise resolve and reject? What is this API intended for?
Please, someone write a doc about it and post it to a mailing list. This even had already been uplifted to v2.2.
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 97•10 years ago
|
||
(In reply to Kan-Ru Chen [:kanru] from comment #96)
> I couldn't find anywhere the doc of setMessageHandlerPromise and it's
> relationship with the request-sync API. To be honest, the comment in
> Navigator.webidl only makes me more confused; when does the promise resolve
> and reject? What is this API intended for?
setMessageHandlerPromise doesn't have a direct relationship with request-sync API but with system messages. As you probably know we use System messages to wake up apps. Request-sync uses sys messages to schedule events for apps.
Now, the app receives the event and it starts doing something. But when does it finish to do this 'something'? We have no information about it, except if the app sets a promise and it resolves/rejects it when the operation is completed. Setting this promise is fully optional.
This idea comes from Sicking. Actually he wanted to kill the app after dispatching the event if the promise was not set. Sicking do you think it's time to do it? Do you still want to do this?
Flags: needinfo?(amarchesini) → needinfo?(jonas)
Comment 98•10 years ago
|
||
A use case from gaia email that might help inform decisions:
When email gets the system message to wake up because of a sync, email will close the app via window.close() if it gets to the end of the sync process and was not made visible to the user. However, if the app became visible to the user while the sync was happening, email does not close it in case the user is in the middle of something like an activity close, or just composing an email.
It is unclear to me how this would be communicated via setting a promise resolution value. I suppose a set of messages could be allowed for the promise resolution that indicate "all done, but don't close me" vs "all done can close".
Maybe this changes once service workers (SW) are the entry point for background sync. For email, I am hoping we can do this sort of flow with SW:
* email's SW is awoken to receive the sync event. email uses the event.waitUntil() to set a promise that keeps the SW alive while email uses postMessage to tell the email shared worker to do a sync
* The SW waits for a postMessage result from the shared worker to know syncing is complete, then the SW resolves the promise.
* The system can then shut down the SW, and if no document windows were open for the email app, it would mean nothing else referencing the shared worker and it can be shut down.
* However, if there is an email document window opened for user interaction, the shared worker stays up, just the SW is shut down.
That is not quite possible yet for SW (SW currently is not specified to be something that can keep a shared worker alive), but hopefully that changes, this issue talks about that:
https://github.com/slightlyoff/ServiceWorker/issues/678
> This idea comes from Sicking. Actually he wanted to kill the app after
> dispatching the event if the promise was not set. Sicking do you think it's
> time to do it? Do you still want to do this?
Yes, I think that would be nice, but not critical. We should move to use Service Workers for this API in a not too distant future anyway, so maybe it's better to focus our efforts there.
Flags: needinfo?(jonas)
Removing sec-review flag
Flags: sec-review?(ptheriault)
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•