Closed Bug 1018320 Opened 10 years ago Closed 9 years ago

Implement RequestSync API for FirefoxOS

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
feature-b2g 2.2+
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: jedp, Assigned: baku)

References

Details

(Keywords: dev-doc-needed)

Attachments

(8 files, 30 obsolete files)

11.07 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
19.36 KB, patch
Details | Diff | Splinter Review
60.66 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
4.23 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
9.49 KB, patch
Details | Diff | Splinter Review
18.92 KB, patch
Details | Diff | Splinter Review
11.83 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
24.57 KB, patch
ehsan.akhgari
: review+
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
I think this warrants an intent to implement thread...  Thanks!
Thanks, Ehsan.  I'll begin a thread on this.
Attached patch 1018320-sync-scheduler.patch (obsolete) — — Splinter Review
Initial sketch (wip - doesn't actually work :)
Attached patch SyncScheduler (wip) (obsolete) — — Splinter Review
Attachment #8431786 - Attachment is obsolete: true
Attached patch SyncScheduler (wip) (obsolete) — — Splinter Review
Attachment #8431852 - Attachment is obsolete: true
Attached patch SyncScheduler (wip) (obsolete) — — Splinter Review
Attachment #8431878 - Attachment is obsolete: true
Carlos and I are working on this together in his gecko-dev git repo:
 https://github.com/carlosdp/gecko-dev/tree/request-sync
Status: NEW → ASSIGNED
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
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
Summary: Implement navigator.requestSync and .unregisterSync → Implement BackgroundSync API for FirefoxOS
Current working branch is now Carlos's sync-db, not request-sync
(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.
Attached patch SyncScheduler (WIP) (obsolete) — — Splinter Review
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)
Component: FxA → DOM
Product: Firefox OS → Core
Assignee: cpadron → amarchesini
Attached patch WIP (obsolete) — — Splinter Review
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)
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)
Attached patch patch 1 - basic API (obsolete) — — Splinter Review
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)
Attached patch patch 2 - wifi only (obsolete) — — Splinter Review
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)
Attached patch patch 3 - promise in sendMessage() (obsolete) — — Splinter Review
Attachment #8519012 - Flags: review?(jonas)
Blocks: 1098289
Blocks: 1098292
Attached patch patch 1 - basic API (obsolete) — — Splinter Review
BackgroundSyncAPI => RequestSyncAPI
Attachment #8517558 - Attachment is obsolete: true
Attachment #8517558 - Flags: review?(jonas)
Attachment #8522286 - Flags: review?(jonas)
Attached patch patch 2 - wifi only (obsolete) — — Splinter Review
Attachment #8518206 - Attachment is obsolete: true
Attachment #8518206 - Flags: review?(jonas)
Attachment #8522288 - Flags: review?(jonas)
Summary: Implement BackgroundSync API for FirefoxOS → Implement RequestSync API for FirefoxOS
Attached patch patch 1 - basic API (obsolete) — — Splinter Review
Attachment #8522286 - Attachment is obsolete: true
Attachment #8522286 - Flags: review?(jonas)
Attachment #8522325 - Flags: review?(jonas)
Attached patch patch 2 - wifi only (obsolete) — — Splinter Review
Attachment #8522288 - Attachment is obsolete: true
Attachment #8522288 - Flags: review?(jonas)
Attachment #8522326 - Flags: review?(jonas)
Attached patch patch 4 - use of promise from sendMessage() (obsolete) — — Splinter Review
Attachment #8522329 - Flags: review?(jonas)
Attached patch patch 1 - basic API (obsolete) — — Splinter Review
no dynamic message name.
Attachment #8522325 - Attachment is obsolete: true
Attachment #8522325 - Flags: review?(jonas)
Attachment #8523504 - Flags: review?(jonas)
Attached patch patch 2 - wifi only (obsolete) — — Splinter Review
rebased
Attachment #8522326 - Attachment is obsolete: true
Attachment #8522326 - Flags: review?(jonas)
Attachment #8523505 - Flags: review?(jonas)
Attached patch patch 3 - promise in sendMessage() (obsolete) — — Splinter Review
rebased
Attachment #8519012 - Attachment is obsolete: true
Attachment #8519012 - Flags: review?(jonas)
Attachment #8523506 - Flags: review?(jonas)
Attached patch patch 4 - use of promise from sendMessage() (obsolete) — — Splinter Review
Attachment #8522329 - Attachment is obsolete: true
Attachment #8522329 - Flags: review?(jonas)
Attachment #8523507 - Flags: review?(jonas)
Attached patch patch 5 - mozSetMessageHandlerPromise (obsolete) — — Splinter Review
This is the last patch for this API.
Attachment #8523509 - Flags: review?(jonas)
Attached patch patch 6 - setPolicy (obsolete) — — Splinter Review
Actually I forgot the 'setPolicy' method in the RequestSyncManager. Here the patch.
Attachment #8523906 - Flags: review?(jonas)
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)
Here what Jonas suggested.
Attachment #8526698 - Flags: review?(fabrice)
Attached patch patch 4 - use of promise (obsolete) — — Splinter Review
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)
Attachment #8526709 - Attachment description: use.patch → patch 4 - use of promise
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+
Attached patch patch 3 - promise in sendMessage() (obsolete) — — Splinter Review
Attachment #8523506 - Attachment is obsolete: true
Attachment #8527159 - Flags: review?(fabrice)
Attached patch patch 4 - use of promise from sendMessage() (obsolete) — — Splinter Review
Attachment #8526709 - Attachment is obsolete: true
Attachment #8526709 - Flags: review?(jonas)
Attachment #8527160 - Flags: review?(jonas)
Attachment #8526698 - Attachment is obsolete: true
Attachment #8526698 - Flags: review?(fabrice)
Attachment #8527161 - Flags: review?(fabrice)
QA Whiteboard: [2.2-feature-qa+]
Ehsan, do you mind to help Jonas to review this API?
Flags: needinfo?(ehsan.akhgari)
Attachment #8523504 - Flags: review?(jonas) → review?(ehsan.akhgari)
Attachment #8523505 - Flags: review?(jonas) → review?(ehsan.akhgari)
Attachment #8523906 - Flags: review?(jonas) → review?(ehsan.akhgari)
Attachment #8527160 - Flags: review?(jonas) → review?(ehsan.akhgari)
Flags: needinfo?(ehsan.akhgari)
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 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+
Attachment #8527161 - Flags: review?(fabrice) → review+
Attachment #8527159 - Attachment is obsolete: true
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 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 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 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)
> > +    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.
Attached patch patch 1 - basic API — — Splinter Review
Attachment #8523504 - Attachment is obsolete: true
Attachment #8537708 - Flags: review?(ehsan.akhgari)
Attached patch patch 1 - interdiff — — Splinter Review
Attachment #8537709 - Flags: review?(ehsan.akhgari)
Attached patch patch 2 - interdiff (obsolete) — — Splinter Review
Attachment #8537722 - Flags: review?(ehsan.akhgari)
Attached patch patch 2 - wifi only (obsolete) — — Splinter Review
Attachment #8523505 - Attachment is obsolete: true
Attachment #8537724 - Flags: review?(ehsan.akhgari)
> > +    // 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.");
}
Attachment #8527160 - Attachment is obsolete: true
rebased
Attachment #8523509 - Attachment is obsolete: true
> ::: 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.
> 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);
Candice, can you follow up this topic? 
Thanks.
feature-b2g: 2.2? → ---
Flags: needinfo?(cserran)
(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 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 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+
(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!
(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 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 on attachment 8537724 [details] [diff] [review]
patch 2 - wifi only

I just reviewed the interdiff.
Attachment #8537724 - Flags: review?(ehsan.akhgari)
Attached patch patch 2 - wifi only — — Splinter Review
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 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+
Attached patch patch 6 - setPolicy (obsolete) — — Splinter Review
Here we have an object instead a dictionary.
Attachment #8523906 - Attachment is obsolete: true
Attachment #8540552 - Flags: review?(ehsan.akhgari)
Attached patch patch 6 - setPolicy (obsolete) — — Splinter Review
Attachment #8540552 - Attachment is obsolete: true
Attachment #8540552 - Flags: review?(ehsan.akhgari)
Attachment #8540683 - Flags: review?(ehsan.akhgari)
This review needs to wait on bug 940273 being reviewed first.  I may not get to it this week...
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-
> @@ +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.
Attached patch patch 6 - setPolicy — — Splinter Review
Attachment #8540683 - Attachment is obsolete: true
Attachment #8543172 - Flags: review?(ehsan)
(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 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+
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
The debug M12 test_info_leak.html "events received in wrong order" is you, too.
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.
(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
this is a platform bug and they are landing in 2.2
feature-b2g: --- → 2.2+
Flags: needinfo?(cserran)
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
Blocks: 1128065
Flags: sec-review?(ptheriault)
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)
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)
(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)
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?
Attachment #8527161 - Flags: approval-mozilla-b2g37?
Attachment #8535556 - Flags: approval-mozilla-b2g37?
Attachment #8537735 - Flags: approval-mozilla-b2g37?
Attachment #8537739 - Flags: approval-mozilla-b2g37?
Attachment #8540119 - Flags: approval-mozilla-b2g37?
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?
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.
(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 :)
Attachment #8527161 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8535556 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8537708 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8537735 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8537739 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8540119 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8543172 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
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)
(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)
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)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.