[Background Sync] Use Mojo IPC to fire background sync events

[Background Sync] Use Mojo IPC to fire background sync events

This patch adds a background sync client service to the renderer process, which accepts connections from the browser, in order to trigger Sync and PeriodicSync events in the service worker. It accepts connections on the main thread, and dispatches to the worker thread to fire the events.

There is a new ServiceWorkerMojoEventDispatcher class in the browser, which makes the connection to the mojo service in the renderer. The mojo message pipe is bound to the browser IO thread.

At the browser level, the Sync and PeriodicSync events are identical, so all of the code in this patch deals only with generic "Sync" events. (The code paths are the same, and the registration structs differ only in the periodicity value.) They only diverge in the renderer, once we determine which event in the service worker script proxy to fire.

BUG=498388

[jkarlin, 2015-06-10 12:21:32]

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

[jkarlin, 2015-06-10 12:21:33]

A couple initial comments

https://codereview.chromium.org/1171173002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1171173002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:747: /* IS this the
right one? */ base::Unretained(this),
No, this is not safe. base::Unretained should rarely be used. See
https://www.chromium.org/developers/design-documents/threading for more
information. 

If you use the class I suggested in a comment in the header file, you'll be
calling that and since it's RefCountedThreadSafe it'll work fine. The function
you call will want a callback for when the event finishes. You can give that
callback a 'this' pointer since it's RefCounted and will be run on the IO
thread.

https://codereview.chromium.org/1171173002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1171173002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.h:577:
BackgroundSyncServiceClientPtr background_sync_client_;
Hmm, this whole class is IO and this one member is UI. Is it safe for this to be
deleted on the IO thread?

I recommend you make a new class that is to be accessed on the UI thread only
like so:

// All public methods must be called on UI thread. Can be constructed, owned,
and destructed on a separate thread.
class BackgroundSyncEventDispatcher : public
RefCountedThreadSafe<BackgroundSyncEventDispatcher,
BrowserThread::DeleteOnUIThread> {

public:
  // Dispatches the event and then runs |callback| on |task_runner| with the
result.
  DispatchSyncEvent(int render_process_id,
      int thread_id,
      const StatusCallback& callback, 
      const scoped_refptr<base::TaskRunner>& task_runner);

private:
  friend class base::RefCountedThreadSafe<BackgroundSyncEventDispatcher,
BrowserThread::DeleteOnUIThread>;
  ~BackgroundSyncEventDispatcher();

  BackgroundSyncServiceClientPtr background_sync_client_;
}

Then add a scoped_refptr<BackgroundSyncEventDispatcher> member to
ServiceWorkerVersion.

[iclelland, 2015-06-11 17:36:33]

Patchset #9 (id:160001) has been deleted

[iclelland, 2015-06-12 15:01:18]

I've implemented the event dispatcher for mojo-supported events; it's a
scoped_refptr on ServiceWorkerVersion, as suggested.

There is one failing unit test currently, which I believe is a false memory
leak. The dispatcher is DeleteOnUIThread, and, while the DeleteSoon call is
being posted to the UI thread, the object is not actually being deleted before
the test completes.

Josh -- do you know if there is some magic incantation that I need to add to the
SetAndUnsetVersions test to have it clear the UI message queue before it tears
down the test?

https://codereview.chromium.org/1171173002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1171173002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.h:577:
BackgroundSyncServiceClientPtr background_sync_client_;
On 2015/06/10 12:21:33, jkarlin wrote:
> Hmm, this whole class is IO and this one member is UI. Is it safe for this to
be
> deleted on the IO thread?
> 
> I recommend you make a new class that is to be accessed on the UI thread only
> like so:
> 
> // All public methods must be called on UI thread. Can be constructed, owned,
> and destructed on a separate thread.
> class BackgroundSyncEventDispatcher : public
> RefCountedThreadSafe<BackgroundSyncEventDispatcher,
> BrowserThread::DeleteOnUIThread> {
> 
> public:
>   // Dispatches the event and then runs |callback| on |task_runner| with the
> result.
>   DispatchSyncEvent(int render_process_id,
>       int thread_id,
>       const StatusCallback& callback, 
>       const scoped_refptr<base::TaskRunner>& task_runner);
> 
> private:
>   friend class base::RefCountedThreadSafe<BackgroundSyncEventDispatcher,
> BrowserThread::DeleteOnUIThread>;
>   ~BackgroundSyncEventDispatcher();
> 
>   BackgroundSyncServiceClientPtr background_sync_client_;
> }
> 
> Then add a scoped_refptr<BackgroundSyncEventDispatcher> member to
> ServiceWorkerVersion.

Would a general ServiceWorkerEventDispatcher make more sense than a
BackgroundSyncEventDispatcher?
It may be premature, but if other services move to this model, then I think I'd
rather encourage authors to extend that class than create a PushEventDispatcher
class and a GeofenceEventDispatcher class, etc.

[jkarlin, 2015-06-12 18:51:37]

> Josh -- do you know if there is some magic incantation that I need to add to
the
> SetAndUnsetVersions test to have it clear the UI message queue before it tears
> down the test?

Looks like the test framework only creates an IO thread, so the UI thread never
gets to run. Try replacing it with a browser thread bundle
(browser_thread_bundle_(TestBrowserThreadBundle::IO_MAINLOOP)) and see if that
fixes it.


> Would a general ServiceWorkerEventDispatcher make more sense than a
> BackgroundSyncEventDispatcher?
> It may be premature, but if other services move to this model, then I think
I'd
> rather encourage authors to extend that class than create a
PushEventDispatcher
> class and a GeofenceEventDispatcher class, etc.

Yes, that's a good idea.

[iclelland, 2015-06-12 19:33:00]

On 2015/06/12 18:51:37, jkarlin wrote:
> > Josh -- do you know if there is some magic incantation that I need to add to
> the
> > SetAndUnsetVersions test to have it clear the UI message queue before it
tears
> > down the test?
> 
> Looks like the test framework only creates an IO thread, so the UI thread
never
> gets to run. Try replacing it with a browser thread bundle
> (browser_thread_bundle_(TestBrowserThreadBundle::IO_MAINLOOP)) and see if that
> fixes it.

Thanks -- that did the trick.

> 
> 
> > Would a general ServiceWorkerEventDispatcher make more sense than a
> > BackgroundSyncEventDispatcher?
> > It may be premature, but if other services move to this model, then I think
> I'd
> > rather encourage authors to extend that class than create a
> PushEventDispatcher
> > class and a GeofenceEventDispatcher class, etc.
> 
> Yes, that's a good idea.

[jkarlin, 2015-06-12 21:08:15]

https://codereview.chromium.org/1171173002/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_event_dispatcher.cc (right):

https://codereview.chromium.org/1171173002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_event_dispatcher.cc:25: }
how about an else { NOTREACHED(); } for good measure?

