Add IDL and initial Blink API for Background Sync

Add IDL and initial Blink API for Background Sync

This patch adds the interfaces for background sync (https://github.com/slightlyoff/BackgroundSync/blob/master/explainer.md) to the IDL and exposes syncManager on the ServiceWorkerRegistration interface.

It includes just enough of the implementation to be able to instantiate SyncRegistration objects and pass them through to the browser code (not included in this patch, except for the header definitions in public/platform/

BUG=449443
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192279

[iclelland, 2015-02-27 15:51:30]

iclelland@chromium.org changed reviewers:
+ jkarlin@chromium.org

[jkarlin, 2015-02-27 20:08:14]

So exciting!

https://codereview.chromium.org/963683002/diff/20001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/963683002/diff/20001/public/platform/Platform...
public/platform/Platform.h:658: // navigator.connect
--------------------------------------------------
Double vertical space here seems to be the norm. Add the deleted line back.

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/OWNERS (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/OWNERS:4: 
remove empty line

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/ServiceWorkerRegistrationSync.cpp (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/ServiceWorkerRegistrationSync.cpp:49: void
ServiceWorkerRegistrationSync::trace(Visitor* visitor)
DEFINE_TRACE(ServiceWorkerRegistrationSync)

see ServiceWorkerRegistrationPush.h/cpp

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/ServiceWorkerRegistrationSync.h (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/ServiceWorkerRegistrationSync.h:26: virtual void
trace(Visitor*) override;
DECLARE_VIRTUAL_TRACE(); instead

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/SyncError.cpp (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncError.cpp:20: return
DOMException::create(QuotaExceededError, webError->message);
Eventually we'll be able to use PermissionDeniedError, but since that doesn't
exist...

QuotaExceededError isn't quite right, the string for that is "An attempt was
made to add something to storage that exceeded the quota."

How about InvalidAccessError? It's string is: "A parameter or an operation was
not supported by the underlying object."

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/SyncError.h (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncError.h:20: typedef WebSyncError WebType;
Should now be: using WebType = WebSyncError;

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncError.h:21: static
PassRefPtrWillBeRawPtr<DOMException> take(ScriptPromiseResolver*, WebType*
webErrorRaw);
Remove parameter name webErrorRaw for blink style

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncError.h:22: static void dispose(WebType*
webErrorRaw);
remove webErrorRaw

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/SyncEvent.cpp (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncEvent.cpp:40: void SyncEvent::trace(Visitor*
visitor)
DEFINE_TRACE(PushEvent)

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/SyncEvent.h (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncEvent.h:40: virtual void trace(Visitor*)
override;
DECLARE_VIRTUAL_TRACE();

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/SyncManager.cpp (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.cpp:27: WebSyncProvider*
backgroundSyncProvider()
contents of nested namespaces in blink are indented 4 spaces

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.cpp:34: } // namespace
don't need the '// namespace' as this is such a short namespace

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.cpp:38: return m_minAllowablePeriod;
return a constant defined in the unnamed namespace

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.cpp:41: ScriptPromise
SyncManager::registerFunction(blink::ScriptState* scriptState)
I don't think you need this method

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.cpp:56:
backgroundSyncProvider()->registerBackgroundSync(0, new
SyncRegistrationCallbacks(resolver, m_registration));
Please see PushManager::subscribe. They have a check for a detached window.
What's that about? Also, why do they have the separate PushController for
registering from the document vs serviceworker? Do we need to do the same here?

Ah, I see in the idl that this is Window only. I think we're going to wind up
adding ServiceWorker as well here.

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.cpp:67:
resolver->reject(DOMException::create(InvalidStateError, "Not implemented"));
Use NotSupportedError

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.cpp:72: void
SyncManager::trace(Visitor *visitor)
DEFINE_TRACE(SyncManager)

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/SyncManager.h (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.h:37: void trace(Visitor*);
DECLARE_TRACE();

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.h:40: explicit
SyncManager(ServiceWorkerRegistration* registration) :
m_registration(registration) { }
Define this in the CPP file and ASSERT(registration) as PushManager does

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.h:42: unsigned long
m_minAllowablePeriod;
Remove this and use a constant

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/SyncManager.idl (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.idl:6: Exposed=(Window),
Add ServiceWorker as well, or a FIXME to do so

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.idl:11:
[CallWith=ScriptState,ImplementedAs=registerFunction] Promise<SyncRegistration>
register(SyncRegistrationOptions options);
Why registerFunction instead of register?

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.idl:11:
[CallWith=ScriptState,ImplementedAs=registerFunction] Promise<SyncRegistration>
register(SyncRegistrationOptions options);
I believe you need to add 'optional' keyword before SyncRegistrationOptions

http://heycam.github.io/webidl/#dfn-optional-argument-default-value

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/SyncRegistration.h (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncRegistration.h:33: void trace(Visitor*);
DECLARE_TRACE();

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/SyncRegistrationCallbacks.h (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncRegistrationCallbacks.h:4: 
This filename seems a bit off. Perhaps SyncCallbacks?

https://codereview.chromium.org/963683002/diff/40001/public/platform/Platform.h
File public/platform/Platform.h (left):

https://codereview.chromium.org/963683002/diff/40001/public/platform/Platform...
public/platform/Platform.h:657: 
Add the line back, as is this file's strange custom

https://codereview.chromium.org/963683002/diff/40001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/963683002/diff/40001/public/platform/Platform...
public/platform/Platform.h:63: class WebSyncProvider;
not alphabetical. Part of me wonders if this should be
WebBackgroundSyncProvider.

https://codereview.chromium.org/963683002/diff/40001/public/platform/modules/...
File public/platform/modules/background_sync/WebSyncProvider.h (right):

https://codereview.chromium.org/963683002/diff/40001/public/platform/modules/...
public/platform/modules/background_sync/WebSyncProvider.h:27: virtual void
registerBackgroundSync(const WebSyncRegistrationOptions*,
WebSyncRegistrationCallbacks*) = 0;
Who owns the WebSyncRegistrationOptions?

[jkarlin, 2015-02-27 20:09:16]

jkarlin@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org

[jkarlin, 2015-02-27 20:09:49]

+r mvanouwerkerk@ as this is so heavily Push influenced.

[iclelland, 2015-03-02 03:51:12]

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/OWNERS (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/OWNERS:4: 
On 2015/02/27 20:08:12, jkarlin wrote:
> remove empty line

Done.

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/ServiceWorkerRegistrationSync.cpp (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/ServiceWorkerRegistrationSync.cpp:49: void
ServiceWorkerRegistrationSync::trace(Visitor* visitor)
On 2015/02/27 20:08:12, jkarlin wrote:
> DEFINE_TRACE(ServiceWorkerRegistrationSync)
> 
> see ServiceWorkerRegistrationPush.h/cpp

Done.

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/ServiceWorkerRegistrationSync.h (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/ServiceWorkerRegistrationSync.h:26: virtual void
trace(Visitor*) override;
On 2015/02/27 20:08:12, jkarlin wrote:
> DECLARE_VIRTUAL_TRACE(); instead

Done.

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/SyncError.cpp (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncError.cpp:20: return
DOMException::create(QuotaExceededError, webError->message);
On 2015/02/27 20:08:12, jkarlin wrote:
> Eventually we'll be able to use PermissionDeniedError, but since that doesn't
> exist...
> 
> QuotaExceededError isn't quite right, the string for that is "An attempt was
> made to add something to storage that exceeded the quota."
> 
> How about InvalidAccessError? It's string is: "A parameter or an operation was
> not supported by the underlying object."

Done.

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/SyncError.h (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncError.h:20: typedef WebSyncError WebType;
On 2015/02/27 20:08:12, jkarlin wrote:
> Should now be: using WebType = WebSyncError;

Done.

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncError.h:21: static
PassRefPtrWillBeRawPtr<DOMException> take(ScriptPromiseResolver*, WebType*
webErrorRaw);
On 2015/02/27 20:08:12, jkarlin wrote:
> Remove parameter name webErrorRaw for blink style

Done.

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncError.h:22: static void dispose(WebType*
webErrorRaw);
On 2015/02/27 20:08:12, jkarlin wrote:
> remove webErrorRaw

Done.

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/SyncEvent.cpp (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncEvent.cpp:40: void SyncEvent::trace(Visitor*
visitor)
On 2015/02/27 20:08:12, jkarlin wrote:
> DEFINE_TRACE(PushEvent)

Done. (As SyncEvent ;) )

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/SyncEvent.h (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncEvent.h:40: virtual void trace(Visitor*)
override;
On 2015/02/27 20:08:12, jkarlin wrote:
> DECLARE_VIRTUAL_TRACE();

Done.

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/SyncManager.cpp (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.cpp:27: WebSyncProvider*
backgroundSyncProvider()
On 2015/02/27 20:08:13, jkarlin wrote:
> contents of nested namespaces in blink are indented 4 spaces

Done.

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.cpp:34: } // namespace
On 2015/02/27 20:08:12, jkarlin wrote:
> don't need the '// namespace' as this is such a short namespace

Done.

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.cpp:38: return m_minAllowablePeriod;
On 2015/02/27 20:08:13, jkarlin wrote:
> return a constant defined in the unnamed namespace

Done.

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.cpp:41: ScriptPromise
SyncManager::registerFunction(blink::ScriptState* scriptState)
On 2015/02/27 20:08:12, jkarlin wrote:
> I don't think you need this method

No, actually, I don't think we do. I was told earlier that calling .register()
-- with no options at all -- was distinct from .register(null) or
.register(options), and in the first case, this method would be called instead.

Tracing through execution, though, I've never been able to trigger it.

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.cpp:56:
backgroundSyncProvider()->registerBackgroundSync(0, new
SyncRegistrationCallbacks(resolver, m_registration));
On 2015/02/27 20:08:12, jkarlin wrote:
> Please see PushManager::subscribe. They have a check for a detached window.
> What's that about? Also, why do they have the separate PushController for
> registering from the document vs serviceworker? Do we need to do the same
here?
> 
> Ah, I see in the idl that this is Window only. I think we're going to wind up
> adding ServiceWorker as well here. 

Right -- I thought that one of the concerns was that a sync event can't trigger
the registration of another event, and that was why we removed register's
visibility from the service worker.

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.cpp:67:
resolver->reject(DOMException::create(InvalidStateError, "Not implemented"));
On 2015/02/27 20:08:13, jkarlin wrote:
> Use NotSupportedError

Sure -- this isn't intended to survive by the time it gets committed, but I'll
change it anyway, just in case.

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.cpp:72: void
SyncManager::trace(Visitor *visitor)
On 2015/02/27 20:08:13, jkarlin wrote:
> DEFINE_TRACE(SyncManager)

Done.

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/SyncManager.h (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.h:37: void trace(Visitor*);
On 2015/02/27 20:08:13, jkarlin wrote:
> DECLARE_TRACE();

Done.

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.h:40: explicit
SyncManager(ServiceWorkerRegistration* registration) :
m_registration(registration) { }
On 2015/02/27 20:08:13, jkarlin wrote:
> Define this in the CPP file and ASSERT(registration) as PushManager does

Done.

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.h:42: unsigned long
m_minAllowablePeriod;
On 2015/02/27 20:08:13, jkarlin wrote:
> Remove this and use a constant

Done.

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/SyncManager.idl (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.idl:6: Exposed=(Window),
On 2015/02/27 20:08:13, jkarlin wrote:
> Add ServiceWorker as well, or a FIXME to do so

Done. Added FIXME until I add the additional IPC for the SW side

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.idl:11:
[CallWith=ScriptState,ImplementedAs=registerFunction] Promise<SyncRegistration>
register(SyncRegistrationOptions options);
On 2015/02/27 20:08:13, jkarlin wrote:
> Why registerFunction instead of register?

register's a c++ reserved keyword

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/SyncRegistration.h (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncRegistration.h:33: void trace(Visitor*);
On 2015/02/27 20:08:13, jkarlin wrote:
> DECLARE_TRACE();

Done.

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
File Source/modules/background_sync/SyncRegistrationCallbacks.h (right):

https://codereview.chromium.org/963683002/diff/40001/Source/modules/backgroun...
Source/modules/background_sync/SyncRegistrationCallbacks.h:4: 
On 2015/02/27 20:08:13, jkarlin wrote:
> This filename seems a bit off. Perhaps SyncCallbacks?

Agreed -- it held only the registration callbacks until very recently. Changed.

https://codereview.chromium.org/963683002/diff/40001/public/platform/Platform.h
File public/platform/Platform.h (left):

https://codereview.chromium.org/963683002/diff/40001/public/platform/Platform...
public/platform/Platform.h:657: 
On 2015/02/27 20:08:13, jkarlin wrote:
> Add the line back, as is this file's strange custom

Done.

https://codereview.chromium.org/963683002/diff/40001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/963683002/diff/40001/public/platform/Platform...
public/platform/Platform.h:63: class WebSyncProvider;
On 2015/02/27 20:08:13, jkarlin wrote:
> not alphabetical. Part of me wonders if this should be
> WebBackgroundSyncProvider.

It's sort of alphabetical now (and yeah, I had it named
WebBackgroundSyncProvider originally, thus the out-of-place declaration.)

https://codereview.chromium.org/963683002/diff/40001/public/platform/modules/...
File public/platform/modules/background_sync/WebSyncProvider.h (right):

https://codereview.chromium.org/963683002/diff/40001/public/platform/modules/...
public/platform/modules/background_sync/WebSyncProvider.h:27: virtual void
registerBackgroundSync(const WebSyncRegistrationOptions*,
WebSyncRegistrationCallbacks*) = 0;
On 2015/02/27 20:08:13, jkarlin wrote:
> Who owns the WebSyncRegistrationOptions?

I believe the caller will retain ownership of that structure -- does that need
to be spelled out here?

[Michael van Ouwerkerk, 2015-03-02 14:19:11]

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
File Source/modules/background_sync/OWNERS (right):

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
Source/modules/background_sync/OWNERS:3: asanka@chrmoium.org
nit: alphabetic order

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
File Source/modules/background_sync/ServiceWorkerGlobalScopeSync.idl (right):

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
Source/modules/background_sync/ServiceWorkerGlobalScopeSync.idl:5: [
nit: add a link to the editor's draft section that defines this idl. Same for
all the other idl files.

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
File Source/modules/background_sync/SyncCallbacks.h (right):

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
Source/modules/background_sync/SyncCallbacks.h:24: // will pass it to the
SyncProvider.
s/SyncProvider/SyncRegistration/

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
Source/modules/background_sync/SyncCallbacks.h:55:
Persistent<ServiceWorkerRegistration> m_serviceWorkerRegistration;
This is never used.

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
File Source/modules/background_sync/SyncEvent.h (right):

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
Source/modules/background_sync/SyncEvent.h:25: static
PassRefPtrWillBeRawPtr<SyncEvent> create(const AtomicString& type,
SyncRegistration* registration, WaitUntilObserver* observer)
nit: since related code also deals with ServiceWorkerRegistration, it would help
readability to always explicitly name SyncRegistration arguments and members as
syncRegistration and m_syncRegistration.

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
File Source/modules/background_sync/SyncManager.cpp (right):

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.cpp:54:
backgroundSyncProvider()->registerBackgroundSync(0, new
SyncRegistrationCallbacks(resolver, m_registration));
The Push API has a separate path for this function that distinguishes between
document context and service worker context. Why does sync not need this?

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.cpp:54:
backgroundSyncProvider()->registerBackgroundSync(0, new
SyncRegistrationCallbacks(resolver, m_registration));
What is the magic 0 for?

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
File Source/modules/background_sync/SyncManager.h (right):

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.h:14: class ExecutionContext;
This is not used.

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.h:16: class ScriptPromiseResolver;
This is not used.

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.h:37: void trace(Visitor*);
Use the macro.

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
File Source/modules/background_sync/SyncManager.idl (right):

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.idl:6: //FIXME: Expose on
ServiceWorker as well
Why is this not exposed on ServiceWorker yet?

https://codereview.chromium.org/963683002/diff/60001/public/platform/modules/...
File public/platform/modules/background_sync/WebSyncProvider.h (right):

https://codereview.chromium.org/963683002/diff/60001/public/platform/modules/...
public/platform/modules/background_sync/WebSyncProvider.h:29: // Takes ownership
of the WebSyncRegistrationCallbacks.
WebSyncUnregistrationCallbacks

[Michael van Ouwerkerk, 2015-03-02 14:19:59]

What is your test plan?

[jkarlin, 2015-03-03 16:12:51]

On 2015/03/02 14:19:59, Michael van Ouwerkerk wrote:
> What is your test plan?

Same as push. http tests for basic idl, permission, and register/unregister
tests. The actual firing of the onsync event isn't really testable in http
tests.

[Michael van Ouwerkerk, 2015-03-03 16:16:47]

On 2015/03/03 16:12:51, jkarlin wrote:
> On 2015/03/02 14:19:59, Michael van Ouwerkerk wrote:
> > What is your test plan?
> 
> Same as push. http tests for basic idl, permission, and register/unregister
> tests. The actual firing of the onsync event isn't really testable in http
> tests.

Sounds good to me. So these will come later, once there is enough functionality
to exercise? I think some, like the basic interface inspection tests, can
probably already be written.

[iclelland, 2015-03-03 16:21:37]

On 2015/03/03 16:16:47, Michael van Ouwerkerk wrote:
> On 2015/03/03 16:12:51, jkarlin wrote:
> > On 2015/03/02 14:19:59, Michael van Ouwerkerk wrote:
> > > What is your test plan?
> > 
> > Same as push. http tests for basic idl, permission, and register/unregister
> > tests. The actual firing of the onsync event isn't really testable in http
> > tests.
> 
> Sounds good to me. So these will come later, once there is enough
functionality
> to exercise? 

>I think some, like the basic interface inspection tests, can
> probably already be written.

Yes, pending some updates to the IDL that I am just integrating now.

[jkarlin, 2015-03-03 16:22:55]

On 2015/03/03 16:16:47, Michael van Ouwerkerk wrote:
> On 2015/03/03 16:12:51, jkarlin wrote:
> > On 2015/03/02 14:19:59, Michael van Ouwerkerk wrote:
> > > What is your test plan?
> > 
> > Same as push. http tests for basic idl, permission, and register/unregister
> > tests. The actual firing of the onsync event isn't really testable in http
> > tests.
> 
> Sounds good to me. So these will come later, once there is enough
functionality
> to exercise? I think some, like the basic interface inspection tests, can
> probably already be written.

Yep, they'll come later. True, some interface tests would be good now.

[iclelland, 2015-03-05 04:32:49]

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
File Source/modules/background_sync/OWNERS (right):

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
Source/modules/background_sync/OWNERS:3: asanka@chrmoium.org
On 2015/03/02 14:19:11, Michael van Ouwerkerk wrote:
> nit: alphabetic order

Done.

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
File Source/modules/background_sync/SyncCallbacks.h (right):

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
Source/modules/background_sync/SyncCallbacks.h:24: // will pass it to the
SyncProvider.
On 2015/03/02 14:19:11, Michael van Ouwerkerk wrote:
> s/SyncProvider/SyncRegistration/

Done.

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
File Source/modules/background_sync/SyncEvent.h (right):

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
Source/modules/background_sync/SyncEvent.h:25: static
PassRefPtrWillBeRawPtr<SyncEvent> create(const AtomicString& type,
SyncRegistration* registration, WaitUntilObserver* observer)
On 2015/03/02 14:19:11, Michael van Ouwerkerk wrote:
> nit: since related code also deals with ServiceWorkerRegistration, it would
help
> readability to always explicitly name SyncRegistration arguments and members
as
> syncRegistration and m_syncRegistration.

Done.

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
File Source/modules/background_sync/SyncManager.cpp (right):

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.cpp:54:
backgroundSyncProvider()->registerBackgroundSync(0, new
SyncRegistrationCallbacks(resolver, m_registration));
On 2015/03/02 14:19:11, Michael van Ouwerkerk wrote:
> The Push API has a separate path for this function that distinguishes between
> document context and service worker context. Why does sync not need this?

Register wasn't going to be available in the service worker context originally
(only from document, to prevent one-shot syncs from re-registering themselves
into periodic syncs.); this is changing, and so I'm adding that code path.

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.cpp:54:
backgroundSyncProvider()->registerBackgroundSync(0, new
SyncRegistrationCallbacks(resolver, m_registration));
On 2015/03/02 14:19:11, Michael van Ouwerkerk wrote:
> What is the magic 0 for?

For temporary. I've added the code now that creates the correct
WebSyncRegistration object to call the provider.

Done.

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
File Source/modules/background_sync/SyncManager.h (right):

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.h:14: class ExecutionContext;
On 2015/03/02 14:19:11, Michael van Ouwerkerk wrote:
> This is not used.

Removed.
Done.

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.h:16: class ScriptPromiseResolver;
On 2015/03/02 14:19:11, Michael van Ouwerkerk wrote:
> This is not used.

Removed.
Done.

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.h:37: void trace(Visitor*);
On 2015/03/02 14:19:11, Michael van Ouwerkerk wrote:
> Use the macro.

Done.

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
File Source/modules/background_sync/SyncManager.idl (right):

https://codereview.chromium.org/963683002/diff/60001/Source/modules/backgroun...
Source/modules/background_sync/SyncManager.idl:6: //FIXME: Expose on
ServiceWorker as well
On 2015/03/02 14:19:11, Michael van Ouwerkerk wrote:
> Why is this not exposed on ServiceWorker yet?

I just wasn't sure if there were complications around making some of the methods
window-only, so for early development I left it off of the service worker global
scope.. That is no longer a requirement, so I've fixed this.

Done.

https://codereview.chromium.org/963683002/diff/60001/public/platform/modules/...
File public/platform/modules/background_sync/WebSyncProvider.h (right):

https://codereview.chromium.org/963683002/diff/60001/public/platform/modules/...
public/platform/modules/background_sync/WebSyncProvider.h:29: // Takes ownership
of the WebSyncRegistrationCallbacks.
On 2015/03/02 14:19:11, Michael van Ouwerkerk wrote:
> WebSyncUnregistrationCallbacks

Done.

[Michael van Ouwerkerk, 2015-03-10 16:58:59]

lgtm (apologies for the delay)

[iclelland, 2015-03-10 18:49:12]

On 2015/03/10 16:58:59, Michael van Ouwerkerk wrote:
> lgtm (apologies for the delay)

Thanks, Michael -- and I'm sorry I didn't see this lgtm; so I pushed one more
patch up -- it includes the scaffolding for getRegistration and
getRegistrations. Can I bother you for one more quick review?

[iclelland, 2015-03-10 19:06:19]

On 2015/03/10 18:49:12, iclelland wrote:
> On 2015/03/10 16:58:59, Michael van Ouwerkerk wrote:
> > lgtm (apologies for the delay)
> 
> Thanks, Michael -- and I'm sorry I didn't see this lgtm; so I pushed one more
> patch up -- it includes the scaffolding for getRegistration and
> getRegistrations. Can I bother you for one more quick review?

Also, I'm not sure who should be OWNER on
public/platform/modules/background_sync -- any opinions?

[Michael van Ouwerkerk, 2015-03-10 19:29:11]

still looks good to me

https://codereview.chromium.org/963683002/diff/240001/Source/modules/backgrou...
File Source/modules/background_sync/SyncManager.cpp (right):

https://codereview.chromium.org/963683002/diff/240001/Source/modules/backgrou...
Source/modules/background_sync/SyncManager.cpp:88: ScriptPromise
SyncManager::getRegistrations(blink::ScriptState* scriptState)
here also

https://codereview.chromium.org/963683002/diff/240001/Source/modules/backgrou...
File Source/modules/background_sync/SyncManager.h (right):

https://codereview.chromium.org/963683002/diff/240001/Source/modules/backgrou...
Source/modules/background_sync/SyncManager.h:34: ScriptPromise
getRegistration(blink::ScriptState*, const String&);
micro nit: omit blink::

[Michael van Ouwerkerk, 2015-03-10 19:30:16]

On 2015/03/10 19:06:19, iclelland wrote:
> On 2015/03/10 18:49:12, iclelland wrote:
> > On 2015/03/10 16:58:59, Michael van Ouwerkerk wrote:
> > > lgtm (apologies for the delay)
> > 
> > Thanks, Michael -- and I'm sorry I didn't see this lgtm; so I pushed one
more
> > patch up -- it includes the scaffolding for getRegistration and
> > getRegistrations. Can I bother you for one more quick review?
> 
> Also, I'm not sure who should be OWNER on
> public/platform/modules/background_sync -- any opinions?

I think you should be an owner :-) Probably together with Josh, who will be
reviewing and touching this feature a lot, right?

[iclelland, 2015-03-10 19:36:14]

On 2015/03/10 19:30:16, Michael van Ouwerkerk wrote:
> On 2015/03/10 19:06:19, iclelland wrote:
> > On 2015/03/10 18:49:12, iclelland wrote:
> > > On 2015/03/10 16:58:59, Michael van Ouwerkerk wrote:
> > > > lgtm (apologies for the delay)
> > > 
> > > Thanks, Michael -- and I'm sorry I didn't see this lgtm; so I pushed one
> more
> > > patch up -- it includes the scaffolding for getRegistration and
> > > getRegistrations. Can I bother you for one more quick review?
> > 
> > Also, I'm not sure who should be OWNER on
> > public/platform/modules/background_sync -- any opinions?
> 
> I think you should be an owner :-) Probably together with Josh, who will be
> reviewing and touching this feature a lot, right?

Thanks -- I'll update and commit :)

[iclelland, 2015-03-10 21:11:07]

iclelland@chromium.org changed reviewers:
+ jochen@chromium.org, michaeln@chromium.org

[iclelland, 2015-03-10 21:12:21]

On 2015/03/10 19:36:14, iclelland wrote:
> On 2015/03/10 19:30:16, Michael van Ouwerkerk wrote:
> > On 2015/03/10 19:06:19, iclelland wrote:
> > > On 2015/03/10 18:49:12, iclelland wrote:
> > > > On 2015/03/10 16:58:59, Michael van Ouwerkerk wrote:
> > > > > lgtm (apologies for the delay)
> > > > 
> > > > Thanks, Michael -- and I'm sorry I didn't see this lgtm; so I pushed one
> > more
> > > > patch up -- it includes the scaffolding for getRegistration and
> > > > getRegistrations. Can I bother you for one more quick review?
> > > 
> > > Also, I'm not sure who should be OWNER on
> > > public/platform/modules/background_sync -- any opinions?
> > 
> > I think you should be an owner :-) Probably together with Josh, who will be
> > reviewing and touching this feature a lot, right?
> 
> Thanks -- I'll update and commit :)

+jochen +michaeln

Do I need an OWNERS review for the ServiceWorkerGlobalScope.idl (reverts the
sync event added in https://codereview.chromium.org/224333003/) and for
public/platform/Platform.h?

[michaeln, 2015-03-11 20:54:31]

r/s lgtm

[jochen (gone - plz use gerrit), 2015-03-13 14:39:03]

just from looking at the implementation, it's rather difficult to figure out
what the change is supposed to do.

could you please expand the CL description somewhat so it's clearer what is
supposed to be worker after this change?

some initial comments inline

https://codereview.chromium.org/963683002/diff/260001/Source/modules/backgrou...
File Source/modules/background_sync/OWNERS (right):

https://codereview.chromium.org/963683002/diff/260001/Source/modules/backgrou...
Source/modules/background_sync/OWNERS:3: jkarlin@chromium.org
why a different set of owners for the module and the headers?

https://codereview.chromium.org/963683002/diff/260001/Source/modules/backgrou...
File Source/modules/background_sync/SyncManager.cpp (right):

https://codereview.chromium.org/963683002/diff/260001/Source/modules/backgrou...
Source/modules/background_sync/SyncManager.cpp:28: unsigned long
kMinAllowablePeriod = 60000;
don't indent in a namespace

what's the unit of time?

https://codereview.chromium.org/963683002/diff/260001/Source/modules/backgrou...
File Source/modules/background_sync/SyncManager.h (right):

https://codereview.chromium.org/963683002/diff/260001/Source/modules/backgrou...
Source/modules/background_sync/SyncManager.h:20: : public
GarbageCollectedFinalized<SyncManager>
plz use blink style (all derived classes in one line)

https://codereview.chromium.org/963683002/diff/260001/public/platform/modules...
File public/platform/modules/background_sync/WebSyncRegistration.h (right):

https://codereview.chromium.org/963683002/diff/260001/public/platform/modules...
public/platform/modules/background_sync/WebSyncRegistration.h:23: unsigned long
maxDelay, unsigned long minPeriod, NetworkType minRequiredNetwork,
what units do those delays have?

[jkarlin, 2015-03-13 18:51:39]

lgtm with nit about OWNERS file. Sorry for the flow response, was on vacation.

https://codereview.chromium.org/963683002/diff/260001/Source/modules/backgrou...
File Source/modules/background_sync/SyncManager.cpp (right):

https://codereview.chromium.org/963683002/diff/260001/Source/modules/backgrou...
Source/modules/background_sync/SyncManager.cpp:28: unsigned long
kMinAllowablePeriod = 60000;
On 2015/03/13 14:39:03, jochen (slow) wrote:
> don't indent in a namespace

Oddly enough the Blink style guide says that nested namespaces should be
indented.

> 
> what's the unit of time?

https://codereview.chromium.org/963683002/diff/260001/public/platform/modules...
File public/platform/modules/background_sync/OWNERS (right):

https://codereview.chromium.org/963683002/diff/260001/public/platform/modules...
public/platform/modules/background_sync/OWNERS:1: iclelland@chromium.org
iclelland@ is a clear choice for an owner except that I don't believe he's a
comitter yet and so not yet eligible. Both OWNERS files in this CL should
probably just be jkarlin@ for now. You could add mvanouwerkerk@ as well if he is
interested as the blink code for this feature is largely a copy of PUSH.

[jkarlin, 2015-03-13 18:52:00]

On 2015/03/13 18:51:39, jkarlin wrote:
> lgtm with nit about OWNERS file. Sorry for the flow response, was on vacation.
> 
>
https://codereview.chromium.org/963683002/diff/260001/Source/modules/backgrou...
> File Source/modules/background_sync/SyncManager.cpp (right):
> 
>
https://codereview.chromium.org/963683002/diff/260001/Source/modules/backgrou...
> Source/modules/background_sync/SyncManager.cpp:28: unsigned long
> kMinAllowablePeriod = 60000;
> On 2015/03/13 14:39:03, jochen (slow) wrote:
> > don't indent in a namespace
> 
> Oddly enough the Blink style guide says that nested namespaces should be
> indented.
> 
> > 
> > what's the unit of time?
> 
>
https://codereview.chromium.org/963683002/diff/260001/public/platform/modules...
> File public/platform/modules/background_sync/OWNERS (right):
> 
>
https://codereview.chromium.org/963683002/diff/260001/public/platform/modules...
> public/platform/modules/background_sync/OWNERS:1:
mailto:iclelland@chromium.org
> iclelland@ is a clear choice for an owner except that I don't believe he's a
> comitter yet and so not yet eligible. Both OWNERS files in this CL should
> probably just be jkarlin@ for now. You could add mvanouwerkerk@ as well if he
is
> interested as the blink code for this feature is largely a copy of PUSH.

slow response that is :)

[iclelland, 2015-03-14 03:09:53]

On 2015/03/13 18:52:00, jkarlin wrote:
> On 2015/03/13 18:51:39, jkarlin wrote:
> > lgtm with nit about OWNERS file. Sorry for the flow response, was on
vacation.
> > 
> >
>
https://codereview.chromium.org/963683002/diff/260001/Source/modules/backgrou...
> > File Source/modules/background_sync/SyncManager.cpp (right):
> > 
> >
>
https://codereview.chromium.org/963683002/diff/260001/Source/modules/backgrou...
> > Source/modules/background_sync/SyncManager.cpp:28: unsigned long
> > kMinAllowablePeriod = 60000;
> > On 2015/03/13 14:39:03, jochen (slow) wrote:
> > > don't indent in a namespace
> > 
> > Oddly enough the Blink style guide says that nested namespaces should be
> > indented.

That's what I had thought -- I reread it today, though, and it seems to be
clear: "The contents of an outermost namespace block (and any nested namespaces
with the same scope) should not be indented.". So, de-indent.

> > 
> > > 
> > > what's the unit of time?
> > 
> >
>
https://codereview.chromium.org/963683002/diff/260001/public/platform/modules...
> > File public/platform/modules/background_sync/OWNERS (right):
> > 
> >
>
https://codereview.chromium.org/963683002/diff/260001/public/platform/modules...
> > public/platform/modules/background_sync/OWNERS:1:
> mailto:iclelland@chromium.org
> > iclelland@ is a clear choice for an owner except that I don't believe he's a
> > comitter yet and so not yet eligible. Both OWNERS files in this CL should
> > probably just be jkarlin@ for now. You could add mvanouwerkerk@ as well if
he
> is
> > interested as the blink code for this feature is largely a copy of PUSH.

Will fix. Thanks for clarifying.
> 
> slow response that is :)

[jkarlin, 2015-03-15 12:43:10]

https://codereview.chromium.org/963683002/diff/260001/Source/modules/backgrou...
File Source/modules/background_sync/SyncManager.cpp (right):

https://codereview.chromium.org/963683002/diff/260001/Source/modules/backgrou...
Source/modules/background_sync/SyncManager.cpp:28: unsigned long
kMinAllowablePeriod = 60000;
On 2015/03/13 18:51:39, jkarlin wrote:
> On 2015/03/13 14:39:03, jochen (slow) wrote:
> > don't indent in a namespace
> 
> Oddly enough the Blink style guide says that nested namespaces should be
> indented.
> 
> > 
> > what's the unit of time?
> 

Ah, I'm wrong.  "The contents of other nested namespaces should be indented."
but I missed the "other" part, referring to non-outermost.

[iclelland, 2015-03-16 13:09:40]

Updated for review comments.

Also added one more patch -- The service worker registration object wasn't being
passed to the browser side (required for actually *registering* syncs with a
service worker). This is corrected.

https://codereview.chromium.org/963683002/diff/260001/Source/modules/backgrou...
File Source/modules/background_sync/OWNERS (right):

https://codereview.chromium.org/963683002/diff/260001/Source/modules/backgrou...
Source/modules/background_sync/OWNERS:3: jkarlin@chromium.org
On 2015/03/13 14:39:03, jochen (slow) wrote:
> why a different set of owners for the module and the headers?

Done. per jkarlin's comments, changed to just jkarlin@ (and aligned with the
other file)

https://codereview.chromium.org/963683002/diff/260001/Source/modules/backgrou...
File Source/modules/background_sync/SyncManager.cpp (right):

https://codereview.chromium.org/963683002/diff/260001/Source/modules/backgrou...
Source/modules/background_sync/SyncManager.cpp:28: unsigned long
kMinAllowablePeriod = 60000;
On 2015/03/13 14:39:03, jochen (slow) wrote:
> don't indent in a namespace
> 
> what's the unit of time?

I'll comment in the code, but all times are in milliseconds. (minDelay,
maxDelay, minPeriod, minAllowablePeriod)

https://codereview.chromium.org/963683002/diff/260001/Source/modules/backgrou...
Source/modules/background_sync/SyncManager.cpp:28: unsigned long
kMinAllowablePeriod = 60000;
On 2015/03/15 12:43:10, jkarlin wrote:
> On 2015/03/13 18:51:39, jkarlin wrote:
> > On 2015/03/13 14:39:03, jochen (slow) wrote:
> > > don't indent in a namespace
> > 
> > Oddly enough the Blink style guide says that nested namespaces should be
> > indented.
> > 
> > > 
> > > what's the unit of time?
> > 
> 
> Ah, I'm wrong.  "The contents of other nested namespaces should be indented."
> but I missed the "other" part, referring to non-outermost.

Done.

https://codereview.chromium.org/963683002/diff/260001/Source/modules/backgrou...
File Source/modules/background_sync/SyncManager.h (right):

https://codereview.chromium.org/963683002/diff/260001/Source/modules/backgrou...
Source/modules/background_sync/SyncManager.h:20: : public
GarbageCollectedFinalized<SyncManager>
On 2015/03/13 14:39:03, jochen (slow) wrote:
> plz use blink style (all derived classes in one line)

Done.

https://codereview.chromium.org/963683002/diff/260001/public/platform/modules...
File public/platform/modules/background_sync/OWNERS (right):

https://codereview.chromium.org/963683002/diff/260001/public/platform/modules...
public/platform/modules/background_sync/OWNERS:1: iclelland@chromium.org
On 2015/03/13 18:51:39, jkarlin wrote:
> iclelland@ is a clear choice for an owner except that I don't believe he's a
> comitter yet and so not yet eligible. Both OWNERS files in this CL should
> probably just be jkarlin@ for now. You could add mvanouwerkerk@ as well if he
is
> interested as the blink code for this feature is largely a copy of PUSH.

Done.

https://codereview.chromium.org/963683002/diff/260001/public/platform/modules...
File public/platform/modules/background_sync/WebSyncRegistration.h (right):

https://codereview.chromium.org/963683002/diff/260001/public/platform/modules...
public/platform/modules/background_sync/WebSyncRegistration.h:23: unsigned long
maxDelay, unsigned long minPeriod, NetworkType minRequiredNetwork,
On 2015/03/13 14:39:03, jochen (slow) wrote:
> what units do those delays have?

All are in milliseconds. (the suffix was removed from the spec:
https://github.com/slightlyoff/BackgroundSync/commit/551e84be3a5f746400cf756d...
)

[jochen (gone - plz use gerrit), 2015-03-17 12:25:35]

lgtm with nits addressed

https://codereview.chromium.org/963683002/diff/320001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/963683002/diff/320001/public/platform/Platfor...
public/platform/Platform.h:665: 
unrelated

https://codereview.chromium.org/963683002/diff/320001/public/platform/modules...
File public/platform/modules/background_sync/WebSyncRegistration.h (right):

https://codereview.chromium.org/963683002/diff/320001/public/platform/modules...
public/platform/modules/background_sync/WebSyncRegistration.h:23: unsigned long
maxDelay, unsigned long minPeriod, NetworkType minRequiredNetwork,
even if the idl doesn't have units, I think we should have units on internal
variable names.

https://codereview.chromium.org/963683002/diff/320001/public/platform/modules...
public/platform/modules/background_sync/WebSyncRegistration.h:32: {
what about adding an assert that minDelay <= maxDelay or whatever the actual
constraint is?

[iclelland, 2015-03-19 13:13:43]

https://codereview.chromium.org/963683002/diff/320001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/963683002/diff/320001/public/platform/Platfor...
public/platform/Platform.h:665: 
On 2015/03/17 12:25:35, jochen (slow) wrote:
> unrelated

Done.

https://codereview.chromium.org/963683002/diff/320001/public/platform/modules...
File public/platform/modules/background_sync/WebSyncRegistration.h (right):

https://codereview.chromium.org/963683002/diff/320001/public/platform/modules...
public/platform/modules/background_sync/WebSyncRegistration.h:23: unsigned long
maxDelay, unsigned long minPeriod, NetworkType minRequiredNetwork,
On 2015/03/17 12:25:35, jochen (slow) wrote:
> even if the idl doesn't have units, I think we should have units on internal
> variable names.

I'll add this as "millis" (By analogy with Java's getCurrentTimeMillis() call).
Is that an appropriate suffix, or should it be "milliseconds", "ms", or (the
camelCaseMegaSecond) "Ms"?

https://codereview.chromium.org/963683002/diff/320001/public/platform/modules...
public/platform/modules/background_sync/WebSyncRegistration.h:32: {
On 2015/03/17 12:25:35, jochen (slow) wrote:
> what about adding an assert that minDelay <= maxDelay or whatever the actual
> constraint is?

I suppose the actual constraint would be
(minDelay <= maxDelay) || (maxDelay == 0)

if there was to be one. Neither of these parameters are hard requirements for
the browser to obey. When passed in from a register() call, they represent the
web site's request (which may not be honoured), and when passed back to blink
from the browser, they would just represent the values which have been stored,
possibly after modification by the user or the browser (and still don't mean
that the browser promises to obey them).

Does the assert make sense in the context of a struct constructor? Without being
a class with private data members, it doesn't actually enforce anything. I can
easily assert in the syncManager.register() call, and reject if the constraint
is violated.

[jochen (gone - plz use gerrit), 2015-03-19 13:24:24]

On 2015/03/19 at 13:13:43, iclelland wrote:
>
https://codereview.chromium.org/963683002/diff/320001/public/platform/Platform.h
> File public/platform/Platform.h (right):
> 
>
https://codereview.chromium.org/963683002/diff/320001/public/platform/Platfor...
> public/platform/Platform.h:665: 
> On 2015/03/17 12:25:35, jochen (slow) wrote:
> > unrelated
> 
> Done.
> 
>
https://codereview.chromium.org/963683002/diff/320001/public/platform/modules...
> File public/platform/modules/background_sync/WebSyncRegistration.h (right):
> 
>
https://codereview.chromium.org/963683002/diff/320001/public/platform/modules...
> public/platform/modules/background_sync/WebSyncRegistration.h:23: unsigned
long maxDelay, unsigned long minPeriod, NetworkType minRequiredNetwork,
> On 2015/03/17 12:25:35, jochen (slow) wrote:
> > even if the idl doesn't have units, I think we should have units on internal
> > variable names.
> 
> I'll add this as "millis" (By analogy with Java's getCurrentTimeMillis()
call). Is that an appropriate suffix, or should it be "milliseconds", "ms", or
(the camelCaseMegaSecond) "Ms"?

public/platform/WebThread.h uses Ms as suffix.

Other APIs use double fooSeconds

> 
>
https://codereview.chromium.org/963683002/diff/320001/public/platform/modules...
> public/platform/modules/background_sync/WebSyncRegistration.h:32: {
> On 2015/03/17 12:25:35, jochen (slow) wrote:
> > what about adding an assert that minDelay <= maxDelay or whatever the actual
> > constraint is?
> 
> I suppose the actual constraint would be
> (minDelay <= maxDelay) || (maxDelay == 0)
> 
> if there was to be one. Neither of these parameters are hard requirements for
the browser to obey. When passed in from a register() call, they represent the
web site's request (which may not be honoured), and when passed back to blink
from the browser, they would just represent the values which have been stored,
possibly after modification by the user or the browser (and still don't mean
that the browser promises to obey them).
> 
> Does the assert make sense in the context of a struct constructor? Without
being a class with private data members, it doesn't actually enforce anything. I
can easily assert in the syncManager.register() call, and reject if the
constraint is violated.

maybe at least document it in a comment in the header?

[iclelland, 2015-03-19 14:37:14]

On 2015/03/19 13:24:24, jochen (slow) wrote:
> On 2015/03/19 at 13:13:43, iclelland wrote:
> >
>
https://codereview.chromium.org/963683002/diff/320001/public/platform/Platform.h
> > File public/platform/Platform.h (right):
> > 
> >
>
https://codereview.chromium.org/963683002/diff/320001/public/platform/Platfor...
> > public/platform/Platform.h:665: 
> > On 2015/03/17 12:25:35, jochen (slow) wrote:
> > > unrelated
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/963683002/diff/320001/public/platform/modules...
> > File public/platform/modules/background_sync/WebSyncRegistration.h (right):
> > 
> >
>
https://codereview.chromium.org/963683002/diff/320001/public/platform/modules...
> > public/platform/modules/background_sync/WebSyncRegistration.h:23: unsigned
> long maxDelay, unsigned long minPeriod, NetworkType minRequiredNetwork,
> > On 2015/03/17 12:25:35, jochen (slow) wrote:
> > > even if the idl doesn't have units, I think we should have units on
internal
> > > variable names.
> > 
> > I'll add this as "millis" (By analogy with Java's getCurrentTimeMillis()
> call). Is that an appropriate suffix, or should it be "milliseconds", "ms", or
> (the camelCaseMegaSecond) "Ms"?
> 
> public/platform/WebThread.h uses Ms as suffix.
> 
> Other APIs use double fooSeconds

Done. Changed to Ms for consistency. (The spec calls for milliseconds on the JS
side, so maintaining consistency with that).
> 
> > 
> >
>
https://codereview.chromium.org/963683002/diff/320001/public/platform/modules...
> > public/platform/modules/background_sync/WebSyncRegistration.h:32: {
> > On 2015/03/17 12:25:35, jochen (slow) wrote:
> > > what about adding an assert that minDelay <= maxDelay or whatever the
actual
> > > constraint is?
> > 
> > I suppose the actual constraint would be
> > (minDelay <= maxDelay) || (maxDelay == 0)
> > 
> > if there was to be one. Neither of these parameters are hard requirements
for
> the browser to obey. When passed in from a register() call, they represent the
> web site's request (which may not be honoured), and when passed back to blink
> from the browser, they would just represent the values which have been stored,
> possibly after modification by the user or the browser (and still don't mean
> that the browser promises to obey them).
> > 
> > Does the assert make sense in the context of a struct constructor? Without
> being a class with private data members, it doesn't actually enforce anything.
I
> can easily assert in the syncManager.register() call, and reject if the
> constraint is violated.
> 
> maybe at least document it in a comment in the header?

Done. Comments added to document the semantics of minDelayMs, maxDelayMs and
minPeriodMs.

[iclelland, 2015-03-20 17:58:58]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2015-03-20 17:58:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvanouwerkerk@chromium.org, michaeln@chromium.org, jkarlin@chromium.org,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/963683002/#ps400001
(title: "Add missing serviceWorkerRegistration.syncManager to interface tests")

[commit-bot: I haz the power, 2015-03-20 17:59:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/963683002/400001

[commit-bot: I haz the power, 2015-03-20 19:28:08]

Committed patchset #21 (id:400001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192279