[Background Sync] Add renderer code to interface between JS API and back-end service

[Background Sync] Add renderer code to interface between JS API and back-end service

This patch introduces a BackgroundSyncProvider for use by the Blink
BackgroundSync module, as well as a proxy object which is used when calling the
provider from off of the main thread. The code in BlinkPlatformImpl will
dispatch to the appropriate object depending on the running thread.

This patch depends on the Mojo service definitions in
https://codereview.chromium.org/1106193002

BUG=479251
Committed: https://crrev.com/c9505ef0e59260df2ae80dd8838e55bb0abb4b5d
Cr-Commit-Position: refs/heads/master@{#329417}

[iclelland, 2015-04-29 20:04:31]

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

[iclelland, 2015-04-29 20:06:18]

jkarlin@ - PTAL -- this does not include the actual mojo service, which will be
a separate CL.

With this patch, the methods will all be exposed to JS, but will return promises
which never resolve. (Though a message will be dispatched from the correct
threads to the non-existing mojo service)

[jkarlin, 2015-05-01 16:13:52]

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider.cc:33: :
service_registry_(service_registry) {
DCHECK(service_registry_)?

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider.cc:84: delete this;
This object is owned by BlinkPlatformImpl right? Seems like a bad idea to delete
it here.

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider.cc:180:
callbacks->onSuccess(new bool(success));
Why does onSuccess take a bool* instead of just a bool?

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider.cc:184: "Sync
registration does not exist"));
Please create a bug (and a TODO) to handle the various errors that are returned.

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider.cc:188:
WorkerTaskRunner::Instance()->PostTask(
Add a comment that it's safe to call PostTask() on a destroyed worker thread.

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
File content/child/background_sync/background_sync_provider.h (right):

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider.h:18: 
Add a class comment here.

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider.h:20: public
WorkerTaskRunner::Observer {
Why is this a WorkerTaskRunner observer? I don't see any calls to
AddStopObserver or RemoveStopObserver.

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider.h:51: const
blink::WebSyncRegistration* options,
This should be a scoped_ptr.

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider.h:71: void
registerCallback(blink::WebSyncRegistrationCallbacks* callbacks,
Caps: RegisterCallback

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider.h:74: void
unregisterCallback(blink::WebSyncUnregistrationCallbacks* callbacks,
ditto

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider.h:76: void
getRegistrationsCallback(
ditto

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
File content/child/background_sync/background_sync_provider_thread_proxy.h
(right):

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider_thread_proxy.h:8:
#include "base/id_map.h"
remove

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider_thread_proxy.h:64:
BackgroundSyncProvider* sync_provider_;
Document why sync_provider_ will outlive this object.

[iclelland, 2015-05-05 14:52:02]

Addressing review comments: Still a scoped_ptr issue to resolve, and some object
lifetime comments.

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider.cc:33: :
service_registry_(service_registry) {
On 2015/05/01 16:13:51, jkarlin wrote:
> DCHECK(service_registry_)?

Done.

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider.cc:180:
callbacks->onSuccess(new bool(success));
On 2015/05/01 16:13:51, jkarlin wrote:
> Why does onSuccess take a bool* instead of just a bool?

The template for WebCallbacks makes pointers out of its type arguments; it's not
possible to make an onSuccess(bool) method. See
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider.cc:184: "Sync
registration does not exist"));
On 2015/05/01 16:13:51, jkarlin wrote:
> Please create a bug (and a TODO) to handle the various errors that are
returned.

Done. crbug.com/484306

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider.cc:188:
WorkerTaskRunner::Instance()->PostTask(
On 2015/05/01 16:13:51, jkarlin wrote:
> Add a comment that it's safe to call PostTask() on a destroyed worker thread.

Done.

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
File content/child/background_sync/background_sync_provider.h (right):

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider.h:18: 
On 2015/05/01 16:13:51, jkarlin wrote:
> Add a class comment here.

Done.

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider.h:20: public
WorkerTaskRunner::Observer {
On 2015/05/01 16:13:51, jkarlin wrote:
> Why is this a WorkerTaskRunner observer? I don't see any calls to
> AddStopObserver or RemoveStopObserver.

This was a holdover from the template I used; Should be deletable.

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider.h:71: void
registerCallback(blink::WebSyncRegistrationCallbacks* callbacks,
On 2015/05/01 16:13:51, jkarlin wrote:
> Caps: RegisterCallback

Done.

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider.h:74: void
unregisterCallback(blink::WebSyncUnregistrationCallbacks* callbacks,
On 2015/05/01 16:13:51, jkarlin wrote:
> ditto

Done.

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider.h:76: void
getRegistrationsCallback(
On 2015/05/01 16:13:51, jkarlin wrote:
> ditto

Done.

[iclelland, 2015-05-07 06:03:49]

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

[iclelland, 2015-05-07 06:03:50]

+r jochen -- can you take a look?

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider.cc:84: delete this;
On 2015/05/01 16:13:51, jkarlin wrote:
> This object is owned by BlinkPlatformImpl right? Seems like a bad idea to
delete
> it here.

Right. I've removed this method, as it was part of the
WorkerTaskRunner::Observer interface, and not needed.

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
File content/child/background_sync/background_sync_provider.h (right):

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider.h:51: const
blink::WebSyncRegistration* options,
On 2015/05/01 16:13:51, jkarlin wrote:
> This should be a scoped_ptr.

Done.

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
File content/child/background_sync/background_sync_provider_thread_proxy.h
(right):

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider_thread_proxy.h:8:
#include "base/id_map.h"
On 2015/05/01 16:13:52, jkarlin wrote:
> remove

Done.

https://codereview.chromium.org/1104283002/diff/20001/content/child/backgroun...
content/child/background_sync/background_sync_provider_thread_proxy.h:64:
BackgroundSyncProvider* sync_provider_;
On 2015/05/01 16:13:52, jkarlin wrote:
> Document why sync_provider_ will outlive this object.

Done.

[jkarlin, 2015-05-07 11:51:53]

Nice. Just a few more comments.

https://codereview.chromium.org/1104283002/diff/80001/content/child/backgroun...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1104283002/diff/80001/content/child/backgroun...
content/child/background_sync/background_sync_provider.cc:48:
const_cast<blink::WebSyncRegistration*>(options));
You could keep the constness of the pointer by having
RegisterBackgroundSyncForWorker take scoped_ptr<const
blink::WebSyncRegistration>. You could then compact this some:

RegisterBackgroundSyncForWorker(make_scoped_ptr(options),                       
      service_worker_registration, callbacks,kNoWorkerThread);

https://codereview.chromium.org/1104283002/diff/80001/content/child/backgroun...
content/child/background_sync/background_sync_provider.cc:87: void
BackgroundSyncProvider::RegisterBackgroundSyncForWorker(
Given the comments for this class, it seems like BackgroundSyncProvider should
be worker-agnostic. Worker code should be in the ProxyThread class. In which
case, how about removing the *ForWorker functions and removing the worker code
from the *Callback functions. Then, you can put the callback thread jumping
logic in the callback that the proxy provides to the BackgroundSyncProvider.

In other words:

BackgroundSyncProviderThreadProxy::getRegistration(callback) {
  // 1. Create a new WebSyncRegistrationCallbacks that jumps to this worker
thread and then runs |callback| with the success or error. 
  // 2. Jump to the main thread and call BackgroundSyncProvider::getRegistration
with the new callback.
}

https://codereview.chromium.org/1104283002/diff/80001/content/child/backgroun...
content/child/background_sync/background_sync_provider.cc:223: (*results)[i] =
mojo::ConvertTo<scoped_ptr<blink::WebSyncRegistration>>(
make_scoped_ptr(registrations[i])

https://codereview.chromium.org/1104283002/diff/80001/content/child/backgroun...
File content/child/background_sync/background_sync_provider_thread_proxy.cc
(right):

https://codereview.chromium.org/1104283002/diff/80001/content/child/backgroun...
content/child/background_sync/background_sync_provider_thread_proxy.cc:55:
Passed(options_ptr.Pass()),
base::Passed?

This could be "base::Passed(make_scoped_ptr(options))" if
RegisterBackgroundSyncForWorker took a scoped_ptr<const
blink::WebSyncRegistration>.

https://codereview.chromium.org/1104283002/diff/80001/content/child/backgroun...
File content/child/background_sync/background_sync_provider_thread_proxy.h
(right):

https://codereview.chromium.org/1104283002/diff/80001/content/child/backgroun...
content/child/background_sync/background_sync_provider_thread_proxy.h:21: //
BackgroundSyncProviderThreadProxy is a a proxy to the BackgroundSyncProvider
"is a a proxy" -> "is a proxy"

[jochen (gone - plz use gerrit), 2015-05-07 15:09:52]

https://codereview.chromium.org/1104283002/diff/80001/content/common/DEPS
File content/common/DEPS (right):

https://codereview.chromium.org/1104283002/diff/80001/content/common/DEPS#new...
content/common/DEPS:40:
"+third_party/WebKit/public/platform/modules/background_sync/WebSyncError.h",
where is that required?

[iclelland, 2015-05-09 06:41:17]

https://codereview.chromium.org/1104283002/diff/80001/content/child/backgroun...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1104283002/diff/80001/content/child/backgroun...
content/child/background_sync/background_sync_provider.cc:48:
const_cast<blink::WebSyncRegistration*>(options));
On 2015/05/07 11:51:53, jkarlin wrote:
> You could keep the constness of the pointer by having
> RegisterBackgroundSyncForWorker take scoped_ptr<const
> blink::WebSyncRegistration>. You could then compact this some:
> 
> RegisterBackgroundSyncForWorker(make_scoped_ptr(options),                     
 
>       service_worker_registration, callbacks,kNoWorkerThread);

Done. But then I removed all of the *ForWorker functions, so moot.

https://codereview.chromium.org/1104283002/diff/80001/content/child/backgroun...
content/child/background_sync/background_sync_provider.cc:87: void
BackgroundSyncProvider::RegisterBackgroundSyncForWorker(
On 2015/05/07 11:51:53, jkarlin wrote:
> Given the comments for this class, it seems like BackgroundSyncProvider should
> be worker-agnostic. Worker code should be in the ProxyThread class. In which
> case, how about removing the *ForWorker functions and removing the worker code
> from the *Callback functions. Then, you can put the callback thread jumping
> logic in the callback that the proxy provides to the BackgroundSyncProvider.
> 
> In other words:
> 
> BackgroundSyncProviderThreadProxy::getRegistration(callback) {
>   // 1. Create a new WebSyncRegistrationCallbacks that jumps to this worker
> thread and then runs |callback| with the success or error. 
>   // 2. Jump to the main thread and call
BackgroundSyncProvider::getRegistration
> with the new callback.
> }

Done. I really like this approach, rather than switching behaviour based on the
worker_thread_id. There's a bit more work to do here, mostly regarding pointer
ownership of the various callback objects, but I've updated the CL with the new
structure.

https://codereview.chromium.org/1104283002/diff/80001/content/child/backgroun...
content/child/background_sync/background_sync_provider.cc:223: (*results)[i] =
mojo::ConvertTo<scoped_ptr<blink::WebSyncRegistration>>(
On 2015/05/07 11:51:53, jkarlin wrote:
> make_scoped_ptr(registrations[i])

registrations[i] is a content::SyncRegistrationPtr; here we're calling
mojo::ConvertTo<> on that object, to return a
scoped_ptr<blink::WebSyncRegistration> (from comments on
https://codereview.chromium.org/1106193002/diff/1/content/child/background_sy...)

https://codereview.chromium.org/1104283002/diff/80001/content/child/backgroun...
File content/child/background_sync/background_sync_provider_thread_proxy.cc
(right):

https://codereview.chromium.org/1104283002/diff/80001/content/child/backgroun...
content/child/background_sync/background_sync_provider_thread_proxy.cc:55:
Passed(options_ptr.Pass()),
On 2015/05/07 11:51:53, jkarlin wrote:
> base::Passed?
> 
> This could be "base::Passed(make_scoped_ptr(options))" if
> RegisterBackgroundSyncForWorker took a scoped_ptr<const
> blink::WebSyncRegistration>.

Yes; that's better -- but based on the other comments in
background_sync_provider.cc, I've changed this significantly. This now calls
registerBackgroundSync directly, which must take a blink::WebSyncRegistration*
(because of the definitions in blink::WebSyncProvider), so it just passes
options directly.

https://codereview.chromium.org/1104283002/diff/80001/content/child/backgroun...
File content/child/background_sync/background_sync_provider_thread_proxy.h
(right):

https://codereview.chromium.org/1104283002/diff/80001/content/child/backgroun...
content/child/background_sync/background_sync_provider_thread_proxy.h:21: //
BackgroundSyncProviderThreadProxy is a a proxy to the BackgroundSyncProvider
On 2015/05/07 11:51:53, jkarlin wrote:
> "is a a proxy" -> "is a proxy"

Done.

https://codereview.chromium.org/1104283002/diff/80001/content/common/DEPS
File content/common/DEPS (right):

https://codereview.chromium.org/1104283002/diff/80001/content/common/DEPS#new...
content/common/DEPS:40:
"+third_party/WebKit/public/platform/modules/background_sync/WebSyncError.h",
On 2015/05/07 15:09:52, jochen (traveling) wrote:
> where is that required?

Thanks for seeing that -- it isn't required any more. Removed.

[jkarlin, 2015-05-11 11:06:27]

https://codereview.chromium.org/1104283002/diff/120001/content/child/backgrou...
File content/child/background_sync/background_sync_provider_thread_proxy.h
(right):

https://codereview.chromium.org/1104283002/diff/120001/content/child/backgrou...
content/child/background_sync/background_sync_provider_thread_proxy.h:40: class
CallbackThreadAdapter : public blink::WebCallbacks<S,T> {
Cool! Can this class be moved to the src file's anonymous namespace?

[iclelland, 2015-05-11 18:57:01]

(Sorry for the previous blank message; I managed to mail comments before the
comments were saved)

https://codereview.chromium.org/1104283002/diff/120001/content/child/backgrou...
File content/child/background_sync/background_sync_provider_thread_proxy.h
(right):

https://codereview.chromium.org/1104283002/diff/120001/content/child/backgrou...
content/child/background_sync/background_sync_provider_thread_proxy.h:40: class
CallbackThreadAdapter : public blink::WebCallbacks<S,T> {
On 2015/05/11 11:06:27, jkarlin wrote:
> Cool! Can this class be moved to the src file's anonymous namespace?

Absolutely! Doesn't need to be exposed in headers.
Done in latest patch set.

[jochen (gone - plz use gerrit), 2015-05-11 23:36:30]

lgtm

[jkarlin, 2015-05-12 13:19:51]

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:47: 
Suggest creating scoped ptrs to options and callbacks at the beginning of this
function and all below so that you don't need to worry about leaking them.

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:49:
mojo::ConvertTo<SyncRegistrationPtr>(*options),
Options is leaked here.

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:52:
base::Unretained(this),callbacks));
callbacks is leaked here.

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:52:
base::Unretained(this),callbacks));
Please add a comment that it's safe to use base::Unretained here because the
mojo channel will be deleted (and wipe its callbacks) before 'this' is deleted.

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:61: DCHECK(callbacks);
Make a scoped_ptr to callbacks

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:78: DCHECK(callbacks);
ditto

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:94: DCHECK(callbacks);
ditto

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:107:
blink::WebSyncRegistrationCallbacks* callbacks,
scoped_ptr<blink::WebSyncRegistrationCallbacks> here and below.

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:108: const
SyncRegistrationPtr &options) {
SyncRegistrationPtr& options, note the & comes directly after the type. I
suspect running 'git cl format' would catch this.

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
File content/child/background_sync/background_sync_provider_thread_proxy.cc
(right):

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
content/child/background_sync/background_sync_provider_thread_proxy.cc:95: //
TODO(iclelland): Ensure this gets deleted, and doesn't leak
Ah, looks like you weren't quite finished. Anyway, I added comments for this in
the other file. Please remove these TODOs once you're done.

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
File content/child/background_sync/background_sync_provider_thread_proxy.h
(right):

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
content/child/background_sync/background_sync_provider_thread_proxy.h:22: //
instance of that class per thread.
s/of that class per thread/per worker thread/g

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
content/child/background_sync/background_sync_provider_thread_proxy.h:66:
scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner_;
insert vertical space here so that you can more easily read the comment below?

[iclelland, 2015-05-12 14:40:05]

scoped_ptr issues addressed; PTAL.

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:49:
mojo::ConvertTo<SyncRegistrationPtr>(*options),
On 2015/05/12 13:19:51, jkarlin wrote:
> Options is leaked here.

Should be fixed with the scoped_ptr now.

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:61: DCHECK(callbacks);
On 2015/05/12 13:19:50, jkarlin wrote:
> Make a scoped_ptr to callbacks

Done.

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:78: DCHECK(callbacks);
On 2015/05/12 13:19:50, jkarlin wrote:
> ditto

Done.

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:94: DCHECK(callbacks);
On 2015/05/12 13:19:51, jkarlin wrote:
> ditto

Done.

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:107:
blink::WebSyncRegistrationCallbacks* callbacks,
On 2015/05/12 13:19:50, jkarlin wrote:
> scoped_ptr<blink::WebSyncRegistrationCallbacks> here and below.

Done.

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:108: const
SyncRegistrationPtr &options) {
On 2015/05/12 13:19:50, jkarlin wrote:
> SyncRegistrationPtr& options, note the & comes directly after the type. I
> suspect running 'git cl format' would catch this.

Thanks -- I'm not sure why I had the idea that style violations were caught on
'cl presubmit', but that must just be the line length checks.

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
File content/child/background_sync/background_sync_provider_thread_proxy.cc
(right):

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
content/child/background_sync/background_sync_provider_thread_proxy.cc:95: //
TODO(iclelland): Ensure this gets deleted, and doesn't leak
On 2015/05/12 13:19:51, jkarlin wrote:
> Ah, looks like you weren't quite finished. Anyway, I added comments for this
in
> the other file. Please remove these TODOs once you're done.

Done.

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
File content/child/background_sync/background_sync_provider_thread_proxy.h
(right):

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
content/child/background_sync/background_sync_provider_thread_proxy.h:22: //
instance of that class per thread.
On 2015/05/12 13:19:51, jkarlin wrote:
> s/of that class per thread/per worker thread/g

Done.

https://codereview.chromium.org/1104283002/diff/140001/content/child/backgrou...
content/child/background_sync/background_sync_provider_thread_proxy.h:66:
scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner_;
On 2015/05/12 13:19:51, jkarlin wrote:
> insert vertical space here so that you can more easily read the comment below?

Done.

[jkarlin, 2015-05-12 14:48:33]

lgtm with nit

https://codereview.chromium.org/1104283002/diff/180001/content/child/backgrou...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1104283002/diff/180001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:48:
make_scoped_ptr(options);
nit: This can be scoped_ptr<const blink::WebSyncRegistration>
optionsPtr(options) here and below.

[iclelland, 2015-05-12 14:59:47]

https://codereview.chromium.org/1104283002/diff/180001/content/child/backgrou...
File content/child/background_sync/background_sync_provider.cc (right):

https://codereview.chromium.org/1104283002/diff/180001/content/child/backgrou...
content/child/background_sync/background_sync_provider.cc:48:
make_scoped_ptr(options);
On 2015/05/12 14:48:33, jkarlin wrote:
> nit: This can be scoped_ptr<const blink::WebSyncRegistration>
> optionsPtr(options) here and below.

Yes. Probably saves a temporary, too. Thanks.
Done.

[iclelland, 2015-05-12 15:32:42]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2015-05-12 15:32:42]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/1104283002/#ps200001
(title: "Better scoped_ptr construction (nit)")

[commit-bot: I haz the power, 2015-05-12 15:33:16]

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

[commit-bot: I haz the power, 2015-05-12 16:28:02]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-05-12 16:28:47]

Patchset 11 (id:??) landed as
https://crrev.com/c9505ef0e59260df2ae80dd8838e55bb0abb4b5d
Cr-Commit-Position: refs/heads/master@{#329417}