[background-sync] Remove WebSyncError and SyncCallbacks

[background-sync] Remove WebSyncError and SyncCallbacks

- Follow up to http://crrev.com/2473483012
- Remove WebSyncError and SyncError
- Remove SyncCallbacks and put all functionality inside BackgroundSyncProvider directly
- Remove WebSyncClient.h (file was not being used anywhere)

BUG=662134
Committed: https://crrev.com/958b47bdc79d4636041ee59a0ac7874dfca84bba
Cr-Commit-Position: refs/heads/master@{#433584}

[adithyas, 2016-11-08 22:15:26]

Description was changed from

==========
Remove WebSyncError and other Blink API dependencies

- Follow up to http://crrev.com/2473483012
- "Rename" WebSyncError to SyncError (SyncError is not a purely static
  class anymore)
- Replace all references to WebSyncError with SyncError
- Remove dependency to WebCallback
- Use Vector<SyncRegistrationPtr> instead of
  WebVector<SyncRegistration*>

BUG=662134
==========

to

==========
- Follow up to http://crrev.com/2473483012
- "Rename" WebSyncError to SyncError (SyncError is not a purely static
  class anymore)
- Replace all references to WebSyncError with SyncError
- Remove dependency to WebCallback
- Use Vector<SyncRegistrationPtr> instead of
  WebVector<SyncRegistration*>
- Remove SyncCallbacks and put all functionality inside BackgroundSyncProvider
directly

BUG=662134
==========

[adithyas, 2016-11-09 22:23:45]

Description was changed from

==========
- Follow up to http://crrev.com/2473483012
- "Rename" WebSyncError to SyncError (SyncError is not a purely static
  class anymore)
- Replace all references to WebSyncError with SyncError
- Remove dependency to WebCallback
- Use Vector<SyncRegistrationPtr> instead of
  WebVector<SyncRegistration*>
- Remove SyncCallbacks and put all functionality inside BackgroundSyncProvider
directly

BUG=662134
==========

to

==========
- Follow up to http://crrev.com/2473483012
- "Rename" WebSyncError to SyncError (SyncError is not a purely static
  class anymore)
- Replace all references to WebSyncError with SyncError
- Remove dependency to WebCallback
- Use Vector<SyncRegistrationPtr> instead of
  WebVector<SyncRegistration*>
- Remove SyncCallbacks and put all functionality inside BackgroundSyncProvider
directly
- Remove WebClient.h (file was not being used anywhere)

BUG=662134
==========

[adithyas, 2016-11-09 22:38:45]

The CQ bit was checked by adithyas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 22:39:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 22:43:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-09 22:43:54]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[adithyas, 2016-11-09 23:06:14]

The CQ bit was checked by adithyas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 23:06:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 23:10:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-09 23:10:39]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[adithyas, 2016-11-18 15:40:53]

The CQ bit was checked by adithyas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 15:41:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[adithyas, 2016-11-18 16:21:35]

adithyas@chromium.org changed reviewers:
+ dcheng@chromium.org, haraken@chromium.org, iclelland@chromium.org,
jbroman@chromium.org

[jbroman, 2016-11-18 16:42:13]

Description was changed from

==========
- Follow up to http://crrev.com/2473483012
- "Rename" WebSyncError to SyncError (SyncError is not a purely static
  class anymore)
- Replace all references to WebSyncError with SyncError
- Remove dependency to WebCallback
- Use Vector<SyncRegistrationPtr> instead of
  WebVector<SyncRegistration*>
- Remove SyncCallbacks and put all functionality inside BackgroundSyncProvider
directly
- Remove WebClient.h (file was not being used anywhere)

BUG=662134
==========

to

==========
- Follow up to http://crrev.com/2473483012
- "Rename" WebSyncError to SyncError (SyncError is not a purely static
  class anymore)
- Replace all references to WebSyncError with SyncError
- Remove dependency to WebCallback
- Use Vector<SyncRegistrationPtr> instead of
  WebVector<SyncRegistration*>
- Remove SyncCallbacks and put all functionality inside BackgroundSyncProvider
directly
- Remove WebSyncClient.h (file was not being used anywhere)

BUG=662134
==========

[jbroman, 2016-11-18 16:53:32]

I think there's a little more code that can be simplified here (see comments).

