Video Capture Mojo (1.4c): Handle Subsequent Access

Video Capture Mojo: Handle Subsequent Access

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1].

A class diagram illustrating the APIs and implementations is found at [2].

* Added class FakeDeviceDescriptorVideoCaptureServiceTest test fixture into the inheritance chain between VideoCaptureServiceTest and FakeDeviceVideoCaptureServiceTest. This is to avoid duplication of test SetUp() code.
* Added test cases using the new test setup:
  1. AccessIsRevokedOnSecondAccess
  2. CanUseSecondRequestedProxy
* In file video_capture_device_factory.mojom: Added comment for specifying expected behavior.
* Added code needed to make new test cases pass.

BUG=584797
TEST=Build of video_capture_unittests succeeds. All video_capture_unittests pass.

[1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit?usp=sharing
[2] https://docs.google.com/a/chromium.org/drawings/d/1RUgghIp8HHwurQOWEcs7-UVQXdfLPUyhM6Afaj1MZtM/edit?usp=sharing

Committed: https://crrev.com/fa6a12d52e60cce192793e16da4a42c17296dc19
Cr-Commit-Position: refs/heads/master@{#416693}

[chfremer, 2016-08-12 17:58:39]

Description was changed from

==========
Merge branch 'FillServicePart2' into FillServicePart3


Fill Service Part 3

BUG=
==========

to

==========
Video Capture Mojo: Handle Subsequent Access

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1].
This is a follow up to a CL [2] currently under review. 

* Added service test:
  + AccessIsRevokedOnSecondAccess
* Added implementation neccessary to make tests pass.

BUG=584797
TEST=Build of video_capture_unittests succeeds. All video_capture_unittests
pass.

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2] https://codereview.chromium.org/2238083004/
==========

[chfremer, 2016-08-12 18:00:55]

chfremer@chromium.org changed reviewers:
+ emircan@chromium.org, mcasas@chromium.org, rockot@chromium.org,
yzshen@chromium.org

[chfremer, 2016-08-12 18:00:56]

mcasas@: Please review general approach and style
yzshen@: Please review Mojo aspects
emircan@ & rockot@: FYI

[chfremer, 2016-09-01 20:45:18]

Description was changed from

==========
Video Capture Mojo: Handle Subsequent Access

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1].
This is a follow up to a CL [2] currently under review. 

* Added service test:
  + AccessIsRevokedOnSecondAccess
* Added implementation neccessary to make tests pass.

BUG=584797
TEST=Build of video_capture_unittests succeeds. All video_capture_unittests
pass.

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2] https://codereview.chromium.org/2238083004/
==========

to

==========
Video Capture Mojo: Handle Subsequent Access

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1].
This is a follow up to a CL [2] currently under review. 

* Added class FakeDeviceDescriptorVideoCaptureServiceTest test fixture into the
inheritance chain between VideoCaptureServiceTest and
FakeDeviceVideoCaptureServiceTest. This is to avoid duplication of test SetUp()
code.
* Added test cases using the new test setup:
  1. AccessIsRevokedOnSecondAccess
  2. CanUseSecondRequestedProxy
* In file video_capture_device_factory.mojom: Added comment for specifying
expected behavior.
* Added code needed to make new test cases pass.

BUG=584797
TEST=Build of video_capture_unittests succeeds. All video_capture_unittests
pass.

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2] https://codereview.chromium.org/2238083004/
==========

[chfremer, 2016-09-01 20:45:30]

Patchset #5 (id:80001) has been deleted

[chfremer, 2016-09-01 20:45:37]

Patchset #4 (id:60001) has been deleted

[chfremer, 2016-09-01 20:45:43]

Patchset #3 (id:40001) has been deleted

[chfremer, 2016-09-01 20:45:51]

Patchset #2 (id:20001) has been deleted

[chfremer, 2016-09-01 20:45:57]

Patchset #1 (id:1) has been deleted

[chfremer, 2016-09-01 20:48:10]

mcasas@: PTAL general review
rockot@: PTAL Mojo specifics

emircan@: FYI
yzshen@: FYI

[chfremer, 2016-09-01 20:49:28]

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

[commit-bot: I haz the power, 2016-09-01 20:49:53]

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

[chfremer, 2016-09-01 20:52:22]

Description was changed from

==========
Video Capture Mojo: Handle Subsequent Access

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1].
This is a follow up to a CL [2] currently under review. 