https://codereview.chromium.org/1171173002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_event_dispatcher.cc:58:
ServiceWorkerEventDispatcher::~ServiceWorkerEventDispatcher() {
DCHECK_CURRENTLY_ON(BrowserThread::UI);

https://codereview.chromium.org/1171173002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_event_dispatcher.cc:61: void
ServiceWorkerEventDispatcher::OnEventFinished(
DCHECK_CURRENTLY_ON(BrowserThread::UI);

https://codereview.chromium.org/1171173002/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_event_dispatcher.h (right):

https://codereview.chromium.org/1171173002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_event_dispatcher.h:20: // All
public methods must be called on UI thread. Can be constructed, owned,
s/Can be constructed/This class can be constructed/

https://codereview.chromium.org/1171173002/diff/200001/content/browser/servic...
File content/browser/service_worker/service_worker_event_dispatcher.h (right):

https://codereview.chromium.org/1171173002/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_event_dispatcher.h:22: class
ServiceWorkerEventDispatcher
Suggest ServiceWorkerMojoEventDispatcher to make it clear since we now have two
IPC mechanisms in ServiceWorkerVersion to think about.

https://codereview.chromium.org/1171173002/diff/200001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1171173002/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:761: void
ServiceWorkerVersion::OnSyncEventFinished(const StatusCallback& callback,
What happens if the ServiceWorker dies mid sync? Does the browser get its
aborted callback or does it wait around forever?

https://codereview.chromium.org/1171173002/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:764: 
You're missing the TRACE_EVENT macro that the original had.

https://codereview.chromium.org/1171173002/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:769: // TODO(iclelland)
factor this out of RemoveCallbackAndStopIfRedundant
You might as well make a StopIfRedundant function now, have
RemoveCallbackAndStopIfRedundant call it, and remove this TODO.

https://codereview.chromium.org/1171173002/diff/200001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1171173002/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:216: void
OnSyncEventFinished(const StatusCallback& callback,
Did this need to be made public?

https://codereview.chromium.org/1171173002/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:573:
scoped_refptr<ServiceWorkerEventDispatcher> event_dispatcher_;
suggest mojo_event_dispatcher_

https://codereview.chromium.org/1171173002/diff/200001/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.cc (right):

https://codereview.chromium.org/1171173002/diff/200001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:30:
weak_ptr_factory_(this) {
DCHECK that we're on main_thread_task_runner's thread

https://codereview.chromium.org/1171173002/diff/200001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:35: const
SyncCallback& callback) {
DCHECK that this is called on the main thread task runner thread

https://codereview.chromium.org/1171173002/diff/200001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:46:
base::Unretained(this),
This base::Unretained makes me nervous. Can you guarantee this object's
survival?

Instead, you could make DispatchSyncForWorker and OnSyncComplete anonymous
functions or statics. They live on different threads and shouldn't have access
to member variables anyway.

https://codereview.chromium.org/1171173002/diff/200001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:56:
callback.Run(SERVICE_WORKER_EVENT_STATUS_ABORT);
Isn't callback supposed to be run on the main thread?

https://codereview.chromium.org/1171173002/diff/200001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:61:
weak_ptr_factory_.GetWeakPtr(), callback));
You can't use weak_ptr_factory_ here as you're on a different thread.

https://codereview.chromium.org/1171173002/diff/200001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:66: if
(!main_thread_task_runner_->RunsTasksOnCurrentThread()) {
This pattern is being phased out in Chromium. Better to be clear about what
thread you're on.

You can instead simply post task regardless.

https://codereview.chromium.org/1171173002/diff/200001/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.h (right):

https://codereview.chromium.org/1171173002/diff/200001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.h:17: 
Document the class. Especially, what thread does it live on?

[iclelland, 2015-06-16 15:59:16]

Comments mostly addressed; the one significant one still outstanding refers to
what happens to the callbacks if the service worker dies mid-sync.

I've also rewritten the ServiceWorkerMojoEventDispatcher, to transfer one
endpoint of the mojo message channel to the IO thread, so that we can avoid a
couple of thread hops.

https://codereview.chromium.org/1171173002/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_event_dispatcher.cc (right):

https://codereview.chromium.org/1171173002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_event_dispatcher.cc:25: }
On 2015/06/12 21:08:12, jkarlin wrote:
> how about an else { NOTREACHED(); } for good measure?

Done. The TODO will also take care of that, but this works for now.

https://codereview.chromium.org/1171173002/diff/200001/content/browser/servic...
File content/browser/service_worker/service_worker_event_dispatcher.h (right):

https://codereview.chromium.org/1171173002/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_event_dispatcher.h:22: class
ServiceWorkerEventDispatcher
On 2015/06/12 21:08:14, jkarlin wrote:
> Suggest ServiceWorkerMojoEventDispatcher to make it clear since we now have
two
> IPC mechanisms in ServiceWorkerVersion to think about.

Done.

https://codereview.chromium.org/1171173002/diff/200001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1171173002/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:764: 
On 2015/06/12 21:08:14, jkarlin wrote:
> You're missing the TRACE_EVENT macro that the original had.
You're right -- that got lost in a reorg. Fixed, thanks.
Done.

https://codereview.chromium.org/1171173002/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:769: // TODO(iclelland)
factor this out of RemoveCallbackAndStopIfRedundant
On 2015/06/12 21:08:14, jkarlin wrote:
> You might as well make a StopIfRedundant function now, have
> RemoveCallbackAndStopIfRedundant call it, and remove this TODO.

Yep; that was the intended refactor.
Done.

https://codereview.chromium.org/1171173002/diff/200001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1171173002/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:216: void
OnSyncEventFinished(const StatusCallback& callback,
On 2015/06/12 21:08:14, jkarlin wrote:
> Did this need to be made public?

No, it did not; (well, once upon a time, during refactoring, it did, but it
didn't need to stay that way.)
Moved to private, as it should be.

https://codereview.chromium.org/1171173002/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:573:
scoped_refptr<ServiceWorkerEventDispatcher> event_dispatcher_;
On 2015/06/12 21:08:14, jkarlin wrote:
> suggest mojo_event_dispatcher_

Done.

https://codereview.chromium.org/1171173002/diff/200001/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.cc (right):

https://codereview.chromium.org/1171173002/diff/200001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:30:
weak_ptr_factory_(this) {
On 2015/06/12 21:08:15, jkarlin wrote:
> DCHECK that we're on main_thread_task_runner's thread

Done.

https://codereview.chromium.org/1171173002/diff/200001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:35: const
SyncCallback& callback) {
On 2015/06/12 21:08:14, jkarlin wrote:
> DCHECK that this is called on the main thread task runner thread

Done.

https://codereview.chromium.org/1171173002/diff/200001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:46:
base::Unretained(this),
On 2015/06/12 21:08:14, jkarlin wrote:
> This base::Unretained makes me nervous. Can you guarantee this object's
> survival?
> 
> Instead, you could make DispatchSyncForWorker and OnSyncComplete anonymous
> functions or statics. They live on different threads and shouldn't have access
> to member variables anyway.

OnSyncComplete needs to have access to at least main_thread_task_runner_, but I
can pass that along.

https://codereview.chromium.org/1171173002/diff/200001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:56:
callback.Run(SERVICE_WORKER_EVENT_STATUS_ABORT);
On 2015/06/12 21:08:15, jkarlin wrote:
> Isn't callback supposed to be run on the main thread?

Yes. Fixed, thanks.

https://codereview.chromium.org/1171173002/diff/200001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:61:
weak_ptr_factory_.GetWeakPtr(), callback));
On 2015/06/12 21:08:14, jkarlin wrote:
> You can't use weak_ptr_factory_ here as you're on a different thread.

Done.

https://codereview.chromium.org/1171173002/diff/200001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:66: if
(!main_thread_task_runner_->RunsTasksOnCurrentThread()) {
On 2015/06/12 21:08:15, jkarlin wrote:
> This pattern is being phased out in Chromium. Better to be clear about what
> thread you're on.

At this point, all I know is that I'm on *a* worker thread; there's no specific
thread id here than I can check against, unless I pass long the original
thread_id from Sync, but it would only be for the purpose of a DCHECK here.
> 
> You can instead simply post task regardless.

But I'm posting back to this function; I suppose I could post the callback task
to the main thread directly instead.

https://codereview.chromium.org/1171173002/diff/200001/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.h (right):

https://codereview.chromium.org/1171173002/diff/200001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.h:17: 
On 2015/06/12 21:08:15, jkarlin wrote:
> Document the class. Especially, what thread does it live on?

Done.

[jkarlin, 2015-06-16 19:00:00]

Only had a chance to review the event dispatcher

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
File content/browser/service_worker/service_worker_mojo_event_dispatcher.cc
(right):

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:36:
ServiceWorkerMojoEventDispatcher::ServiceWorkerMojoEventDispatcher() {
DCHECK_CURRENTLY_ON(BrowserThread::IO);

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:56: this,
thread_id, callback)));
nit: prefer you return here and remove the else {} below.

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:71: void
ServiceWorkerMojoEventDispatcher::Connect(
This should be called ConnectOnUIThread and should either be static or an
unnamed-namespace function.

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:76:
RenderProcessHost* host = RenderProcessHost::FromID(render_process_id);
There is no guarantee that the render process still exists at this point, check
the pointer.

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:82:
registry->ConnectToRemoteService(mojo::GetProxy(&background_sync_client));
ConnectToRemoveService isn't documented, but appears to have weird behavior if
the remove provider isn't yet connected. Do you need to check for null on what's
returned?

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:94: if
(!background_sync_client_.get()) {
DCHECK(background_sync_client_.get())?

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:107:
content::SyncRegistrationPtr TEMP_null_event(
tmp_null_event, lowercase

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
File content/browser/service_worker/service_worker_mojo_event_dispatcher.h
(right):

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.h:22: //
separate thread.
The above paragraph is out of date. Just mention that this class should be
called on the IO thread.

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.h:24: :
public base::RefCountedThreadSafe<ServiceWorkerMojoEventDispatcher,
This doesn't need to be RefCounted any longer. Instead, add a WeakPtrFactory
member and use the weak ptr for your posts.

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.h:36:
template <typename MojoServicePtrType>
The typename differs between here and source file. Is it MojoServicePtrType or
MojoServiceType?

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.h:51: friend
class base::DeleteHelper<ServiceWorkerMojoEventDispatcher>;
delete the above friends

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.h:58:
BackgroundSyncServiceClientPtr background_sync_client_;
DISALLOW_COPY_AND_ASSIGN(ServiceWorkerMojoEventDispatcher);

[iclelland, 2015-06-16 22:53:13]

Patchset #14 (id:280001) has been deleted

[iclelland, 2015-06-17 12:39:27]

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
File content/browser/service_worker/service_worker_mojo_event_dispatcher.cc
(right):

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:36:
ServiceWorkerMojoEventDispatcher::ServiceWorkerMojoEventDispatcher() {
On 2015/06/16 18:59:59, jkarlin wrote:
> DCHECK_CURRENTLY_ON(BrowserThread::IO);

Done.

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:56: this,
thread_id, callback)));
On 2015/06/16 19:00:00, jkarlin wrote:
> nit: prefer you return here and remove the else {} below.

Done.

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:71: void
ServiceWorkerMojoEventDispatcher::Connect(
On 2015/06/16 19:00:00, jkarlin wrote:
> This should be called ConnectOnUIThread and should either be static or an
> unnamed-namespace function. 

Done.

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:76:
RenderProcessHost* host = RenderProcessHost::FromID(render_process_id);
On 2015/06/16 19:00:00, jkarlin wrote:
> There is no guarantee that the render process still exists at this point,
check
> the pointer.

Done.

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:82:
registry->ConnectToRemoteService(mojo::GetProxy(&background_sync_client));
On 2015/06/16 19:00:00, jkarlin wrote:
> ConnectToRemoveService isn't documented, but appears to have weird behavior if
> the remove provider isn't yet connected. Do you need to check for null on
what's
> returned?

Yes; see cleanup patch. My understanding (from
content/common/mojo/service_registry_impl.cc and
third_party/mojo/src/mojo/public/cpp/system/handle.h) is that the InterfacePtr
will not be valid if the connection fails, and I'm checking for that in the
callback.

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:94: if
(!background_sync_client_.get()) {
On 2015/06/16 18:59:59, jkarlin wrote:
> DCHECK(background_sync_client_.get())?

No -- this is checking whether the interface has been bound yet or not. If it
has not been bound, then we check whether a valid info struct was passed in from
Connect, and connect in that case. (See cleanup thread)

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:107:
content::SyncRegistrationPtr TEMP_null_event(
On 2015/06/16 19:00:00, jkarlin wrote:
> tmp_null_event, lowercase

I didn't expect this identifier to last too long, but yeah. I've removed the
TEMP_, and let the TODO stand for the fact that this is a temporary situation.
Done.

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
File content/browser/service_worker/service_worker_mojo_event_dispatcher.h
(right):

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.h:22: //
separate thread.
On 2015/06/16 19:00:00, jkarlin wrote:
> The above paragraph is out of date. Just mention that this class should be
> called on the IO thread.

Done.

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.h:24: :
public base::RefCountedThreadSafe<ServiceWorkerMojoEventDispatcher,
On 2015/06/16 19:00:00, jkarlin wrote:
> This doesn't need to be RefCounted any longer. Instead, add a WeakPtrFactory
> member and use the weak ptr for your posts. 

Done.

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.h:36:
template <typename MojoServicePtrType>
On 2015/06/16 19:00:00, jkarlin wrote:
> The typename differs between here and source file. Is it MojoServicePtrType or
> MojoServiceType?

Thanks -- I didn't catch that. Should be MojoServiceType (I call out
InterfacePtr<MojoServiceType> when necessary)

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.h:51: friend
class base::DeleteHelper<ServiceWorkerMojoEventDispatcher>;
On 2015/06/16 19:00:00, jkarlin wrote:
> delete the above friends

Done.

https://codereview.chromium.org/1171173002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.h:58:
BackgroundSyncServiceClientPtr background_sync_client_;
On 2015/06/16 19:00:00, jkarlin wrote:
> DISALLOW_COPY_AND_ASSIGN(ServiceWorkerMojoEventDispatcher);

Done.

[jkarlin, 2015-06-17 15:17:30]

Still focused on the Dispatcher and ServiceWorkerVersion.

https://codereview.chromium.org/1171173002/diff/300001/content/browser/servic...
File content/browser/service_worker/service_worker_mojo_event_dispatcher.cc
(right):

https://codereview.chromium.org/1171173002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:86:
BrowserThread::PostTask(
This is much better, thanks! One more suggestion. You can have ConnectOnUIThread
return the MojoInterfacePtr instead of dealing with a callback, like so:

  BrowserThread::PostTaskAndReplyWithResult(
      BrowserThread::UI,
      FROM_HERE,
      base::Bind(&ConnectOnUIThread<BackgroundSyncServiceClient>,
                 render_process_id),
     
base::Bind(&ServiceWorkerMojoEventDispatcher::BindChannelAndDispatchSyncEvent,
                 weak_ptr_factory_.GetWeakPtr(),
                 thread_id, callback));

Not sure why the formatting above gets messed up, but you get the idea.

https://codereview.chromium.org/1171173002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:126:
base::Bind(&ServiceWorkerMojoEventDispatcher::OnEventFinished,
Why not just pass the callback here? What purpose does OnEventFinished serve?

https://codereview.chromium.org/1171173002/diff/300001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (left):

https://codereview.chromium.org/1171173002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1105:
SERVICE_WORKER_ERROR_FAILED);
We still need this (or an equivalent) to stop the sync if the SW dies.

https://codereview.chromium.org/1171173002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1906:
!sync_callbacks_.IsEmpty() ||
We can't just get rid of this. HasInflightRequests is used to determine when the
SW can be safely shut down.

We're going to need keep sync_callbacks_ (or some equivalent). It's used here
and it's used to run callbacks when the SW is stopped.

https://codereview.chromium.org/1171173002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1988: return
RunIDMapCallback(&sync_callbacks_, info.id,
We still need this (or an equivalent) to time out our requests after 5 minutes.

https://codereview.chromium.org/1171173002/diff/300001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (left):

https://codereview.chromium.org/1171173002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:526:
IDMap<StatusCallback, IDMapOwnPointer> sync_callbacks_;
You're going to need to keep this, for the reasons pointed out in the source
file.

https://codereview.chromium.org/1171173002/diff/300001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1171173002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:575:
scoped_ptr<ServiceWorkerMojoEventDispatcher> mojo_event_dispatcher_;
Why a scoped_ptr? This could just be a member.

https://codereview.chromium.org/1171173002/diff/300001/content/public/common/...
File content/public/common/service_worker_event_status.mojom (right):

https://codereview.chromium.org/1171173002/diff/300001/content/public/common/...
content/public/common/service_worker_event_status.mojom:10: ABORT,
ABORTED?

[jkarlin, 2015-06-18 15:41:35]

A few nits

https://codereview.chromium.org/1171173002/diff/300001/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.cc (right):

https://codereview.chromium.org/1171173002/diff/300001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:53: void
BackgroundSyncClientImpl::DispatchSyncForWorker(
DispatchSyncOnWorkerThread

https://codereview.chromium.org/1171173002/diff/300001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:55: 
extra line here? Please run git cl format. In fact, please add a PRESUBMIT.py to
this directory that requires clang formatting. See
content/browser/background_sync/PRESUBMIT.py as an example.

https://codereview.chromium.org/1171173002/diff/300001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:71: void
BackgroundSyncClientImpl::OnSyncComplete(
OnSyncCompleteOnWorkerThread

https://codereview.chromium.org/1171173002/diff/300001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/1171173002/diff/300001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.h:54: using
SyncCallback = mojo::Callback<void(ServiceWorkerEventStatus)>;
put this in the class instead of the content namespace?

[iclelland, 2015-06-18 16:00:02]

Patchset #16 (id:340001) has been deleted

[iclelland, 2015-06-18 16:01:15]

https://codereview.chromium.org/1171173002/diff/300001/content/browser/servic...
File content/browser/service_worker/service_worker_mojo_event_dispatcher.cc
(right):

https://codereview.chromium.org/1171173002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:86:
BrowserThread::PostTask(
On 2015/06/17 15:17:29, jkarlin wrote:
> This is much better, thanks! One more suggestion. You can have
ConnectOnUIThread
> return the MojoInterfacePtr instead of dealing with a callback, like so:
> 
>   BrowserThread::PostTaskAndReplyWithResult(
>       BrowserThread::UI,
>       FROM_HERE,
>       base::Bind(&ConnectOnUIThread<BackgroundSyncServiceClient>,
>                  render_process_id),
>      
> base::Bind(&ServiceWorkerMojoEventDispatcher::BindChannelAndDispatchSyncEvent,
>                  weak_ptr_factory_.GetWeakPtr(),
>                  thread_id, callback));
> 
> Not sure why the formatting above gets messed up, but you get the idea.

That's a good API -- thanks for that pointer!

https://codereview.chromium.org/1171173002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:126:
base::Bind(&ServiceWorkerMojoEventDispatcher::OnEventFinished,
On 2015/06/17 15:17:29, jkarlin wrote:
> Why not just pass the callback here? What purpose does OnEventFinished serve?

The only thing it does currently is convert the callback result (a mojo
ServiceWorkerEventStatus) into something useful for the browser process (a
content::ServiceWorkerStatusCode). I haven't found a way to make the two types
identical, so I've added this to perform the conversion.

If we move the callback accounting into this class, then this will also be where
that cleanup happens. (But that's for the future)

https://codereview.chromium.org/1171173002/diff/300001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (left):

https://codereview.chromium.org/1171173002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1105:
SERVICE_WORKER_ERROR_FAILED);
On 2015/06/17 15:17:29, jkarlin wrote:
> We still need this (or an equivalent) to stop the sync if the SW dies.

Done.

https://codereview.chromium.org/1171173002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1906:
!sync_callbacks_.IsEmpty() ||
On 2015/06/17 15:17:29, jkarlin wrote:
> We can't just get rid of this. HasInflightRequests is used to determine when
the
> SW can be safely shut down.
> 
> We're going to need keep sync_callbacks_ (or some equivalent). It's used here
> and it's used to run callbacks when the SW is stopped.

Done.

https://codereview.chromium.org/1171173002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1988: return
RunIDMapCallback(&sync_callbacks_, info.id,
On 2015/06/17 15:17:29, jkarlin wrote:
> We still need this (or an equivalent) to time out our requests after 5
minutes.

Done.

https://codereview.chromium.org/1171173002/diff/300001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (left):

https://codereview.chromium.org/1171173002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:526:
IDMap<StatusCallback, IDMapOwnPointer> sync_callbacks_;
On 2015/06/17 15:17:29, jkarlin wrote:
> You're going to need to keep this, for the reasons pointed out in the source
> file.

As we discussed, I've reinstated all of the callback accounting in this class
for sync events. (Applies to all sync_callbacks_-related comments)

https://codereview.chromium.org/1171173002/diff/300001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1171173002/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:575:
scoped_ptr<ServiceWorkerMojoEventDispatcher> mojo_event_dispatcher_;
On 2015/06/17 15:17:29, jkarlin wrote:
> Why a scoped_ptr? This could just be a member.

No good reason, I suppose; I've untangled the #include dependencies to make this
a member now.
Done.

https://codereview.chromium.org/1171173002/diff/300001/content/public/common/...
File content/public/common/service_worker_event_status.mojom (right):

https://codereview.chromium.org/1171173002/diff/300001/content/public/common/...
content/public/common/service_worker_event_status.mojom:10: ABORT,
On 2015/06/17 15:17:29, jkarlin wrote:
> ABORTED?

Done.

https://codereview.chromium.org/1171173002/diff/300001/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.cc (right):

https://codereview.chromium.org/1171173002/diff/300001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:53: void
BackgroundSyncClientImpl::DispatchSyncForWorker(
On 2015/06/18 15:41:35, jkarlin wrote:
> DispatchSyncOnWorkerThread

Done.

https://codereview.chromium.org/1171173002/diff/300001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:55: 
On 2015/06/18 15:41:35, jkarlin wrote:
> extra line here? Please run git cl format. In fact, please add a PRESUBMIT.py
to
> this directory that requires clang formatting. See
> content/browser/background_sync/PRESUBMIT.py as an example.

Done.

https://codereview.chromium.org/1171173002/diff/300001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:71: void
BackgroundSyncClientImpl::OnSyncComplete(
On 2015/06/18 15:41:35, jkarlin wrote:
> OnSyncCompleteOnWorkerThread

Done.

https://codereview.chromium.org/1171173002/diff/300001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/1171173002/diff/300001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.h:54: using
SyncCallback = mojo::Callback<void(ServiceWorkerEventStatus)>;
On 2015/06/18 15:41:35, jkarlin wrote:
> put this in the class instead of the content namespace?

Done. Also addressed analogous situation in ServiceWorkerMojoEventDispatcher.

[jkarlin, 2015-06-18 18:13:57]

Awesome. lgtm with changes.

https://codereview.chromium.org/1171173002/diff/330018/content/browser/servic...
File content/browser/service_worker/service_worker_mojo_event_dispatcher.cc
(right):

https://codereview.chromium.org/1171173002/diff/330018/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:20:
ServiceWorkerStatusCode statusCodeFromMojoStatus(
caps, StatusCodeFromMojoStatus

https://codereview.chromium.org/1171173002/diff/330018/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:35: //
Establishes a connection to a mojo service, and then detaches the local
"detaches the local endpoint of the message pipe from the UI thread". I don't
see any explicit code that does that. Seems like you could omit that text.

https://codereview.chromium.org/1171173002/diff/330018/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1171173002/diff/330018/content/browser/servic...
content/browser/service_worker/service_worker_version.h:25: #include
"content/common/background_sync_service.mojom.h"
I don't think you need this one any longer.

https://codereview.chromium.org/1171173002/diff/330018/content/common/backgro...
File content/common/background_sync_service.mojom (right):

https://codereview.chromium.org/1171173002/diff/330018/content/common/backgro...
content/common/background_sync_service.mojom:36: Sync(SyncRegistration event,
int32 thread_id)
perhaps rename event to registration? Event isn't very clear.

https://codereview.chromium.org/1171173002/diff/330018/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.cc (right):

https://codereview.chromium.org/1171173002/diff/330018/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:38:
DCHECK(main_thread_task_runner_->RunsTasksOnCurrentThread());
DCHECK should be first line, ideally with a newline after it to separate it from
the implementation.

https://codereview.chromium.org/1171173002/diff/330018/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:45: // abort
status.
PostTask will return false on a deleted task runner. So check the return of the
PostTask() to resolve the above TODO.

https://codereview.chromium.org/1171173002/diff/330018/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:78:
base::Bind(&BackgroundSyncClientImpl::OnSyncCompleteOnMainThread,
base::Bind(callback, status) and then delete OnSyncCompleteOnMainThread

https://codereview.chromium.org/1171173002/diff/330018/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1171173002/diff/330018/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:205:
SyncEventCallbacksMap sync_event_callbacks_;
no trailing _ in struct member names.

https://codereview.chromium.org/1171173002/diff/330018/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:758: void
ServiceWorkerContextClient::OnSyncEvent(int request_id) {
I believe you need to remove this.

https://codereview.chromium.org/1171173002/diff/330018/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/1171173002/diff/330018/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.h:54: using
SyncCallback = mojo::Callback<void(ServiceWorkerEventStatus)>;
Move this into the class.

https://codereview.chromium.org/1171173002/diff/330018/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.h:150: virtual
void DispatchSyncEvent(const SyncCallback& callbacks);
callback singular right? Applies to member variables in source file too

[iclelland, 2015-06-19 13:44:32]

https://codereview.chromium.org/1171173002/diff/330018/content/browser/servic...
File content/browser/service_worker/service_worker_mojo_event_dispatcher.cc
(right):

https://codereview.chromium.org/1171173002/diff/330018/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:20:
ServiceWorkerStatusCode statusCodeFromMojoStatus(
On 2015/06/18 18:13:56, jkarlin wrote:
> caps, StatusCodeFromMojoStatus

oldHabitsDieHard :)

Done.

https://codereview.chromium.org/1171173002/diff/330018/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:35: //
Establishes a connection to a mojo service, and then detaches the local
On 2015/06/18 18:13:56, jkarlin wrote:
> "detaches the local endpoint of the message pipe from the UI thread". I don't
> see any explicit code that does that. Seems like you could omit that text.

It's actually what InterfacePtr<>::PassInterface is for:
  // Unbinds the InterfacePtr and returns the information which could be used
  // to setup an InterfacePtr again. This method may be used to move the proxy
  // to a different thread

I'll try to reword it to use "unbind" in place of "detach".

https://codereview.chromium.org/1171173002/diff/330018/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1171173002/diff/330018/content/browser/servic...
content/browser/service_worker/service_worker_version.h:25: #include
"content/common/background_sync_service.mojom.h"
On 2015/06/18 18:13:56, jkarlin wrote:
> I don't think you need this one any longer.

You're right; thanks for catching it.

https://codereview.chromium.org/1171173002/diff/330018/content/common/backgro...
File content/common/background_sync_service.mojom (right):

https://codereview.chromium.org/1171173002/diff/330018/content/common/backgro...
content/common/background_sync_service.mojom:36: Sync(SyncRegistration event,
int32 thread_id)
On 2015/06/18 18:13:56, jkarlin wrote:
> perhaps rename event to registration? Event isn't very clear.

Yes -- I thought I'd fixed all of those; apparently just in the implementation
side, though.
Done.

https://codereview.chromium.org/1171173002/diff/330018/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.cc (right):

https://codereview.chromium.org/1171173002/diff/330018/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:38:
DCHECK(main_thread_task_runner_->RunsTasksOnCurrentThread());
On 2015/06/18 18:13:56, jkarlin wrote:
> DCHECK should be first line, ideally with a newline after it to separate it
from
> the implementation.

Done.

https://codereview.chromium.org/1171173002/diff/330018/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:45: // abort
status.
On 2015/06/18 18:13:56, jkarlin wrote:
> PostTask will return false on a deleted task runner. So check the return of
the
> PostTask() to resolve the above TODO.

Done. Thanks.

https://codereview.chromium.org/1171173002/diff/330018/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:78:
base::Bind(&BackgroundSyncClientImpl::OnSyncCompleteOnMainThread,
On 2015/06/18 18:13:57, jkarlin wrote:
> base::Bind(callback, status) and then delete OnSyncCompleteOnMainThread

This doesn't appear to be possible; callback is a mojo::Callback, not a
base::Callback :(

https://codereview.chromium.org/1171173002/diff/330018/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1171173002/diff/330018/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:205:
SyncEventCallbacksMap sync_event_callbacks_;
On 2015/06/18 18:13:57, jkarlin wrote:
> no trailing _ in struct member names.

Done.

https://codereview.chromium.org/1171173002/diff/330018/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:758: void
ServiceWorkerContextClient::OnSyncEvent(int request_id) {
Done.

https://codereview.chromium.org/1171173002/diff/330018/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/1171173002/diff/330018/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.h:54: using
SyncCallback = mojo::Callback<void(ServiceWorkerEventStatus)>;
On 2015/06/18 18:13:57, jkarlin wrote:
> Move this into the class.

Done.

https://codereview.chromium.org/1171173002/diff/330018/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.h:150: virtual
void DispatchSyncEvent(const SyncCallback& callbacks);
On 2015/06/18 18:13:57, jkarlin wrote:
> callback singular right? Applies to member variables in source file too
Done.

[jkarlin, 2015-06-19 13:51:42]

slgtm!

https://codereview.chromium.org/1171173002/diff/330018/content/browser/servic...
File content/browser/service_worker/service_worker_mojo_event_dispatcher.cc
(right):

https://codereview.chromium.org/1171173002/diff/330018/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:35: //
Establishes a connection to a mojo service, and then detaches the local
On 2015/06/19 13:44:32, iclelland wrote:
> On 2015/06/18 18:13:56, jkarlin wrote:
> > "detaches the local endpoint of the message pipe from the UI thread". I
don't
> > see any explicit code that does that. Seems like you could omit that text.
> 
> It's actually what InterfacePtr<>::PassInterface is for:
>   // Unbinds the InterfacePtr and returns the information which could be used
>   // to setup an InterfacePtr again. This method may be used to move the proxy
>   // to a different thread
> 
> I'll try to reword it to use "unbind" in place of "detach".

Got it, thanks.

https://codereview.chromium.org/1171173002/diff/370001/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.cc (right):

https://codereview.chromium.org/1171173002/diff/370001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:44: if
(!worker_task_runner->GetTaskRunnerFor(thread_id)->PostTask(
Should be combined with the above conditional.

[iclelland, 2015-06-19 17:56:10]

https://codereview.chromium.org/1171173002/diff/370001/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.cc (right):

https://codereview.chromium.org/1171173002/diff/370001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.cc:44: if
(!worker_task_runner->GetTaskRunnerFor(thread_id)->PostTask(
On 2015/06/19 13:51:42, jkarlin wrote:
> Should be combined with the above conditional.

Done.

[iclelland, 2015-06-22 17:32:48]

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

[iclelland, 2015-06-22 17:46:32]

+reviewers

michaeln - I think you last looked at the sync event code, with chasej's patches
-- can you take a look at this too?
tsepez - Can you look at the mojo code here; there's a new service running in
the render that the browser is going to connect to, to fire events.
jochen - I need your signoff for the build files, and for the addition of the
service in render_thread_impl.cc, but please take a look at the rest for sanity
:)

There are two failing unit tests currently -- the recently-added
ServiceWorkerVersionBrowserTest.SyncEventWaitUntil_Fulfilled and
ServiceWorkerVersionBrowserTest.SyncEventWaitUntil_Rejected. As far as I can
tell, these are failing because the test code is tearing everything down before
the callbacks happen; in a running browser, everything is working correctly. I'm
still working on fixing that last issue before this is ready to commit, but I'd
appreciate the code review in the meantime.

Thanks!

[jochen (gone - plz use gerrit), 2015-06-23 14:56:03]

https://codereview.chromium.org/1171173002/diff/410001/content/renderer/backg...
File content/renderer/background_sync/PRESUBMIT.py (right):

https://codereview.chromium.org/1171173002/diff/410001/content/renderer/backg...
content/renderer/background_sync/PRESUBMIT.py:2: # Use of this source code is
governed by a BSD-style license that can be
why did you add this?

[jkarlin, 2015-06-23 14:59:54]

https://codereview.chromium.org/1171173002/diff/410001/content/renderer/backg...
File content/renderer/background_sync/PRESUBMIT.py (right):

https://codereview.chromium.org/1171173002/diff/410001/content/renderer/backg...
content/renderer/background_sync/PRESUBMIT.py:2: # Use of this source code is
governed by a BSD-style license that can be
On 2015/06/23 14:56:02, jochen wrote:
> why did you add this?

I asked him to. The rest of the background sync code also checks for formatting
via presubmit.

[jkarlin, 2015-06-23 17:48:13]

https://codereview.chromium.org/1171173002/diff/410001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1171173002/diff/410001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:663: int
request_id = context_->sync_event_callbacks.Add(&callback);
This is why mojo is complaining about the callback being dropped. You're passing
&callback to something expecting a heap allocated pointer.

[iclelland, 2015-06-23 19:42:23]

Patchset #20 (id:430001) has been deleted

[iclelland, 2015-06-23 20:30:04]

https://codereview.chromium.org/1171173002/diff/410001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1171173002/diff/410001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:663: int
request_id = context_->sync_event_callbacks.Add(&callback);
On 2015/06/23 17:48:12, jkarlin wrote:
> This is why mojo is complaining about the callback being dropped. You're
passing
> &callback to something expecting a heap allocated pointer.

Thanks for spotting that! I was staring at everything on the *other* side of the
connection, and not seeing this one. I've uploaded a fix for that.

[michaeln, 2015-06-24 00:12:01]

thnx for the opportunity to learn something about mojo! here's some initial
comments but i still need to look more closely.

https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
File content/browser/service_worker/service_worker_mojo_event_dispatcher.cc
(right):

https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:35: //
Establishes a connection to a mojo service, and then unbinds the local side
(mojo noob question) Where does the 'unbinding' happen in here?

https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:50: if
(host) {
we generally prefer early returns

if (!host)
  return mojo::InterfacePtr<MojoServiceType>();

ServiceRegistry* registry = host->GetServiceRegistry();
if (!registry)
  return mojo::InterfacePtr<MojoServiceType>();

mojo::InterfacePtr<MojoServiceType> mojo_interface_ptr;
registry->ConnectToRemoteService(mojo::GetProxy(&mojo_interface_ptr));

https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:51:
content::ServiceRegistry* registry = host->GetServiceRegistry();
i don't think the content:: prefix is needed

https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:53:
registry->ConnectToRemoteService(mojo::GetProxy(&mojo_interface_ptr));
How expensive is it to 'connect' to a remote service? Just curious?

https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:71: int
render_process_id,
is this param needed? is there not one dispatcher per renderer?

https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:80:
BrowserThread::PostTaskAndReplyWithResult(
What if we get another DispatchSyncEvent() call prior to this async op being
complete?

https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:97: if
(!background_sync_client_.get()) {
I see, additional connect results are dropped here. Is it ok to have multiple
intf ptrs to the same servicetype coexist at the same time (i would think so,
but just checking)?

https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
File content/browser/service_worker/service_worker_mojo_event_dispatcher.h
(right):

https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.h:44:
base::WeakPtrFactory<ServiceWorkerMojoEventDispatcher> weak_ptr_factory_;
The name 'weak_factory_' is used consistently for this in the /service_worker
directory, for consistency it'd be good to do that here too.

https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:759:
embedded_worker_->process_id(), embedded_worker_->thread_id(),
This won't quite work because ServiceWorkerVersion instances outlive individual
running script instances. A ServiceWorkerVersion can serially start many script
instances in possibly different processes over its lifetime.

food4thought

In the serviceworker codebase, the ServiceWorkerDispatcherHost's is paired with
a particular renderer processes lifetime. Arranging for a mojo_event_dispatcher_
instance to be owned and scoped by ServiceWorkerDispatcherHost might make sense.
The ServiceWorkerContextCore class serves as a registry of sorts. It's a
centrally located class that is reachable by most things and maintains ptrs to
instances of significant other things (see the Add/Remove/GetProviderHost and
Add/Remove/GetLiveXXX methods). Maybe we can Add/Remove/GetSyncEventDispatcher
ptrs as well. Have the dispatcherhost add/rmv the instance it creates for it's
process, and have versions get it by process id as needed.

https://codereview.chromium.org/1171173002/diff/410001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1171173002/diff/410001/content/renderer/rende...
content/renderer/render_thread_impl.cc:699:
base::Bind(&content::BackgroundSyncClientImpl::Create, GetTaskRunner()));
content:: not needed

[michaeln, 2015-06-24 01:34:03]

https://codereview.chromium.org/1171173002/diff/330018/content/browser/servic...
File content/browser/service_worker/service_worker_mojo_event_dispatcher.cc
(right):

https://codereview.chromium.org/1171173002/diff/330018/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:35: //
Establishes a connection to a mojo service, and then detaches the local
On 2015/06/19 13:44:32, iclelland wrote:
> On 2015/06/18 18:13:56, jkarlin wrote:
> > "detaches the local endpoint of the message pipe from the UI thread". I
don't
> > see any explicit code that does that. Seems like you could omit that text.
> 
> It's actually what InterfacePtr<>::PassInterface is for:
>   // Unbinds the InterfacePtr and returns the information which could be used
>   // to setup an InterfacePtr again. This method may be used to move the proxy
>   // to a different thread
> 
> I'll try to reword it to use "unbind" in place of "detach".

ah... now i see where that occurs :)

[michaeln, 2015-06-24 02:02:05]

https://codereview.chromium.org/1171173002/diff/410001/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.h (right):

https://codereview.chromium.org/1171173002/diff/410001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.h:19: // Return
messages from the workers are rerouted to the main thread in order to
Routing everything, coming and going, thru the main renderer thread is very
unfortunate? The msg src/dst thread is always a worker thread.

I'd like to explore ways to eliminate the trips thru the main thread. It'd be
good to get an example of mojo usage that avoids those extra context switches
into the codebase because whatever pattern is used here will inevitably be
copied.

The technique we use for non-mojo based messaging involves filtering (with a
BrowserMessageFilter) IPCs on the IPC thread and then posting a task straight to
the destination thread. And using the ThreadSafeSender class to send messages
directly from background threads (which internally posts tasks to the IPC
thread).

I don't know enuf about mojo (yet) to know how to get that same runtime behavior
wrt thread switching, but peeling off interface ptrs for use on other threads
looks like it could be applicable?

[iclelland, 2015-06-24 14:29:30]

Thanks for looking at all this, Michael! I'm still looking at your food4thought
comment; but the other style issues are addressed now.

https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
File content/browser/service_worker/service_worker_mojo_event_dispatcher.cc
(right):

https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:35: //
Establishes a connection to a mojo service, and then unbinds the local side
On 2015/06/24 00:12:00, michaeln wrote:
> (mojo noob question) Where does the 'unbinding' happen in here?

By calling .PassInterface(), and allowing the InterfacePtr<> to go out of scope,
the original connection is torn down, but we retain all of the information
required to reconstruct the connection on a new thread, in the form of an
InterfacePtrInfo<> struct.

https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:71: int
render_process_id,
On 2015/06/24 00:12:00, michaeln wrote:
> is this param needed? is there not one dispatcher per renderer?

Yes, there should be one dispatcher per renderer (assuming that the embedded
worker in the ServiceWorkerVersion never changes), so this param shouldn't ever
change. We need to get the pid somewhere, though, so that we can look up the
RenderProcessHost. We can't do it at ServiceWorkerVersion construction time, as
the worker won't be running yet.

I'm taking a closer look at your comments on service_worker_version.cc to see if
there's another design that makes more sense.

https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:80:
BrowserThread::PostTaskAndReplyWithResult(
On 2015/06/24 00:12:00, michaeln wrote:
> What if we get another DispatchSyncEvent() call prior to this async op being
> complete?

In that case, the second connection will be dropped in the
BindChannelAndDispatchSyncEvent method below. (But you already saw that)

https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:97: if
(!background_sync_client_.get()) {
On 2015/06/24 00:12:01, michaeln wrote:
> I see, additional connect results are dropped here. Is it ok to have multiple
> intf ptrs to the same servicetype coexist at the same time (i would think so,
> but just checking)?

Yes, that should be a safe pattern; it's a little bit of wasted effort; we could
probably use some atomic condition variable to avoid it running Connect twice if
we have to, but I'd rather do the simpler thing until it becomes an obvious
performance problem.

https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
File content/browser/service_worker/service_worker_mojo_event_dispatcher.h
(right):

https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.h:44:
base::WeakPtrFactory<ServiceWorkerMojoEventDispatcher> weak_ptr_factory_;
On 2015/06/24 00:12:01, michaeln wrote:
> The name 'weak_factory_' is used consistently for this in the /service_worker
> directory, for consistency it'd be good to do that here too.

Done.

https://codereview.chromium.org/1171173002/diff/410001/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.h (right):

https://codereview.chromium.org/1171173002/diff/410001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.h:19: // Return
messages from the workers are rerouted to the main thread in order to
On 2015/06/24 02:02:05, michaeln wrote:
> Routing everything, coming and going, thru the main renderer thread is very
> unfortunate?

It is, and I'd like to get rid of it

> The msg src/dst thread is always a worker thread.
> 
> I'd like to explore ways to eliminate the trips thru the main thread. It'd be
> good to get an example of mojo usage that avoids those extra context switches
> into the codebase because whatever pattern is used here will inevitably be
> copied.
> 
> The technique we use for non-mojo based messaging involves filtering (with a
> BrowserMessageFilter) IPCs on the IPC thread and then posting a task straight
to
> the destination thread. And using the ThreadSafeSender class to send messages
> directly from background threads (which internally posts tasks to the IPC
> thread).
> 
> I don't know enuf about mojo (yet) to know how to get that same runtime
behavior
> wrt thread switching, but peeling off interface ptrs for use on other threads
> looks like it could be applicable?

That would work, as long as we can have a connection to each worker thread -- I
need to look closer at the relationship between renderer process and worker
threads to see exactly how to do it. If there are multiple service workers on a
renderer, then we'll need to maintain a connection to each, and look them up by
thread_id (or some other id).

https://codereview.chromium.org/1171173002/diff/410001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1171173002/diff/410001/content/renderer/rende...
content/renderer/render_thread_impl.cc:699:
base::Bind(&content::BackgroundSyncClientImpl::Create, GetTaskRunner()));
On 2015/06/24 00:12:01, michaeln wrote:
> content:: not needed

Done.

[jkarlin, 2015-06-24 14:38:09]

https://codereview.chromium.org/1171173002/diff/410001/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.h (right):

https://codereview.chromium.org/1171173002/diff/410001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.h:19: // Return
messages from the workers are rerouted to the main thread in order to
On 2015/06/24 14:29:30, iclelland wrote:
> On 2015/06/24 02:02:05, michaeln wrote:
> > Routing everything, coming and going, thru the main renderer thread is very
> > unfortunate?
> 
> It is, and I'd like to get rid of it
> 
> > The msg src/dst thread is always a worker thread.
> > 
> > I'd like to explore ways to eliminate the trips thru the main thread. It'd
be
> > good to get an example of mojo usage that avoids those extra context
switches
> > into the codebase because whatever pattern is used here will inevitably be
> > copied.
> > 
> > The technique we use for non-mojo based messaging involves filtering (with a
> > BrowserMessageFilter) IPCs on the IPC thread and then posting a task
straight
> to
> > the destination thread. And using the ThreadSafeSender class to send
messages
> > directly from background threads (which internally posts tasks to the IPC
> > thread).
> > 
> > I don't know enuf about mojo (yet) to know how to get that same runtime
> behavior
> > wrt thread switching, but peeling off interface ptrs for use on other
threads
> > looks like it could be applicable?
> 
> That would work, as long as we can have a connection to each worker thread --
I
> need to look closer at the relationship between renderer process and worker
> threads to see exactly how to do it. If there are multiple service workers on
a
> renderer, then we'll need to maintain a connection to each, and look them up
by
> thread_id (or some other id).

Not sure I'm entirely following this. I think you guys are suggesting that we
move the mojo::InterfaceRequest to the appropriate thread before binding, right?
If so, that sgtm.

[iclelland, 2015-06-24 14:42:05]

https://codereview.chromium.org/1171173002/diff/410001/content/renderer/backg...
File content/renderer/background_sync/background_sync_client_impl.h (right):

https://codereview.chromium.org/1171173002/diff/410001/content/renderer/backg...
content/renderer/background_sync/background_sync_client_impl.h:19: // Return
messages from the workers are rerouted to the main thread in order to
On 2015/06/24 14:38:09, jkarlin wrote:
> On 2015/06/24 14:29:30, iclelland wrote:
> > On 2015/06/24 02:02:05, michaeln wrote:
> > > Routing everything, coming and going, thru the main renderer thread is
very
> > > unfortunate?
> > 
> > It is, and I'd like to get rid of it
> > 
> > > The msg src/dst thread is always a worker thread.
> > > 
> > > I'd like to explore ways to eliminate the trips thru the main thread. It'd
> be
> > > good to get an example of mojo usage that avoids those extra context
> switches
> > > into the codebase because whatever pattern is used here will inevitably be
> > > copied.
> > > 
> > > The technique we use for non-mojo based messaging involves filtering (with
a
> > > BrowserMessageFilter) IPCs on the IPC thread and then posting a task
> straight
> > to
> > > the destination thread. And using the ThreadSafeSender class to send
> messages
> > > directly from background threads (which internally posts tasks to the IPC
> > > thread).
> > > 
> > > I don't know enuf about mojo (yet) to know how to get that same runtime
> > behavior
> > > wrt thread switching, but peeling off interface ptrs for use on other
> threads
> > > looks like it could be applicable?
> > 
> > That would work, as long as we can have a connection to each worker thread
--
> I
> > need to look closer at the relationship between renderer process and worker
> > threads to see exactly how to do it. If there are multiple service workers
on
> a
> > renderer, then we'll need to maintain a connection to each, and look them up
> by
> > thread_id (or some other id).
> 
> Not sure I'm entirely following this. I think you guys are suggesting that we
> move the mojo::InterfaceRequest to the appropriate thread before binding,
right?
> If so, that sgtm. 

Right -- so in this case, we're moving *both* endpoints of the original
connection -- in the browser, we move it from UI to IO, and in the renderer,
from the main thread to the worker. Then the browser IO thread can communicate
directly with the worker.

[jkarlin, 2015-06-24 14:44:45]

On 2015/06/24 14:42:05, iclelland wrote:
>
https://codereview.chromium.org/1171173002/diff/410001/content/renderer/backg...
> File content/renderer/background_sync/background_sync_client_impl.h (right):
> 
>
https://codereview.chromium.org/1171173002/diff/410001/content/renderer/backg...
> content/renderer/background_sync/background_sync_client_impl.h:19: // Return
> messages from the workers are rerouted to the main thread in order to
> On 2015/06/24 14:38:09, jkarlin wrote:
> > On 2015/06/24 14:29:30, iclelland wrote:
> > > On 2015/06/24 02:02:05, michaeln wrote:
> > > > Routing everything, coming and going, thru the main renderer thread is
> very
> > > > unfortunate?
> > > 
> > > It is, and I'd like to get rid of it
> > > 
> > > > The msg src/dst thread is always a worker thread.
> > > > 
> > > > I'd like to explore ways to eliminate the trips thru the main thread.
It'd
> > be
> > > > good to get an example of mojo usage that avoids those extra context
> > switches
> > > > into the codebase because whatever pattern is used here will inevitably
be
> > > > copied.
> > > > 
> > > > The technique we use for non-mojo based messaging involves filtering
(with
> a
> > > > BrowserMessageFilter) IPCs on the IPC thread and then posting a task
> > straight
> > > to
> > > > the destination thread. And using the ThreadSafeSender class to send
> > messages
> > > > directly from background threads (which internally posts tasks to the
IPC
> > > > thread).
> > > > 
> > > > I don't know enuf about mojo (yet) to know how to get that same runtime
> > > behavior
> > > > wrt thread switching, but peeling off interface ptrs for use on other
> > threads
> > > > looks like it could be applicable?
> > > 
> > > That would work, as long as we can have a connection to each worker thread
> --
> > I
> > > need to look closer at the relationship between renderer process and
worker
> > > threads to see exactly how to do it. If there are multiple service workers
> on
> > a
> > > renderer, then we'll need to maintain a connection to each, and look them
up
> > by
> > > thread_id (or some other id).
> > 
> > Not sure I'm entirely following this. I think you guys are suggesting that
we
> > move the mojo::InterfaceRequest to the appropriate thread before binding,
> right?
> > If so, that sgtm. 
> 
> Right -- so in this case, we're moving *both* endpoints of the original
> connection -- in the browser, we move it from UI to IO, and in the renderer,
> from the main thread to the worker. Then the browser IO thread can communicate
> directly with the worker.

SGTM

[jochen (gone - plz use gerrit), 2015-06-24 14:58:53]

my stuff lgtm

[michaeln, 2015-06-24 19:17:28]

On 2015/06/24 14:42:05, iclelland wrote:
>
https://codereview.chromium.org/1171173002/diff/410001/content/renderer/backg...
> File content/renderer/background_sync/background_sync_client_impl.h (right):
> 
>
https://codereview.chromium.org/1171173002/diff/410001/content/renderer/backg...
> content/renderer/background_sync/background_sync_client_impl.h:19: // Return
> messages from the workers are rerouted to the main thread in order to
> On 2015/06/24 14:38:09, jkarlin wrote:
> > On 2015/06/24 14:29:30, iclelland wrote:
> > > On 2015/06/24 02:02:05, michaeln wrote:
> > > > Routing everything, coming and going, thru the main renderer thread is
> very
> > > > unfortunate?
> > > 
> > > It is, and I'd like to get rid of it
> > > 
> > > > The msg src/dst thread is always a worker thread.
> > > > 
> > > > I'd like to explore ways to eliminate the trips thru the main thread.
It'd
> > be
> > > > good to get an example of mojo usage that avoids those extra context
> > switches
> > > > into the codebase because whatever pattern is used here will inevitably
be
> > > > copied.
> > > > 
> > > > The technique we use for non-mojo based messaging involves filtering
(with
> a
> > > > BrowserMessageFilter) IPCs on the IPC thread and then posting a task
> > straight
> > > to
> > > > the destination thread. And using the ThreadSafeSender class to send
> > messages
> > > > directly from background threads (which internally posts tasks to the
IPC
> > > > thread).
> > > > 
> > > > I don't know enuf about mojo (yet) to know how to get that same runtime
> > > behavior
> > > > wrt thread switching, but peeling off interface ptrs for use on other
> > threads
> > > > looks like it could be applicable?
> > > 
> > > That would work, as long as we can have a connection to each worker thread
> --
> > I
> > > need to look closer at the relationship between renderer process and
worker
> > > threads to see exactly how to do it. If there are multiple service workers
> on
> > a
> > > renderer, then we'll need to maintain a connection to each, and look them
up
> > by
> > > thread_id (or some other id).
> > 
> > Not sure I'm entirely following this. I think you guys are suggesting that
we
> > move the mojo::InterfaceRequest to the appropriate thread before binding,
> right?
> > If so, that sgtm. 
> 
> Right -- so in this case, we're moving *both* endpoints of the original
> connection -- in the browser, we move it from UI to IO, and in the renderer,
> from the main thread to the worker. Then the browser IO thread can communicate
> directly with the worker.

Thanx for being open to working it out!

Another option could be to have exactly one mojo client/service connection
between a renderer and the browser process, conduct the mojo messaging on the
renderer's IPC thread directly, and internally dispatch to worker threads in the
same way we do in the non-mojo case. I think that might be most analogous to how
we currently do worker thread msg routing stuff.

[iclelland, 2015-06-25 15:08:53]

Patchset #22 (id:490001) has been deleted

[iclelland, 2015-06-25 15:17:34]

https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:759:
embedded_worker_->process_id(), embedded_worker_->thread_id(),
On 2015/06/24 00:12:01, michaeln wrote:
> This won't quite work because ServiceWorkerVersion instances outlive
individual
> running script instances. A ServiceWorkerVersion can serially start many
script
> instances in possibly different processes over its lifetime.
> 
> food4thought
> 
> In the serviceworker codebase, the ServiceWorkerDispatcherHost's is paired
with
> a particular renderer processes lifetime. Arranging for a
mojo_event_dispatcher_
> instance to be owned and scoped by ServiceWorkerDispatcherHost might make
sense.
> The ServiceWorkerContextCore class serves as a registry of sorts. It's a
> centrally located class that is reachable by most things and maintains ptrs to
> instances of significant other things (see the Add/Remove/GetProviderHost and
> Add/Remove/GetLiveXXX methods). Maybe we can Add/Remove/GetSyncEventDispatcher
> ptrs as well. Have the dispatcherhost add/rmv the instance it creates for it's
> process, and have versions get it by process id as needed.

I've uploaded a new patchset that does just this -- see if it makes sense, and
actually does what you're suggesting here.

[Marijn Kruisselbrink, 2015-06-25 19:04:01]

mek@chromium.org changed reviewers:
+ mek@chromium.org

[Marijn Kruisselbrink, 2015-06-25 19:04:03]

is there a design doc somewhere for how mojo event dispatching to service
workers is supposed to work? I'm kind of interested in this as I'm trying to use
mojo for navigator.(services.)connect as well.

https://codereview.chromium.org/1171173002/diff/510001/content/browser/servic...
File content/browser/service_worker/service_worker_mojo_event_dispatcher.cc
(right):

https://codereview.chromium.org/1171173002/diff/510001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:57:
registry->ConnectToRemoteService(mojo::GetProxy(&mojo_interface_ptr));
My understanding of the "canonical" way to connect to a mojo service where you
want communication to happen on something other than the main thread is that you
call mojo::GetProxy on the thread you want communication to happen, then pass
that ptr to the UI thread and there you do the actual connection (and you can
even start using the interface before the connection is fully set up). I'm not
quite sure if that entirely applies here, but it might be something to keep in
mind if you haven't already.
(see also https://codereview.chromium.org/1192713004 where I'm doing the same
for renderer->browser communication where I set up a connection from a worker
thread in a renderer to the io thread in the browser that way)

[Marijn Kruisselbrink, 2015-06-25 20:03:16]

one more small nit that I noticed

https://codereview.chromium.org/1171173002/diff/510001/content/browser/servic...
File content/browser/service_worker/service_worker_mojo_event_dispatcher.cc
(right):

https://codereview.chromium.org/1171173002/diff/510001/content/browser/servic...
content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:20:
ServiceWorkerStatusCode StatusCodeFromMojoStatus(
this should probably be a mojo::TypeConverter and use mojo::ConvertTo<> to do
the conversion? At least that seems more typical mojo usage.

[iclelland, 2015-06-25 20:16:26]

On 2015/06/25 19:04:03, Marijn Kruisselbrink wrote:
> is there a design doc somewhere for how mojo event dispatching to service
> workers is supposed to work? I'm kind of interested in this as I'm trying to
use
> mojo for navigator.(services.)connect as well.

I've been writing it up; it's a couple weeks out-of-date at this point, so I
need to update. I'll fix it up tonight and circulate it tomorrow.

> 
>
https://codereview.chromium.org/1171173002/diff/510001/content/browser/servic...
> File content/browser/service_worker/service_worker_mojo_event_dispatcher.cc
> (right):
> 
>
https://codereview.chromium.org/1171173002/diff/510001/content/browser/servic...
> content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:57:
> registry->ConnectToRemoteService(mojo::GetProxy(&mojo_interface_ptr));
> My understanding of the "canonical" way to connect to a mojo service where you
> want communication to happen on something other than the main thread is that
you
> call mojo::GetProxy on the thread you want communication to happen, then pass
> that ptr to the UI thread and there you do the actual connection (and you can
> even start using the interface before the connection is fully set up). I'm not
> quite sure if that entirely applies here, but it might be something to keep in
> mind if you haven't already.
> (see also https://codereview.chromium.org/1192713004 where I'm doing the same
> for renderer->browser communication where I set up a connection from a worker
> thread in a renderer to the io thread in the browser that way)

I'll take a close look at that -- There may have been an issue with the
serviceregistry being only available on the UI thread, but I'll have to go back
and check that again.

[Marijn Kruisselbrink, 2015-06-25 20:20:15]

On 2015/06/25 at 20:16:26, iclelland wrote:
> On 2015/06/25 19:04:03, Marijn Kruisselbrink wrote:
> > is there a design doc somewhere for how mojo event dispatching to service
> > workers is supposed to work? I'm kind of interested in this as I'm trying to
use
> > mojo for navigator.(services.)connect as well.
> 
> I've been writing it up; it's a couple weeks out-of-date at this point, so I
need to update. I'll fix it up tonight and circulate it tomorrow.
Awesome, looking forward to it. I think we want to make sure we get this right,
since whatever the first mojo-using API is going to do is likely going to get
copied by other APIs, so it is potentially expensive to have to refactor this
later.

> 
> > 
> >
https://codereview.chromium.org/1171173002/diff/510001/content/browser/servic...
> > File content/browser/service_worker/service_worker_mojo_event_dispatcher.cc
> > (right):
> > 
> >
https://codereview.chromium.org/1171173002/diff/510001/content/browser/servic...
> > content/browser/service_worker/service_worker_mojo_event_dispatcher.cc:57:
> > registry->ConnectToRemoteService(mojo::GetProxy(&mojo_interface_ptr));
> > My understanding of the "canonical" way to connect to a mojo service where
you
> > want communication to happen on something other than the main thread is that
you
> > call mojo::GetProxy on the thread you want communication to happen, then
pass
> > that ptr to the UI thread and there you do the actual connection (and you
can
> > even start using the interface before the connection is fully set up). I'm
not
> > quite sure if that entirely applies here, but it might be something to keep
in
> > mind if you haven't already.
> > (see also https://codereview.chromium.org/1192713004 where I'm doing the
same
> > for renderer->browser communication where I set up a connection from a
worker
> > thread in a renderer to the io thread in the browser that way)
> 
> I'll take a close look at that -- There may have been an issue with the
serviceregistry being only available on the UI thread, but I'll have to go back
and check that again.

Yes, service registry is only available on the UI thread, but you can create
your proxy (and start using it) on any thread, and then asynchronously on the UI
thread actually connect that proxy to a service. All API calls etc will just get
queued until you actually set up a connection.

[michaeln, 2015-06-25 22:05:32]

> Yes, service registry is only available on the UI thread, but you can create
> your proxy (and start using it) on any thread, and then asynchronously on the
UI
> thread actually connect that proxy to a service. All API calls etc will just
get
> queued until you actually set up a connection.

In this scheme, are msgs sent from and received by the proxy routed thru UI
thread? Or after the proxy is setup and the connection establish, are incoming
and outgoing msgs exchanged directly between the IPC and backgroud
destination/origination thread w/o needing a context switch to the main thread?

We should shoot for the latter behavior.

[Marijn Kruisselbrink, 2015-06-25 22:25:47]

On 2015/06/25 at 22:05:32, michaeln wrote:
> > Yes, service registry is only available on the UI thread, but you can create
> > your proxy (and start using it) on any thread, and then asynchronously on
the UI
> > thread actually connect that proxy to a service. All API calls etc will just
get
> > queued until you actually set up a connection.
> 
> In this scheme, are msgs sent from and received by the proxy routed thru UI
thread? Or after the proxy is setup and the connection establish, are incoming
and outgoing msgs exchanged directly between the IPC and backgroud
destination/origination thread w/o needing a context switch to the main thread?
> 
> We should shoot for the latter behavior.

I'm not entirely sure how it works internally in mojo, but I assume that it
wouldn't do unnecessary thread jumping and it'll somehow manage to get messages
directly from the IO thread to whatever thread messages have to be delivered on
(generally the thread GetProxy was called on one side and the thread the service
was created on the other side).

[Marijn Kruisselbrink, 2015-06-25 23:35:57]

On 2015/06/25 at 22:25:47, Marijn Kruisselbrink wrote:
> On 2015/06/25 at 22:05:32, michaeln wrote:
> > > Yes, service registry is only available on the UI thread, but you can
create
> > > your proxy (and start using it) on any thread, and then asynchronously on
the UI
> > > thread actually connect that proxy to a service. All API calls etc will
just get
> > > queued until you actually set up a connection.
> > 
> > In this scheme, are msgs sent from and received by the proxy routed thru UI
thread? Or after the proxy is setup and the connection establish, are incoming
and outgoing msgs exchanged directly between the IPC and backgroud
destination/origination thread w/o needing a context switch to the main thread?
> > 
> > We should shoot for the latter behavior.
> 
> I'm not entirely sure how it works internally in mojo, but I assume that it
wouldn't do unnecessary thread jumping and it'll somehow manage to get messages
directly from the IO thread to whatever thread messages have to be delivered on
(generally the thread GetProxy was called on one side and the thread the service
was created on the other side).

And a I think somewhat clean way to achieve all this for event dispatching might
be for the EmbeddedWorkerHostMsg_WorkerScriptLoaded IPC (which currently is used
to tell the browser of the thread ID of the worker thread) to also somehow
include a (proxy to) a mojo::ServiceProvider (mojo::MessagePipeHandle can be
send over IPC, so I assume that somehow this should be fairly straightforward).
That ServiceProvider will expose services specific to that worker thread, and
can then be used to connect to whatever service is needed for whichever event
we're trying to dispatch.
Another more hacky solution might be to register services with the renderer
process global ServiceProvider encoding the thread id in the service name, but
that seems both hackier and not really better in any way. But I should probably
wait for the design doc and comment on that instead of proposing things that
might have already been considered and rejected.

[iclelland, 2015-06-29 13:48:07]

On 2015/06/25 23:35:57, Marijn Kruisselbrink wrote:
> On 2015/06/25 at 22:25:47, Marijn Kruisselbrink wrote:
> > On 2015/06/25 at 22:05:32, michaeln wrote:
> > > > Yes, service registry is only available on the UI thread, but you can
> create
> > > > your proxy (and start using it) on any thread, and then asynchronously
on
> the UI
> > > > thread actually connect that proxy to a service. All API calls etc will
> just get
> > > > queued until you actually set up a connection.
> > > 
> > > In this scheme, are msgs sent from and received by the proxy routed thru
UI
> thread? Or after the proxy is setup and the connection establish, are incoming
> and outgoing msgs exchanged directly between the IPC and backgroud
> destination/origination thread w/o needing a context switch to the main
thread?
> > > 
> > > We should shoot for the latter behavior.
> > 
> > I'm not entirely sure how it works internally in mojo, but I assume that it
> wouldn't do unnecessary thread jumping and it'll somehow manage to get
messages
> directly from the IO thread to whatever thread messages have to be delivered
on
> (generally the thread GetProxy was called on one side and the thread the
service
> was created on the other side).
> 
> And a I think somewhat clean way to achieve all this for event dispatching
might
> be for the EmbeddedWorkerHostMsg_WorkerScriptLoaded IPC (which currently is
used
> to tell the browser of the thread ID of the worker thread) to also somehow
> include a (proxy to) a mojo::ServiceProvider (mojo::MessagePipeHandle can be
> send over IPC, so I assume that somehow this should be fairly
straightforward).
> That ServiceProvider will expose services specific to that worker thread, and
> can then be used to connect to whatever service is needed for whichever event
> we're trying to dispatch.
> Another more hacky solution might be to register services with the renderer
> process global ServiceProvider encoding the thread id in the service name, but
> that seems both hackier and not really better in any way. But I should
probably
> wait for the design doc and comment on that instead of proposing things that
> might have already been considered and rejected.

I threw up the in-progress design in docs, at
https://docs.google.com/a/chromium.org/document/d/1NTPhkqxnZMQofK5m0P5A8SMpcp...
(for chromium accounts). I added a couple of paragraphs in 'other options' about
streamlining the thread-hopping code. Lets definitely discuss all of this there.
Thanks!

[Tom Sepez, 2015-06-29 18:36:20]

.mojom lgtm

[Marijn Kruisselbrink, 2015-07-02 00:14:42]

On 2015/06/29 at 13:48:07, iclelland wrote:
> I threw up the in-progress design in docs, at
https://docs.google.com/a/chromium.org/document/d/1NTPhkqxnZMQofK5m0P5A8SMpcp...
(for chromium accounts). I added a couple of paragraphs in 'other options' about
streamlining the thread-hopping code. Lets definitely discuss all of this there.
> Thanks!

I added/updated a description of my proposed way of doing this to that document,
and uploaded CLs to https://codereview.chromium.org/1221503003 and
https://codereview.chromium.org/1210643002 to actually implement my proposal.

[iclelland, 2015-07-03 13:18:59]

On 2015/07/02 00:14:42, Marijn Kruisselbrink wrote:
> On 2015/06/29 at 13:48:07, iclelland wrote:
> > I threw up the in-progress design in docs, at
>
https://docs.google.com/a/chromium.org/document/d/1NTPhkqxnZMQofK5m0P5A8SMpcp...
> (for chromium accounts). I added a couple of paragraphs in 'other options'
about
> streamlining the thread-hopping code. Lets definitely discuss all of this
there.
> > Thanks!
> 
> I added/updated a description of my proposed way of doing this to that
document,
> and uploaded CLs to https://codereview.chromium.org/1221503003 and
> https://codereview.chromium.org/1210643002 to actually implement my proposal.

I've rebased this against that CL, and uploaded a different version of this one
at https://codereview.chromium.org/1220943003 -- it's about hald the size, with
the same functionality. Take a look and see what you think.

[iclelland, 2015-07-17 16:06:22]

On 2015/07/03 13:18:59, iclelland wrote:
> On 2015/07/02 00:14:42, Marijn Kruisselbrink wrote:
> > On 2015/06/29 at 13:48:07, iclelland wrote:
> > > I threw up the in-progress design in docs, at
> >
>
https://docs.google.com/a/chromium.org/document/d/1NTPhkqxnZMQofK5m0P5A8SMpcp...
> > (for chromium accounts). I added a couple of paragraphs in 'other options'
> about
> > streamlining the thread-hopping code. Lets definitely discuss all of this
> there.
> > > Thanks!
> > 
> > I added/updated a description of my proposed way of doing this to that
> document,
> > and uploaded CLs to https://codereview.chromium.org/1221503003 and
> > https://codereview.chromium.org/1210643002 to actually implement my
proposal.
> 
> I've rebased this against that CL, and uploaded a different version of this
one
> at https://codereview.chromium.org/1220943003 -- it's about hald the size,
with
> the same functionality. Take a look and see what you think.

Closing this CL, in favour of https://codereview.chromium.org/1220943003