Otherwise, (non-owner) lgtm.

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp
(right):

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:21:
if (!resolver->getExecutionContext() ||
Here and below: ScriptPromiseResolver::resolveOrReject already checks internally
whether active DOM objects are stopped; is this necessary here (and below)?

If you do remove this check here, then this function becomes extremely short,
and it might be simpler to just inline it at its call site.

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:59:
void BackgroundSyncProvider::onGetRegistrationsError(
If you don't take my suggestion above and remove this function, it's probably
clearer to just remove this one and rename "onRegisterError" to a more generic
name, like "onError".

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h
(right):

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:10:
#include <memory>
super-nit: I think this include was here for unique_ptr, which is no longer used

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:15:
class ScriptPromiseResolver;
nit: alphabetize these

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:32:
ScriptPromiseResolver*);
Since there's only the one implementation of Sync*Callbacks, this seems like a
reasonable simplification to make. It does bind this class fairly closely to the
interface of SyncManager. WDYT of (probably in a followup, since it changes
lifetime semantics) merging this class into SyncManager?

That way, the method that creates the promise and the one that resolves it live
in the same class, so it's easier to see at a glance what values the promise can
be resolved/rejected with.

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/background_sync/SyncError.h (right):

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/background_sync/SyncError.h:14: class
SyncError {
Is this class still useful? It seems to just be a place to store a string and
enum until we immediately convert to DOMException in the same source file. It
seems to me that the onRegisterError calls could simply be:

case mojom::blink::BackgroundSyncError::STORAGE:
  resolver->reject(DOMException::create(UnknownError, "Background Sync is
disabled."));
  break;

and so on. WDYT?

[commit-bot: I haz the power, 2016-11-18 17:58:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-18 17:58:53]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-11-18 18:52:03]

LGTM with jbroman's comments addressed.

[adithyas, 2016-11-18 20:05:00]

Addressed jbroman's comments

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp
(right):

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:21:
if (!resolver->getExecutionContext() ||
On 2016/11/18 at 16:53:31, jbroman wrote:
> Here and below: ScriptPromiseResolver::resolveOrReject already checks
internally whether active DOM objects are stopped; is this necessary here (and
below)?
> 
> If you do remove this check here, then this function becomes extremely short,
and it might be simpler to just inline it at its call site.

Yeah, it's not necessary, I'll just inline it. The old implementation of the
callbacks had the check and I just copied them over.

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h
(right):

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:10:
#include <memory>
On 2016/11/18 at 16:53:32, jbroman wrote:
> super-nit: I think this include was here for unique_ptr, which is no longer
used

Done.

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:15:
class ScriptPromiseResolver;
On 2016/11/18 at 16:53:31, jbroman wrote:
> nit: alphabetize these

Done.

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:32:
ScriptPromiseResolver*);
On 2016/11/18 at 16:53:32, jbroman wrote:
> Since there's only the one implementation of Sync*Callbacks, this seems like a
reasonable simplification to make. It does bind this class fairly closely to the
interface of SyncManager. WDYT of (probably in a followup, since it changes
lifetime semantics) merging this class into SyncManager?
> 
> That way, the method that creates the promise and the one that resolves it
live in the same class, so it's easier to see at a glance what values the
promise can be resolved/rejected with.

Yup I definitely agree with that thought, both classes basically have identical
interfaces. This was something I wanted to change in this CL, but like you
mentioned, it changes lifetime semantics with BackgroundSyncProvider currently
having thread specific instances. 

I also wanted to know the owners' thoughts about this before changing the
implementation.

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/background_sync/SyncError.h (right):

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/background_sync/SyncError.h:14: class
SyncError {
On 2016/11/18 at 16:53:32, jbroman wrote:
> Is this class still useful? It seems to just be a place to store a string and
enum until we immediately convert to DOMException in the same source file. It
seems to me that the onRegisterError calls could simply be:
> 
> case mojom::blink::BackgroundSyncError::STORAGE:
>   resolver->reject(DOMException::create(UnknownError, "Background Sync is
disabled."));
>   break;
> 
> and so on. WDYT?

+1, I'll get rid of it. Now that I look at it again, I don't know why
SyncError::take even needs ScriptPromiseResolver as a parameter.

[adithyas, 2016-11-18 20:05:45]

The CQ bit was checked by adithyas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 20:06:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbroman, 2016-11-18 20:14:17]

Still LGTM; please update CL description to correspond with the changes you've
made.

https://codereview.chromium.org/2481393002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp
(right):

https://codereview.chromium.org/2481393002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:96:
for (const mojom::blink::SyncRegistrationPtr& r :
super-nit: that this line has to wrap is kinda unfortunate; it's okay to use
"const auto&" in cases like this where the type is clear:

for (const auto& registration : registrations.storage()) {
  tags.append(registration->tag);
}

It's not important, but an option to keep in mind. :)

[jbroman, 2016-11-18 20:15:12]

(Oh, and also keep the first line of "Description" in sync with "Title". IIRC,
only "Description" gets propagated to the git commit message, and "Title" is
purely for display in the code review UI.)

[adithyas, 2016-11-18 20:20:56]

Description was changed from

==========
- Follow up to http://crrev.com/2473483012
- "Rename" WebSyncError to SyncError (SyncError is not a purely static
  class anymore)
- Replace all references to WebSyncError with SyncError
- Remove dependency to WebCallback
- Use Vector<SyncRegistrationPtr> instead of
  WebVector<SyncRegistration*>
- Remove SyncCallbacks and put all functionality inside BackgroundSyncProvider
directly
- Remove WebSyncClient.h (file was not being used anywhere)

BUG=662134
==========

to

==========
[background-sync] Remove WebSyncError and SyncCallbacks

- Follow up to http://crrev.com/2473483012
- Remove WebSyncError and SyncError
- Remove SyncCallbacks and put all functionality inside BackgroundSyncProvider
directly
- Remove WebSyncClient.h (file was not being used anywhere)

BUG=662134
==========

[iclelland, 2016-11-18 20:53:46]

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h
(right):

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:32:
ScriptPromiseResolver*);
On 2016/11/18 20:05:00, adithyas wrote:
> On 2016/11/18 at 16:53:32, jbroman wrote:
> > Since there's only the one implementation of Sync*Callbacks, this seems like
a
> reasonable simplification to make. It does bind this class fairly closely to
the
> interface of SyncManager. WDYT of (probably in a followup, since it changes
> lifetime semantics) merging this class into SyncManager?
> > 
> > That way, the method that creates the promise and the one that resolves it
> live in the same class, so it's easier to see at a glance what values the
> promise can be resolved/rejected with.
> 
> Yup I definitely agree with that thought, both classes basically have
identical
> interfaces. This was something I wanted to change in this CL, but like you
> mentioned, it changes lifetime semantics with BackgroundSyncProvider currently
> having thread specific instances. 
> 
> I also wanted to know the owners' thoughts about this before changing the
> implementation.

The difference, I believe, is that there can be multiple SyncManagers present in
the main thread -- up to one per ServiceWorkerRegistration for the page's
origin. They all use the same BackgroundSyncProvider, though, since we only need
one IPC channel between the renderer and the browser.

https://codereview.chromium.org/2481393002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h
(right):

https://codereview.chromium.org/2481393002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:14:
class SyncError;
blink::SyncError isn't used in this file; do you need this declaration?

https://codereview.chromium.org/2481393002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/background_sync/SyncManager.cpp (right):

https://codereview.chromium.org/2481393002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/background_sync/SyncManager.cpp:57:
std::move(syncRegistration), m_registration->webRegistration(), resolver);
The old SyncRegistrationCallbacks object held a reference to the service worker
registration -- I think that was so that even if this SyncManager was destroyed,
the service worker registration object would be kept alive long enough that the
provider could handle the success/failure of the register() call, and call the
JS callback function.

I haven't traced it completely through, so I may be wrong, but I'm not sure that
we still have that guarantee here.

[iclelland, 2016-11-18 21:22:36]

On deeper inspection, this lgtm :)

https://codereview.chromium.org/2481393002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/background_sync/SyncManager.cpp (right):

https://codereview.chromium.org/2481393002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/background_sync/SyncManager.cpp:57:
std::move(syncRegistration), m_registration->webRegistration(), resolver);
On 2016/11/18 20:53:46, iclelland wrote:
> The old SyncRegistrationCallbacks object held a reference to the service
worker
> registration -- I think that was so that even if this SyncManager was
destroyed,
> the service worker registration object would be kept alive long enough that
the
> provider could handle the success/failure of the register() call, and call the
> JS callback function.
> 
> I haven't traced it completely through, so I may be wrong, but I'm not sure
that
> we still have that guarantee here.
> 
> 

Tracing through the git history, and the evolution of that file, it looks like
that reference was originally used so that we could pass an active
SyncRegistration object to the (JS) callback on success. That was pulled out in
crrev.com/1667623004, but we never got rid of the constructor arg.

I think that in this case, the fact that you're making the ScriptPromiseResolver
persistent is enough to make sure that the callback will run correctly, and we
don't actually need to keep the service worker registration object alive
explicitly.

[commit-bot: I haz the power, 2016-11-18 22:27:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-18 22:27:04]

Dry run: This issue passed the CQ dry run.

[jbroman, 2016-11-19 03:32:15]

My non-owner two cents on the lifetime discussion.

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h
(right):

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:32:
ScriptPromiseResolver*);
On 2016/11/18 at 20:53:46, iclelland wrote:
> On 2016/11/18 20:05:00, adithyas wrote:
> > On 2016/11/18 at 16:53:32, jbroman wrote:
> > > Since there's only the one implementation of Sync*Callbacks, this seems
like a
> > reasonable simplification to make. It does bind this class fairly closely to
the
> > interface of SyncManager. WDYT of (probably in a followup, since it changes
> > lifetime semantics) merging this class into SyncManager?
> > > 
> > > That way, the method that creates the promise and the one that resolves it
> > live in the same class, so it's easier to see at a glance what values the
> > promise can be resolved/rejected with.
> > 
> > Yup I definitely agree with that thought, both classes basically have
identical
> > interfaces. This was something I wanted to change in this CL, but like you
> > mentioned, it changes lifetime semantics with BackgroundSyncProvider
currently
> > having thread specific instances. 
> > 
> > I also wanted to know the owners' thoughts about this before changing the
> > implementation.
> 
> The difference, I believe, is that there can be multiple SyncManagers present
in the main thread -- up to one per ServiceWorkerRegistration for the page's
origin. They all use the same BackgroundSyncProvider, though, since we only need
one IPC channel between the renderer and the browser.

Right, that's the difference (unless there's a correctness difference I'm
missing).