* Added class FakeDeviceDescriptorVideoCaptureServiceTest test fixture into the
inheritance chain between VideoCaptureServiceTest and
FakeDeviceVideoCaptureServiceTest. This is to avoid duplication of test SetUp()
code.
* Added test cases using the new test setup:
  1. AccessIsRevokedOnSecondAccess
  2. CanUseSecondRequestedProxy
* In file video_capture_device_factory.mojom: Added comment for specifying
expected behavior.
* Added code needed to make new test cases pass.

BUG=584797
TEST=Build of video_capture_unittests succeeds. All video_capture_unittests
pass.

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2] https://codereview.chromium.org/2238083004/
==========

to

==========
Video Capture Mojo: Handle Subsequent Access

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1].

* Added class FakeDeviceDescriptorVideoCaptureServiceTest test fixture into the
inheritance chain between VideoCaptureServiceTest and
FakeDeviceVideoCaptureServiceTest. This is to avoid duplication of test SetUp()
code.
* Added test cases using the new test setup:
  1. AccessIsRevokedOnSecondAccess
  2. CanUseSecondRequestedProxy
* In file video_capture_device_factory.mojom: Added comment for specifying
expected behavior.
* Added code needed to make new test cases pass.

BUG=584797
TEST=Build of video_capture_unittests succeeds. All video_capture_unittests
pass.

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

[commit-bot: I haz the power, 2016-09-01 22:17:44]

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

[commit-bot: I haz the power, 2016-09-01 22:17:45]

Dry run: This issue passed the CQ dry run.

[mcasas, 2016-09-02 16:15:15]

Some minor comments.

https://codereview.chromium.org/2244773002/diff/100001/services/video_capture...
File services/video_capture/fake_device_descriptor_video_capture_service_test.h
(right):

https://codereview.chromium.org/2244773002/diff/100001/services/video_capture...
services/video_capture/fake_device_descriptor_video_capture_service_test.h:15: :
public VideoCaptureServiceTest {
The class hierarchy makes sense, but as commented 
offline, the names get too verbose, I'd suggest
naming following the tested member variable's type,
i.e. 
s/FakeDeviceDescriptorVCSTest/FakeDeviceDescriptorTest/ 
and 
s/FakeDeviceVCSTest/FakeVideoCaptureDeviceProxyTest/

or some shortened versions, as long as they are clearly
unique in the current context && namespace.

https://codereview.chromium.org/2244773002/diff/100001/services/video_capture...
File
services/video_capture/fake_device_descriptor_video_capture_service_unittest.cc
(right):

https://codereview.chromium.org/2244773002/diff/100001/services/video_capture...
services/video_capture/fake_device_descriptor_video_capture_service_unittest.cc:39:
mojom::VideoCaptureDeviceProxyPtr device_proxy_2;
Insert in between l.38-39
ASSERT_FALSE(device_access_1_revoked);

Also, shouldn't 
ASSERT_EQ(mojom::DeviceAccessResultCode::SUCCESS, result_code_1);
be here? 

Oh I see, you run the RunLoop only once at the end,
but in that case, aren't you potentially missing
the intermediate state where |device_proxy_1| is
correctly bound and |device_proxy_2| is not existing
yet? 

Consider running the loop twice, or adding a simple
test case where a Proxy is just created successfully.

https://codereview.chromium.org/2244773002/diff/100001/services/video_capture...
services/video_capture/fake_device_descriptor_video_capture_service_unittest.cc:75:
&wait_loop));
I like the local lambda methods, but MOCKs would
also allow you to specify how many times the method
is expected to be called, and will kick in if
expectations are not met or unexpected method calls
are there.  So, why not MOCKing :-)  ?

https://codereview.chromium.org/2244773002/diff/100001/services/video_capture...
File services/video_capture/video_capture_device_proxy_impl.cc (right):

https://codereview.chromium.org/2244773002/diff/100001/services/video_capture...
services/video_capture/video_capture_device_proxy_impl.cc:15:
VideoCaptureDeviceProxyImpl::~VideoCaptureDeviceProxyImpl() = default;
Hmm I wonder if we should call Stop() here
as well, or if the VCD implementation(s) do
clean up on destruction.

[chfremer, 2016-09-02 17:44:32]

mcasas@: PTAL

https://codereview.chromium.org/2244773002/diff/100001/services/video_capture...
File services/video_capture/fake_device_descriptor_video_capture_service_test.h
(right):

