Update navigator.services API to use the new services.onconnect event [2/3].

Update navigator.services API to use the new services.onconnect event [2/3].

This is part of a series of patches to change the service side API from
a global oncrossoriginconnect event to a new navigator.services.onconnect
event.

This patch builds on https://codereview.chromium.org/1221503003 to setup a
mojo service in the worker thread in the renderer which is used to
dispatch the new event to blink.

[1/3] https://codereview.chromium.org/1210633002
      Adds new event to blink.
[2/3] This patch
[3/3] https://codereview.chromium.org/1205783004
      Removes old event from blink and adds some tests specific to
      the new event.

BUG=426458
Committed: https://crrev.com/ef0b94c53140a46f65c8233c7833b0bacd789792
Cr-Commit-Position: refs/heads/master@{#339347}

[michaeln, 2015-07-07 22:13:16]

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

[michaeln, 2015-07-07 22:13:17]

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

https://codereview.chromium.org/1210643002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:938:
service_port_dispatcher_->Connect(
What happens to service_port_dispatcher_ when the embedded instance 'stops' and
then is started in a new process?

[Marijn Kruisselbrink, 2015-07-07 23:58:42]

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

https://codereview.chromium.org/1210643002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:938:
service_port_dispatcher_->Connect(
On 2015/07/07 at 22:13:17, michaeln wrote:
> What happens to service_port_dispatcher_ when the embedded instance 'stops'
and then is started in a new process?

Hmm, good question. I seem to have assumed that that would automatically sever
the connection, thus causing service_port_dispatcher_ to call its connection
error handler, but I think you're right that either OnStoppedInternal or some
method like that should sever these connections on the browser side, or
ServicePortDispatcherImpl would have to destroy itself/sever the connection when
the embedded worker it "belongs" to is stopped.

[iclelland, 2015-07-08 02:59:29]

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

[iclelland, 2015-07-08 02:59:30]

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

https://codereview.chromium.org/1210643002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:929:
embedded_worker_->GetServiceRegistry()->ConnectToRemoteService(
This looks to me like it has the same problem as one of my earlier proposals for
DispatchSyncEvent -- that the ServiceWorkerVersion may outlive any given worker
thread -- I think that service_port_dispatcher_ here will be set once, and never
reset if the worker thread dies. The solution in the Sync case was to move the
mojo connection from the ServiceWorkerVersion to the
ServiceWorkerDispatcherHost.

(see michaeln's comment at
https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
)

https://codereview.chromium.org/1210643002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:938:
service_port_dispatcher_->Connect(
On 2015/07/07 23:58:42, Marijn Kruisselbrink wrote:
> On 2015/07/07 at 22:13:17, michaeln wrote:
> > What happens to service_port_dispatcher_ when the embedded instance 'stops'
> and then is started in a new process?
> 
> Hmm, good question. I seem to have assumed that that would automatically sever
> the connection, thus causing service_port_dispatcher_ to call its connection
> error handler, but I think you're right that either OnStoppedInternal or some
> method like that should sever these connections on the browser side, or
> ServicePortDispatcherImpl would have to destroy itself/sever the connection
when
> the embedded worker it "belongs" to is stopped.

I took a closer look at this last week, since I had a draft message asking
essentially the same thing. As far as I can tell, when the embedded worker
stops, the connection will be shut down automatically, and the InterfacePtr will
test as false the next time that DispatchCrossOriginConnectEvent is called. (see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/mojo/s...)
If there are outstanding callbacks, then the connection error handler will be
called first.
I'd like to test this behaviour, and probably loop someone like rockot@ in to
make sure it's okay, but I think it's correct.

[iclelland, 2015-07-08 03:00:51]

On 2015/07/08 02:59:30, iclelland wrote:
>
https://codereview.chromium.org/1210643002/diff/80001/content/browser/service...
> File content/browser/service_worker/service_worker_version.cc (right):
> 
>
https://codereview.chromium.org/1210643002/diff/80001/content/browser/service...
> content/browser/service_worker/service_worker_version.cc:929:
> embedded_worker_->GetServiceRegistry()->ConnectToRemoteService(
> This looks to me like it has the same problem as one of my earlier proposals
for
> DispatchSyncEvent -- that the ServiceWorkerVersion may outlive any given
worker
> thread -- I think that service_port_dispatcher_ here will be set once, and
never
> reset if the worker thread dies. The solution in the Sync case was to move the
> mojo connection from the ServiceWorkerVersion to the
> ServiceWorkerDispatcherHost.
> 
> (see michaeln's comment at
>
https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
> )
> 
>
https://codereview.chromium.org/1210643002/diff/80001/content/browser/service...
> content/browser/service_worker/service_worker_version.cc:938:
> service_port_dispatcher_->Connect(
> On 2015/07/07 23:58:42, Marijn Kruisselbrink wrote:
> > On 2015/07/07 at 22:13:17, michaeln wrote:
> > > What happens to service_port_dispatcher_ when the embedded instance
'stops'
> > and then is started in a new process?
> > 
> > Hmm, good question. I seem to have assumed that that would automatically
sever
> > the connection, thus causing service_port_dispatcher_ to call its connection
> > error handler, but I think you're right that either OnStoppedInternal or
some
> > method like that should sever these connections on the browser side, or
> > ServicePortDispatcherImpl would have to destroy itself/sever the connection
> when
> > the embedded worker it "belongs" to is stopped.
> 
> I took a closer look at this last week, since I had a draft message asking
> essentially the same thing. As far as I can tell, when the embedded worker
> stops, the connection will be shut down automatically, and the InterfacePtr
will
> test as false the next time that DispatchCrossOriginConnectEvent is called.
(see
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/mojo/s...)
> If there are outstanding callbacks, then the connection error handler will be
> called first.
> I'd like to test this behaviour, and probably loop someone like rockot@ in to
> make sure it's okay, but I think it's correct.

... And I managed to accidentally publish the draft that I mentioned. Please
ignore that one, or take it as my thoughts before I looked more closely at the
mojo code.

[Marijn Kruisselbrink, 2015-07-08 16:55:06]

On 2015/07/08 at 02:59:30, iclelland wrote:
>
https://codereview.chromium.org/1210643002/diff/80001/content/browser/service...
> File content/browser/service_worker/service_worker_version.cc (right):
> 
>
https://codereview.chromium.org/1210643002/diff/80001/content/browser/service...
> content/browser/service_worker/service_worker_version.cc:929:
embedded_worker_->GetServiceRegistry()->ConnectToRemoteService(
> This looks to me like it has the same problem as one of my earlier proposals
for DispatchSyncEvent -- that the ServiceWorkerVersion may outlive any given
worker thread -- I think that service_port_dispatcher_ here will be set once,
and never reset if the worker thread dies. The solution in the Sync case was to
move the mojo connection from the ServiceWorkerVersion to the
ServiceWorkerDispatcherHost.
That's a different problem. ServiceWorkerDispatcherHost will also outlive a
given worker thread. It just won't outlive a given renderer process. As you
point out below, the problem of outliving the renderer process does not exist in
this approach.

> 
> (see michaeln's comment at
https://codereview.chromium.org/1171173002/diff/410001/content/browser/servic...
)
> 
>
https://codereview.chromium.org/1210643002/diff/80001/content/browser/service...
> content/browser/service_worker/service_worker_version.cc:938:
service_port_dispatcher_->Connect(
> On 2015/07/07 23:58:42, Marijn Kruisselbrink wrote:
> > On 2015/07/07 at 22:13:17, michaeln wrote:
> > > What happens to service_port_dispatcher_ when the embedded instance
'stops'
> > and then is started in a new process?
> > 
> > Hmm, good question. I seem to have assumed that that would automatically
sever
> > the connection, thus causing service_port_dispatcher_ to call its connection
> > error handler, but I think you're right that either OnStoppedInternal or
some
> > method like that should sever these connections on the browser side, or
> > ServicePortDispatcherImpl would have to destroy itself/sever the connection
when
> > the embedded worker it "belongs" to is stopped.
> 
> I took a closer look at this last week, since I had a draft message asking
essentially the same thing. As far as I can tell, when the embedded worker
stops, the connection will be shut down automatically, and the InterfacePtr will
test as false the next time that DispatchCrossOriginConnectEvent is called. (see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/mojo/s...)
> If there are outstanding callbacks, then the connection error handler will be
called first.
> I'd like to test this behaviour, and probably loop someone like rockot@ in to
make sure it's okay, but I think it's correct.

If the renderer process the embedded worker lives in dies the connection will be
shut down and everything will work correctly. If just the worker within the
renderer is stopped there currently is nothing that will actually kill the
connection. Ideally there would be a way to tell mojo that when one connection
dies several other connections should also be closed (in this case when the
connection between ServiceProviders dies, the connection to all services that
were setup via that connection should also be closed). But since that doesn't
exist as part of mojo as far as I can tell, we'll have to add something
ourselves, and for that it might indeed be easiest to have a separate class just
as container for all InterfacePtr<> instances that were made to a particular
worker thread. When a worker is stopped that class instance can be destroyed,
and new services just need to add a member to that class instead of adding their
cleanup code to some particular method that they/their reviewer may miss (I know
I've at least missed one cleanup spot in ServiceWorkerVersion when adding an
event in the past).

[Marijn Kruisselbrink, 2015-07-14 17:40:25]

I think I addressed the lifetime issues around the ServicePortDispatcher
connection. The ServicePortDispatcherImpl in the renderer now destroys itself
when the worker thread it lives in goes away (just like the traditional IPC
thread specific dispatcher classes do), and I changed the cleanup code in
ServiceWorkerVersion to simply close the connection (which triggers failed
callbacks) instead of only calling callbacks for still outstanding requests.

[michaeln, 2015-07-15 02:47:26]

Idk if this is really an improvement. There's more code and the new code is more
difficult to grok than the code it replaces?

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

https://codereview.chromium.org/1210643002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:935:
service_port_dispatcher_->Connect(
More curiousity about where the "message pipe" fifo ordering is guaranteed. 
* between MojoApplicationHost and MojoApplication
* between each ServiceRegistryImpl  pair
* between each Service pair

I'd assume its between each Service pair, but just checking?

Also, rockot mentioned that a when you make a mojo call with a callback, no
other calls go thru until completion. The caller can issue more Connect() calls,
but mojo will sit on them until a prior connect call completes. (I hope thats
not right, but i think thats what i heard?)

 Connect(string target_url, string origin, int32 port_id)
       => (ServicePortConnectResult result, string name, string data);

The existing ipc code code delivers ServiceWorkerMsg_CrossOriginConnectEvent
msgs back to back giving the remote side a chance to process them concurrently
if possible. Do we lose that impl detail with this switch to mojo?

https://codereview.chromium.org/1210643002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:2179: // for messages
that are still outstanding for those services.
I'm curious. Prior to getting here, the embeddedworkerinstance will have reset
the registry from whence this dispatcher came. What affect does that registry
reset have on this dispatcher object?

https://codereview.chromium.org/1210643002/diff/100001/content/child/navigato...
File content/child/navigator_connect/service_port_dispatcher_impl.cc (right):

https://codereview.chromium.org/1210643002/diff/100001/content/child/navigato...
content/child/navigator_connect/service_port_dispatcher_impl.cc:16: class
ConnectCallbacks : public blink::WebServicePortConnectEventCallbacks {
nit: maybe name this differently to make it more obvious that a ConnectCallbacks
and a ConnectCallback are difference things. WebConnectCallbacksImpl maybe?

[Marijn Kruisselbrink, 2015-07-15 20:49:40]

On 2015/07/15 at 02:47:26, michaeln wrote:
> Idk if this is really an improvement. There's more code and the new code is
more difficult to grok than the code it replaces?

One reason it's more code is because it's more decoupled. On the renderer side
the only coupling is one line in ServiceWorkerContextClient to register the mojo
service with the service registry, other than that the navigator.connect code is
fully self contained (well, on the blink side they're still somewhat linked, but
that's a relatively straight-forward fix as well). On the browser side it is
true that this decoupling results in slightly more code. We could instead have
one big mojo interface for all events that we might want to dispatch to a
service worker. That way we wouldn't have to deal with service registries, there
wouldn't be extra service connections to keep track of, and the code in
ServiceWorkerVersion could indeed be simpler. I don't think stronger coupling is
better, even if it results in less code though.

I'm not entirely happy with some of the complexity and coupling in the browser
side code here. I think there is value in trying to refactor
ServiceWorkerVersion etc more, so we don't have to add code for every new event
to SWV. I'm just not sure how feasible that is, and I haven't put enough thought
into it yet to come up with an elegant way to do so.

Finally part of the "more code" part comes from this being the first mojo
service that is used this way. With more services we could probably abstract
away more of the code, although I don't think that would exactly make it easier
to grok (for one I'm not sure if the templated OnMojoConnectionError method
really is any easier to read than just writing separate error handling methods
for each service). And I don't agree that the new code in ServiceWorkerVersion
is really any more complicated than the existing code (I do think there is room
for improvement for both, but I tried to not change too much at once).

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

https://codereview.chromium.org/1210643002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:935:
service_port_dispatcher_->Connect(
On 2015/07/15 at 02:47:25, michaeln wrote:
> More curiousity about where the "message pipe" fifo ordering is guaranteed. 
> * between MojoApplicationHost and MojoApplication
> * between each ServiceRegistryImpl  pair
> * between each Service pair
> 
> I'd assume its between each Service pair, but just checking?
Yes, I'm pretty sure it's between each Service pair.

> Also, rockot mentioned that a when you make a mojo call with a callback, no
other calls go thru until completion. The caller can issue more Connect() calls,
but mojo will sit on them until a prior connect call completes. (I hope thats
not right, but i think thats what i heard?)
>
>  Connect(string target_url, string origin, int32 port_id)
>        => (ServicePortConnectResult result, string name, string data);
> 
> The existing ipc code code delivers ServiceWorkerMsg_CrossOriginConnectEvent
msgs back to back giving the remote side a chance to process them concurrently
if possible. Do we lose that impl detail with this switch to mojo?

I'd be surprised if that is true (and from
https://groups.google.com/a/chromium.org/d/msg/mojo-dev/7kNfPXtVLxM/IaCbIgCI2v4J
it appears that isn't true, although I might have phrased my question wrong),
although it wouldn't be too disastrous here, since with one service per worker
multiple calls will end up being serialized/queued somewhere anyway.

https://codereview.chromium.org/1210643002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:2179: // for messages
that are still outstanding for those services.
On 2015/07/15 at 02:47:26, michaeln wrote:
> I'm curious. Prior to getting here, the embeddedworkerinstance will have reset
the registry from whence this dispatcher came. What affect does that registry
reset have on this dispatcher object?

The registry reset has no effect on dispatcher objects. This is where I was
hoping mojo had some kind of way to make connections depend on the lifetime of
other connections to automatically clean up all connections to services when the
registry is closed, but that doesn't happen. Each channel is fully independent.

https://codereview.chromium.org/1210643002/diff/100001/content/child/navigato...
File content/child/navigator_connect/service_port_dispatcher_impl.cc (right):

https://codereview.chromium.org/1210643002/diff/100001/content/child/navigato...
content/child/navigator_connect/service_port_dispatcher_impl.cc:16: class
ConnectCallbacks : public blink::WebServicePortConnectEventCallbacks {
On 2015/07/15 at 02:47:26, michaeln wrote:
> nit: maybe name this differently to make it more obvious that a
ConnectCallbacks and a ConnectCallback are difference things.
WebConnectCallbacksImpl maybe?

Good point, done.

[michaeln, 2015-07-16 22:23:47]

lgtm (again thanx for you patience and taking the time to learn me some mojo)

> I'm not entirely happy with some of the complexity and coupling in
> the browser side code here.

That's what i'm looking at too, i have a comment about that below.

> And I don't agree that the new code in ServiceWorkerVersion
> is really any more complicated than the existing code

Some of that is just my reaction to seeing mojo usage in detail for the first
time. Some things are quite magical.

mojo::InterfacePtr<Foo> foo;
  ConnectToRemoteService(mojo::GetProxy(&foo));
turns into
  ConntectToRemoteService(StringPiece, ScopedMessagePipeHandle);
  // look ma, seemingly nothing todo with foo?
which makes the the foo ptr magically callable, wow super whizzy ptr :)

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

https://codereview.chromium.org/1210643002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:935:
service_port_dispatcher_->Connect(
On 2015/07/15 20:49:40, Marijn Kruisselbrink wrote:
> On 2015/07/15 at 02:47:25, michaeln wrote:
> > More curiousity about where the "message pipe" fifo ordering is guaranteed. 
> > * between MojoApplicationHost and MojoApplication
> > * between each ServiceRegistryImpl  pair
> > * between each Service pair
> > 
> > I'd assume its between each Service pair, but just checking?
> Yes, I'm pretty sure it's between each Service pair.

Right, "there is one FIFO per interface"
 
> > Also, rockot mentioned that a when you make a mojo call with a callback, no
> other calls go thru until completion. The caller can issue more Connect()
calls,
> but mojo will sit on them until a prior connect call completes. (I hope thats
> not right, but i think thats what i heard?)
> >
> >  Connect(string target_url, string origin, int32 port_id)
> >        => (ServicePortConnectResult result, string name, string data);
> > 
> > The existing ipc code code delivers ServiceWorkerMsg_CrossOriginConnectEvent
> msgs back to back giving the remote side a chance to process them concurrently
> if possible. Do we lose that impl detail with this switch to mojo?
> 
> I'd be surprised if that is true (and from
>
https://groups.google.com/a/chromium.org/d/msg/mojo-dev/7kNfPXtVLxM/IaCbIgCI2v4J
> it appears that isn't true, although I might have phrased my question wrong),
> although it wouldn't be too disastrous here, since with one service per worker
> multiple calls will end up being serialized/queued somewhere anyway.

Thnx for checking. Your question and the answer looks clear enough to me. Nice
that mojo will complain about neglecting to return when the idl (err mojom)
describes a return.

https://codereview.chromium.org/1210643002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:2179: // for messages
that are still outstanding for those services.
On 2015/07/15 20:49:40, Marijn Kruisselbrink wrote:
> On 2015/07/15 at 02:47:26, michaeln wrote:
> > I'm curious. Prior to getting here, the embeddedworkerinstance will have
reset
> the registry from whence this dispatcher came. What affect does that registry
> reset have on this dispatcher object?
> 
> The registry reset has no effect on dispatcher objects. This is where I was
> hoping mojo had some kind of way to make connections depend on the lifetime of
> other connections to automatically clean up all connections to services when
the
> registry is closed, but that doesn't happen. Each channel is fully
independent.

Got it (or maybe starting to get it). In mojo terms, i guess "each message pipe
is fully independent" would be more accurate, and each instance of an
interfaceptr/interfaceimpl has a distinct pipe.

https://codereview.chromium.org/1210643002/diff/100001/content/child/navigato...
File content/child/navigator_connect/service_port_dispatcher_impl.cc (right):

https://codereview.chromium.org/1210643002/diff/100001/content/child/navigato...
content/child/navigator_connect/service_port_dispatcher_impl.cc:16: class
ConnectCallbacks : public blink::WebServicePortConnectEventCallbacks {
On 2015/07/15 20:49:40, Marijn Kruisselbrink wrote:
> On 2015/07/15 at 02:47:26, michaeln wrote:
> > nit: maybe name this differently to make it more obvious that a
> ConnectCallbacks and a ConnectCallback are difference things.
> WebConnectCallbacksImpl maybe?
> 
> Good point, done.

thnx!

https://codereview.chromium.org/1210643002/diff/180001/content/browser/naviga...
File
content/browser/navigator_connect/navigator_connect_service_worker_service_factory.cc
(right):

https://codereview.chromium.org/1210643002/diff/180001/content/browser/naviga...
content/browser/navigator_connect/navigator_connect_service_worker_service_factory.cc:174:
active_version->DispatchServicePortConnectEvent(
Generalizing this to asyncly return either a  ref to a desired interfaceptr or 
ServiceRegistry might help with greater decoupling.

[Marijn Kruisselbrink, 2015-07-17 00:04:23]

https://codereview.chromium.org/1210643002/diff/180001/content/browser/naviga...
File
content/browser/navigator_connect/navigator_connect_service_worker_service_factory.cc
(right):

https://codereview.chromium.org/1210643002/diff/180001/content/browser/naviga...
content/browser/navigator_connect/navigator_connect_service_worker_service_factory.cc:174:
active_version->DispatchServicePortConnectEvent(
On 2015/07/16 at 22:23:46, michaeln wrote:
> Generalizing this to asyncly return either a  ref to a desired interfaceptr or
 ServiceRegistry might help with greater decoupling.

Yeah, something like that might help, although the main issue with trying to
decouple dispatching some specific event from ServiceWorkerVersion is all the
timeout related stuff. Starting a worker if it's not running and sending it some
kind of IPC (traditional or mojo) isn't hard without touching any service worker
code. What is hard is doing it in such a way that SWVersion knows there is a
request in progress, and when that request finishes (as well as giving SWVersion
a way to tell the external code that some request should have timed out). Any
solution I can come up with is either ridiculously complicated/involves many
layers of abstractions and potentially template magic, or would result in a
large amount of new boilerplate code for each event type, neither of which seem
really great.

Something that might work is to have new methods like:
int ServiceWorkerVersion::StartRequest(base::Callback<void(int request_id,
ServiceWorkerStatusCode)>& error_callback);
void ServiceWorkerVerison::RequestFinished(int request_id);

SWVersion would call the error_callback when the worker stops or if a request
should timeout, and otherwise whoever called StartRequest is responsible for
calling RequestFinished when it got a response for a request.
Actually figuring out where to store mojo InterfacePtrs and when to close them
would still be potentially problematic/would need some separate API...

[Marijn Kruisselbrink, 2015-07-17 17:07:27]

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

[Marijn Kruisselbrink, 2015-07-17 17:07:28]

+tsepez for mojom/IPC owners

[Tom Sepez, 2015-07-17 21:36:38]

lgtm

[Marijn Kruisselbrink, 2015-07-17 22:31:46]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2015-07-17 22:31:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/1210643002/#ps200001
(title: "properly run error callbacks when worker is stopped")

[commit-bot: I haz the power, 2015-07-17 22:32:04]

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

[commit-bot: I haz the power, 2015-07-17 22:45:15]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-07-17 22:46:11]

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