Personally I'd prefer to simplify here. IIUC Mojo pipes are fairly inexpensive,
and it makes the lifetime easier to reason about without having an thread-local
object to track, as well as putting the code from JS bindings to Mojo call and
back again in one place. But you're the modules/background_sync/ owner, so it
probably matters more what you think. :)

[haraken, 2016-11-21 00:21:58]

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h
(right):

https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:32:
ScriptPromiseResolver*);
On 2016/11/19 03:32:15, jbroman wrote:
> On 2016/11/18 at 20:53:46, iclelland wrote:
> > On 2016/11/18 20:05:00, adithyas wrote:
> > > On 2016/11/18 at 16:53:32, jbroman wrote:
> > > > Since there's only the one implementation of Sync*Callbacks, this seems
> like a
> > > reasonable simplification to make. It does bind this class fairly closely
to
> the
> > > interface of SyncManager. WDYT of (probably in a followup, since it
changes
> > > lifetime semantics) merging this class into SyncManager?
> > > > 
> > > > That way, the method that creates the promise and the one that resolves
it
> > > live in the same class, so it's easier to see at a glance what values the
> > > promise can be resolved/rejected with.
> > > 
> > > Yup I definitely agree with that thought, both classes basically have
> identical
> > > interfaces. This was something I wanted to change in this CL, but like you
> > > mentioned, it changes lifetime semantics with BackgroundSyncProvider
> currently
> > > having thread specific instances. 
> > > 
> > > I also wanted to know the owners' thoughts about this before changing the
> > > implementation.
> > 
> > The difference, I believe, is that there can be multiple SyncManagers
present
> in the main thread -- up to one per ServiceWorkerRegistration for the page's
> origin. They all use the same BackgroundSyncProvider, though, since we only
need
> one IPC channel between the renderer and the browser.
> 
> Right, that's the difference (unless there's a correctness difference I'm
> missing).
> 
> Personally I'd prefer to simplify here. IIUC Mojo pipes are fairly
inexpensive,
> and it makes the lifetime easier to reason about without having an
thread-local
> object to track, as well as putting the code from JS bindings to Mojo call and
> back again in one place. But you're the modules/background_sync/ owner, so it
> probably matters more what you think. :)