https://codereview.chromium.org/2244773002/diff/100001/services/video_capture...
services/video_capture/fake_device_descriptor_video_capture_service_test.h:15: :
public VideoCaptureServiceTest {
On 2016/09/02 16:15:14, mcasas wrote:
> The class hierarchy makes sense, but as commented 
> offline, the names get too verbose, I'd suggest
> naming following the tested member variable's type,
> i.e. 
> s/FakeDeviceDescriptorVCSTest/FakeDeviceDescriptorTest/ 
> and 
> s/FakeDeviceVCSTest/FakeVideoCaptureDeviceProxyTest/
> 
> or some shortened versions, as long as they are clearly
> unique in the current context && namespace.

Done.

https://codereview.chromium.org/2244773002/diff/100001/services/video_capture...
File
services/video_capture/fake_device_descriptor_video_capture_service_unittest.cc
(right):

https://codereview.chromium.org/2244773002/diff/100001/services/video_capture...
services/video_capture/fake_device_descriptor_video_capture_service_unittest.cc:39:
mojom::VideoCaptureDeviceProxyPtr device_proxy_2;
On 2016/09/02 16:15:14, mcasas wrote:
> Insert in between l.38-39
> ASSERT_FALSE(device_access_1_revoked);
> 
> Also, shouldn't 
> ASSERT_EQ(mojom::DeviceAccessResultCode::SUCCESS, result_code_1);
> be here? 
> 
> Oh I see, you run the RunLoop only once at the end,
> but in that case, aren't you potentially missing
> the intermediate state where |device_proxy_1| is
> correctly bound and |device_proxy_2| is not existing
> yet? 
> 
> Consider running the loop twice, or adding a simple
> test case where a Proxy is just created successfully.
> 

Good observation. I am considering adding a new test case for checking that a
Proxy is just created successfully.
But the same already happens and is verified as part of the
FakeDeviceTest::SetUp() method.
Is it worth adding an explicit test for it?

https://codereview.chromium.org/2244773002/diff/100001/services/video_capture...
services/video_capture/fake_device_descriptor_video_capture_service_unittest.cc:75:
&wait_loop));
On 2016/09/02 16:15:15, mcasas wrote:
> I like the local lambda methods, but MOCKs would
> also allow you to specify how many times the method
> is expected to be called, and will kick in if
> expectations are not met or unexpected method calls
> are there.  So, why not MOCKing :-)  ?

Good point. I didn't think of it, because I traditionally only think of using
mocks to implement interface classes. But as you are suggesting, callback
methods are actually also interfaces. I guess the only reason not to do it would
be the extra amount of code it would need. But it is not that much.

I'll give it a try.

https://codereview.chromium.org/2244773002/diff/100001/services/video_capture...
File services/video_capture/video_capture_device_proxy_impl.cc (right):

https://codereview.chromium.org/2244773002/diff/100001/services/video_capture...
services/video_capture/video_capture_device_proxy_impl.cc:15:
VideoCaptureDeviceProxyImpl::~VideoCaptureDeviceProxyImpl() = default;
On 2016/09/02 16:15:15, mcasas wrote:
> Hmm I wonder if we should call Stop() here
> as well, or if the VCD implementation(s) do
> clean up on destruction.

Thanks for catching this. The VCD description says "StopAndDeAllocate() must be
called/ before the object is deleted". It does not say whether or not it is okay
to call it twice, though, so I'll add a flag to make sure we do not call it
twice when the device is already stopped when the destructor is called.

[chfremer, 2016-09-02 18:26:50]

Description was changed from

==========
Video Capture Mojo: Handle Subsequent Access

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1].

* Added class FakeDeviceDescriptorVideoCaptureServiceTest test fixture into the
inheritance chain between VideoCaptureServiceTest and
FakeDeviceVideoCaptureServiceTest. This is to avoid duplication of test SetUp()
code.
* Added test cases using the new test setup:
  1. AccessIsRevokedOnSecondAccess
  2. CanUseSecondRequestedProxy
* In file video_capture_device_factory.mojom: Added comment for specifying
expected behavior.
* Added code needed to make new test cases pass.

BUG=584797
TEST=Build of video_capture_unittests succeeds. All video_capture_unittests
pass.

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

to

==========
Video Capture Mojo: Handle Subsequent Access

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1].

A class diagram illustrating the APIs and implementations is found at [2].

* Added class FakeDeviceDescriptorVideoCaptureServiceTest test fixture into the
inheritance chain between VideoCaptureServiceTest and
FakeDeviceVideoCaptureServiceTest. This is to avoid duplication of test SetUp()
code.
* Added test cases using the new test setup:
  1. AccessIsRevokedOnSecondAccess
  2. CanUseSecondRequestedProxy
* In file video_capture_device_factory.mojom: Added comment for specifying
expected behavior.
* Added code needed to make new test cases pass.

BUG=584797
TEST=Build of video_capture_unittests succeeds. All video_capture_unittests
pass.

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2]
https://docs.google.com/a/chromium.org/drawings/d/1RUgghIp8HHwurQOWEcs7-UVQXd...
==========

[mcasas, 2016-09-03 01:23:48]

lgtm

bots please :-)

[chfremer, 2016-09-04 19:48:41]

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

[commit-bot: I haz the power, 2016-09-04 19:48:51]

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

[commit-bot: I haz the power, 2016-09-04 21:02:27]

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

[commit-bot: I haz the power, 2016-09-04 21:02:28]

Dry run: This issue passed the CQ dry run.

[chfremer, 2016-09-06 15:20:08]

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

[chfremer, 2016-09-06 15:20:09]

tsepez@: PTAL for video_capture_device_factory.mojom: Added comment for
specifying expected behavior.

[Ken Rockot(use gerrit already), 2016-09-06 16:50:26]

lgtm

https://codereview.chromium.org/2244773002/diff/140001/services/video_capture...
File services/video_capture/video_capture_device_factory_impl.h (right):

https://codereview.chromium.org/2244773002/diff/140001/services/video_capture...
services/video_capture/video_capture_device_factory_impl.h:62:
std::unique_ptr<mojo::Binding<mojom::VideoCaptureDeviceProxy>> binding_;
Apologies if I missed this in a prior CL, but why the unique_ptr? If you just
owned a mojo::Binding, you could Close and re-Bind it as needed.

https://codereview.chromium.org/2244773002/diff/140001/services/video_capture...
File services/video_capture/video_capture_device_proxy_impl.h (right):

https://codereview.chromium.org/2244773002/diff/140001/services/video_capture...
services/video_capture/video_capture_device_proxy_impl.h:45: bool
device_running_;
nitty nit: My personal preference is to use in-class initializers rather than
constructor initializers when possible. But it is just a preference so feel free
to ignore

[Ken Rockot(use gerrit already), 2016-09-06 17:00:29]

https://codereview.chromium.org/2244773002/diff/140001/services/video_capture...
File services/video_capture/video_capture_device_factory_impl.h (right):

https://codereview.chromium.org/2244773002/diff/140001/services/video_capture...
services/video_capture/video_capture_device_factory_impl.h:62:
std::unique_ptr<mojo::Binding<mojom::VideoCaptureDeviceProxy>> binding_;
On 2016/09/06 at 16:50:26, Ken Rockot wrote:
> Apologies if I missed this in a prior CL, but why the unique_ptr? If you just
owned a mojo::Binding, you could Close and re-Bind it as needed.

Nevermind, thought this was a field in the impl, not in DeviceEntry. Makes
sense.

[chfremer, 2016-09-06 17:29:59]

https://codereview.chromium.org/2244773002/diff/140001/services/video_capture...
File services/video_capture/video_capture_device_factory_impl.h (right):

https://codereview.chromium.org/2244773002/diff/140001/services/video_capture...
services/video_capture/video_capture_device_factory_impl.h:62:
std::unique_ptr<mojo::Binding<mojom::VideoCaptureDeviceProxy>> binding_;
On 2016/09/06 17:00:28, Ken Rockot wrote:
> On 2016/09/06 at 16:50:26, Ken Rockot wrote:
> > Apologies if I missed this in a prior CL, but why the unique_ptr? If you
just
> owned a mojo::Binding, you could Close and re-Bind it as needed.
> 
> Nevermind, thought this was a field in the impl, not in DeviceEntry. Makes
> sense.

Thanks for raising this. After offline discussion, I added a TODO and bug entry
with a feature request to make mojo::Binding a movable type.

https://codereview.chromium.org/2244773002/diff/140001/services/video_capture...
File services/video_capture/video_capture_device_proxy_impl.h (right):

https://codereview.chromium.org/2244773002/diff/140001/services/video_capture...
services/video_capture/video_capture_device_proxy_impl.h:45: bool
device_running_;
On 2016/09/06 16:50:26, Ken Rockot wrote:
> nitty nit: My personal preference is to use in-class initializers rather than
> constructor initializers when possible. But it is just a preference so feel
free
> to ignore