+1

[dcheng, 2016-11-21 10:06:02]

mojo changes lgtm, though I'd also be interested in seeing the simplifications
(in a followup)

[iclelland, 2016-11-21 13:42:57]

On 2016/11/19 03:32:15, jbroman wrote:
> My non-owner two cents on the lifetime discussion.
> 
>
https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h
> (right):
> 
>
https://codereview.chromium.org/2481393002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:32:
> ScriptPromiseResolver*);
> On 2016/11/18 at 20:53:46, iclelland wrote:
> > On 2016/11/18 20:05:00, adithyas wrote:
> > > On 2016/11/18 at 16:53:32, jbroman wrote:
> > > > Since there's only the one implementation of Sync*Callbacks, this seems
> like a
> > > reasonable simplification to make. It does bind this class fairly closely
to
> the
> > > interface of SyncManager. WDYT of (probably in a followup, since it
changes
> > > lifetime semantics) merging this class into SyncManager?
> > > > 
> > > > That way, the method that creates the promise and the one that resolves
it
> > > live in the same class, so it's easier to see at a glance what values the
> > > promise can be resolved/rejected with.
> > > 
> > > Yup I definitely agree with that thought, both classes basically have
> identical
> > > interfaces. This was something I wanted to change in this CL, but like you
> > > mentioned, it changes lifetime semantics with BackgroundSyncProvider
> currently
> > > having thread specific instances. 
> > > 
> > > I also wanted to know the owners' thoughts about this before changing the
> > > implementation.
> > 
> > The difference, I believe, is that there can be multiple SyncManagers
present
> in the main thread -- up to one per ServiceWorkerRegistration for the page's
> origin. They all use the same BackgroundSyncProvider, though, since we only
need
> one IPC channel between the renderer and the browser.
> 
> Right, that's the difference (unless there's a correctness difference I'm
> missing).
> 
> Personally I'd prefer to simplify here. IIUC Mojo pipes are fairly
inexpensive,
> and it makes the lifetime easier to reason about without having an
thread-local
> object to track, as well as putting the code from JS bindings to Mojo call and
> back again in one place. But you're the modules/background_sync/ owner, so it
> probably matters more what you think. :)

Given that mojo connections *are* supposed to be really cheap, the
simplification is probably worth it here.

I expect that the mean number of service worker registrations explicitly used by
a given page is probably very very close to 1 (among pages which use a service
worker at all); I doubt we're seeing pages with dozens or hundreds of active
registrations, and even those are unlikely to all be using background sync :)

[adithyas, 2016-11-21 15:25:17]

https://codereview.chromium.org/2481393002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp
(right):

https://codereview.chromium.org/2481393002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.cpp:96:
for (const mojom::blink::SyncRegistrationPtr& r :
On 2016/11/18 at 20:14:17, jbroman wrote:
> super-nit: that this line has to wrap is kinda unfortunate; it's okay to use
"const auto&" in cases like this where the type is clear:
> 
> for (const auto& registration : registrations.storage()) {
>   tags.append(registration->tag);
> }
> 
> It's not important, but an option to keep in mind. :)

Done.

https://codereview.chromium.org/2481393002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h
(right):

https://codereview.chromium.org/2481393002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/background_sync/BackgroundSyncProvider.h:14:
class SyncError;
On 2016/11/18 at 20:53:46, iclelland wrote:
> blink::SyncError isn't used in this file; do you need this declaration?

Don't need it, removed :)

[adithyas, 2016-11-21 15:26:03]

The CQ bit was checked by adithyas@chromium.org

[adithyas, 2016-11-21 15:26:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, dcheng@chromium.org, jbroman@chromium.org,
iclelland@chromium.org
Link to the patchset: https://codereview.chromium.org/2481393002/#ps160001
(title: "Remove stray SyncError declaration")

[commit-bot: I haz the power, 2016-11-21 15:26:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 18:03:58]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1479741963022270,
"parent_rev": "2fea1ea0e457f5913b2a7fb436a5e86ad067d3e8", "commit_rev":
"5431ac40a7f48c62cbc455e6eae027b3d2e5d958"}

[commit-bot: I haz the power, 2016-11-21 18:04:32]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-11-21 18:06:12]

Description was changed from

==========
[background-sync] Remove WebSyncError and SyncCallbacks

- Follow up to http://crrev.com/2473483012
- Remove WebSyncError and SyncError
- Remove SyncCallbacks and put all functionality inside BackgroundSyncProvider
directly
- Remove WebSyncClient.h (file was not being used anywhere)

BUG=662134
==========

to

==========
[background-sync] Remove WebSyncError and SyncCallbacks

- Follow up to http://crrev.com/2473483012
- Remove WebSyncError and SyncError
- Remove SyncCallbacks and put all functionality inside BackgroundSyncProvider
directly
- Remove WebSyncClient.h (file was not being used anywhere)

BUG=662134

Committed: https://crrev.com/958b47bdc79d4636041ee59a0ac7874dfca84bba
Cr-Commit-Position: refs/heads/master@{#433584}
==========

[commit-bot: I haz the power, 2016-11-21 18:06:13]

Patchset 9 (id:??) landed as
https://crrev.com/958b47bdc79d4636041ee59a0ac7874dfca84bba
Cr-Commit-Position: refs/heads/master@{#433584}