Done.

[Tom Sepez, 2016-09-06 17:57:03]

RS LGTM on comment in mojom

[chfremer, 2016-09-06 18:12:31]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2016-09-06 18:12:31]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/2244773002/#ps160001
(title: "rockot's comments")

[commit-bot: I haz the power, 2016-09-06 18:13:01]

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

[commit-bot: I haz the power, 2016-09-06 20:19:33]

Description was changed from

==========
Video Capture Mojo: Handle Subsequent Access

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1].

A class diagram illustrating the APIs and implementations is found at [2].

* Added class FakeDeviceDescriptorVideoCaptureServiceTest test fixture into the
inheritance chain between VideoCaptureServiceTest and
FakeDeviceVideoCaptureServiceTest. This is to avoid duplication of test SetUp()
code.
* Added test cases using the new test setup:
  1. AccessIsRevokedOnSecondAccess
  2. CanUseSecondRequestedProxy
* In file video_capture_device_factory.mojom: Added comment for specifying
expected behavior.
* Added code needed to make new test cases pass.

BUG=584797
TEST=Build of video_capture_unittests succeeds. All video_capture_unittests
pass.

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2]
https://docs.google.com/a/chromium.org/drawings/d/1RUgghIp8HHwurQOWEcs7-UVQXd...
==========

to

==========
Video Capture Mojo: Handle Subsequent Access

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1].

A class diagram illustrating the APIs and implementations is found at [2].

* Added class FakeDeviceDescriptorVideoCaptureServiceTest test fixture into the
inheritance chain between VideoCaptureServiceTest and
FakeDeviceVideoCaptureServiceTest. This is to avoid duplication of test SetUp()
code.
* Added test cases using the new test setup:
  1. AccessIsRevokedOnSecondAccess
  2. CanUseSecondRequestedProxy
* In file video_capture_device_factory.mojom: Added comment for specifying
expected behavior.
* Added code needed to make new test cases pass.

BUG=584797
TEST=Build of video_capture_unittests succeeds. All video_capture_unittests
pass.

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2]
https://docs.google.com/a/chromium.org/drawings/d/1RUgghIp8HHwurQOWEcs7-UVQXd...
==========

[commit-bot: I haz the power, 2016-09-06 20:19:34]

Committed patchset #4 (id:160001)

[commit-bot: I haz the power, 2016-09-06 20:22:01]

Description was changed from

==========
Video Capture Mojo: Handle Subsequent Access

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1].

A class diagram illustrating the APIs and implementations is found at [2].

* Added class FakeDeviceDescriptorVideoCaptureServiceTest test fixture into the
inheritance chain between VideoCaptureServiceTest and
FakeDeviceVideoCaptureServiceTest. This is to avoid duplication of test SetUp()
code.
* Added test cases using the new test setup:
  1. AccessIsRevokedOnSecondAccess
  2. CanUseSecondRequestedProxy
* In file video_capture_device_factory.mojom: Added comment for specifying
expected behavior.
* Added code needed to make new test cases pass.

BUG=584797
TEST=Build of video_capture_unittests succeeds. All video_capture_unittests
pass.

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2]
https://docs.google.com/a/chromium.org/drawings/d/1RUgghIp8HHwurQOWEcs7-UVQXd...
==========

to

==========
Video Capture Mojo: Handle Subsequent Access

This CL is part of the Mojo Video Capture work. For the bigger picture, see [1].

A class diagram illustrating the APIs and implementations is found at [2].

* Added class FakeDeviceDescriptorVideoCaptureServiceTest test fixture into the
inheritance chain between VideoCaptureServiceTest and
FakeDeviceVideoCaptureServiceTest. This is to avoid duplication of test SetUp()
code.
* Added test cases using the new test setup:
  1. AccessIsRevokedOnSecondAccess
  2. CanUseSecondRequestedProxy
* In file video_capture_device_factory.mojom: Added comment for specifying
expected behavior.
* Added code needed to make new test cases pass.

BUG=584797
TEST=Build of video_capture_unittests succeeds. All video_capture_unittests
pass.

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2]
https://docs.google.com/a/chromium.org/drawings/d/1RUgghIp8HHwurQOWEcs7-UVQXd...

Committed: https://crrev.com/fa6a12d52e60cce192793e16da4a42c17296dc19
Cr-Commit-Position: refs/heads/master@{#416693}
==========

[commit-bot: I haz the power, 2016-09-06 20:22:03]

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