Shape Detection: Add two layout tests for face detection

Shape Detection: Add two layout tests for face detection.

This CL includes security tests for empty image and undecodable image.

BUG=646083
TEST=third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html

Committed: https://crrev.com/3995eb71799c911b01c05f8c128f8065636591c3
Cr-Commit-Position: refs/heads/master@{#427234}

[xianglu, 2016-10-21 03:38:07]

Description was changed from

==========
Added two layouttests for FaceDetector: empty image and undecodable image.

BUG=646083
TEST=
==========

to

==========
Added two layouttests for FaceDetector: empty image and undecodable image.

BUG=646083
TEST=third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html
==========

[xianglu, 2016-10-21 03:40:02]

Description was changed from

==========
Added two layouttests for FaceDetector: empty image and undecodable image.

BUG=646083
TEST=third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html
==========

to

==========
This CL adds layout tests for empty image and undecodable image respectively.

BUG=646083
TEST=third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html
==========

[xianglu, 2016-10-21 03:44:02]

Patchset #1 (id:1) has been deleted

[xianglu, 2016-10-21 03:44:16]

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

[commit-bot: I haz the power, 2016-10-21 03:44:32]

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

[xianglu, 2016-10-21 03:48:38]

Description was changed from

==========
This CL adds layout tests for empty image and undecodable image respectively.

BUG=646083
TEST=third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html
==========

to

==========
Shape Detection: Add two layout tests for face detection.

This CL adds two security tests for empty image and undecodable image
respectively).

BUG=646083
TEST=third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html
==========

[xianglu, 2016-10-21 03:48:57]

Description was changed from

==========
Shape Detection: Add two layout tests for face detection.

This CL adds two security tests for empty image and undecodable image
respectively).

BUG=646083
TEST=third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html
==========

to

==========
Shape Detection: Add two layout tests for face detection.

This CL adds security tests for empty image and undecodable image.

BUG=646083
TEST=third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html
==========

[xianglu, 2016-10-21 03:49:13]

Description was changed from

==========
Shape Detection: Add two layout tests for face detection.

This CL adds security tests for empty image and undecodable image.

BUG=646083
TEST=third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html
==========

to

==========
Shape Detection: Add two layout tests for face detection.

This CL includes security tests for empty image and undecodable image.

BUG=646083
TEST=third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html
==========

[xianglu, 2016-10-21 03:49:40]

xianglu@chromium.org changed reviewers:
+ mcasas@chromium.org

[xianglu, 2016-10-21 03:49:41]

ptal.

[mcasas, 2016-10-21 04:41:46]

Looking good, a few comments.

https://codereview.chromium.org/2441953002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html
(right):

https://codereview.chromium.org/2441953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html:6:
// TODO(xianglu): Consider tests for image with only one dimension equal to
zero.
"Consider adding ..."

Although there's no hard limit of 80 characters in JS files
that I know of, it's a good idea to stick to it, especially since
here you have two lines

https://codereview.chromium.org/2441953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html:12:
assert_equals(emptyImage.naturalWidth + emptyImage.naturalHeight, 0);
Here you can just assert_equals(), no need to create
and added test step such as l.11. Same elsewhere:
since we're inside an async_test(), you can directly
assert_*(), no need for extra step()s.

https://codereview.chromium.org/2441953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html:17:
.then(faceDetectionResult => {
Usually .then() and .catch() clauses are indented 4
spaces to the right.

https://codereview.chromium.org/2441953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html:29:
});
This whole l.16-29 should be simplified to:

assert_throws("InvalidStateError",
                function() {
                    faceDetector.detect(emptyImage);
                },
                "FaceDetector throws InvalidStateError when called with an empty
Image");

Same for l.37-50, it should simplify this code
quite a bit.

https://codereview.chromium.org/2441953002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right):

https://codereview.chromium.org/2441953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:30:
DOMException::create(InvalidStateError, "HTMLImageElement is broken."));
I suggest you line up the comments in l.30, 38 and 49 to the
DLOG(ERROR)s nearby. 
Also, in this case, both l. 28 and l.30 could say something like
"Failed to acquire Resource for Image".

https://codereview.chromium.org/2441953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:36:
DLOG(ERROR) << "Failed to convert ImageSource to blink::Image.";
s/ImageSource/ImageResource/

https://codereview.chromium.org/2441953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:62: <<
allocationSize << "bytes. limit = 16777216";
This number is [1], right? Consider pulling it here if 
so, don't write a hardcoded number.

[1]
https://cs.chromium.org/chromium/src/mojo/edk/system/broker_host.cc?q=mojo+sh...

[commit-bot: I haz the power, 2016-10-21 06:50:43]

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

[commit-bot: I haz the power, 2016-10-21 06:50:43]

Dry run: This issue passed the CQ dry run.

[xianglu, 2016-10-21 20:59:52]

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

[commit-bot: I haz the power, 2016-10-21 21:00:34]

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

[xianglu, 2016-10-21 21:08:30]

ptal.

https://codereview.chromium.org/2441953002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html
(right):

https://codereview.chromium.org/2441953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html:6:
// TODO(xianglu): Consider tests for image with only one dimension equal to
zero.
On 2016/10/21 04:41:45, mcasas wrote:
> "Consider adding ..."
> 
> Although there's no hard limit of 80 characters in JS files
> that I know of, it's a good idea to stick to it, especially since
> here you have two lines

Done.

https://codereview.chromium.org/2441953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html:12:
assert_equals(emptyImage.naturalWidth + emptyImage.naturalHeight, 0);
On 2016/10/21 04:41:46, mcasas wrote:
> Here you can just assert_equals(), no need to create
> and added test step such as l.11. Same elsewhere:
> since we're inside an async_test(), you can directly
> assert_*(), no need for extra step()s.

The difference is that without a step function, the test result will be TIMEOUT
when an assert fails. I might prefer to see a FAIL in that case.

https://codereview.chromium.org/2441953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html:17:
.then(faceDetectionResult => {
On 2016/10/21 04:41:45, mcasas wrote:
> Usually .then() and .catch() clauses are indented 4
> spaces to the right.

Done.

https://codereview.chromium.org/2441953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html:29:
});
On 2016/10/21 04:41:45, mcasas wrote:
> This whole l.16-29 should be simplified to:
> 
> assert_throws("InvalidStateError",
>                 function() {
>                     faceDetector.detect(emptyImage);
>                 },
>                 "FaceDetector throws InvalidStateError when called with an
empty
> Image");
> 
> Same for l.37-50, it should simplify this code
> quite a bit.

I think instead throwing an exception itself, faceDetector.detect(emptyImage)
actually returns a promise which throws an exception. In other words, the
exception is not thrown by detect().

https://codereview.chromium.org/2441953002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right):

https://codereview.chromium.org/2441953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:30:
DOMException::create(InvalidStateError, "HTMLImageElement is broken."));
On 2016/10/21 04:41:46, mcasas wrote:
> I suggest you line up the comments in l.30, 38 and 49 to the
> DLOG(ERROR)s nearby. 
> Also, in this case, both l. 28 and l.30 could say something like
> "Failed to acquire Resource for Image".

Done.

https://codereview.chromium.org/2441953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:36:
DLOG(ERROR) << "Failed to convert ImageSource to blink::Image.";
On 2016/10/21 04:41:46, mcasas wrote:
> s/ImageSource/ImageResource/

Done.

https://codereview.chromium.org/2441953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:62: <<
allocationSize << "bytes. limit = 16777216";
On 2016/10/21 04:41:46, mcasas wrote:
> This number is [1], right? Consider pulling it here if 
> so, don't write a hardcoded number.
> 
> [1]
>
https://cs.chromium.org/chromium/src/mojo/edk/system/broker_host.cc?q=mojo+sh...

Done.

[xianglu, 2016-10-21 21:10:26]

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

[commit-bot: I haz the power, 2016-10-21 21:10:34]

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

[xianglu, 2016-10-21 21:10:51]

Patchset #2 (id:40001) has been deleted

[xianglu, 2016-10-21 22:00:32]

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

[commit-bot: I haz the power, 2016-10-21 22:01:04]

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

[xianglu, 2016-10-21 22:23:30]

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

[xianglu, 2016-10-21 22:23:55]

Patchset #3 (id:80001) has been deleted

[xianglu, 2016-10-21 22:24:08]

Patchset #2 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-10-21 22:24:25]

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

[mcasas, 2016-10-21 23:43:16]

https://codereview.chromium.org/2441953002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right):

https://codereview.chromium.org/2441953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:62: <<
allocationSize << "bytes. limit = 16777216";
On 2016/10/21 21:08:30, xianglu wrote:
> On 2016/10/21 04:41:46, mcasas wrote:
> > This number is [1], right? Consider pulling it here if 
> > so, don't write a hardcoded number.
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/mojo/edk/system/broker_host.cc?q=mojo+sh...
> 
> Done.

Well, I meant using |kMaxSharedBufferSize|,
but now I see that is not static and is in another
compilation unit, what about something like:

    DLOG(ERROR) << "Requested allocation : " << allocationSize
                << "B, larger than |mojo::edk::kMaxSharedBufferSize| == 16MB ";

But then again, it might be better to give this info
in the rejection text -- think about a Web developer having 
a  gigantic HTMLImageElement, they might be able to react
to it on their side upon seeing such rejection message.

https://codereview.chromium.org/2441953002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html
(right):

https://codereview.chromium.org/2441953002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html:9:
// Returns a promise that will resolve to a rejection from detect method.
This reads a bit confusing, suggest instead:
// Returns a Promise that is resolve()d if detect() fails.

In light of that, maybe
s/makeTestPromiseWithImageUrl/detectFaceAndExpectError/
or something like that? 

To avoid having the suffix ..WithImageUrl, just rename
the parameter s/url/imageUrl/ and it'll be self explanatory :)

https://codereview.chromium.org/2441953002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right):

https://codereview.chromium.org/2441953002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:34: Image*
const blinkImage = imageResource->getImage();
I'm not 100% sure, but I think before doing this
we could check imageResource->errorOccurred() [1]
which will tell us if the image is broken. If so, that'd
be the appropriate place to reject with the 'broken
image' error message, and not in l.80. WDYT?

(If you do this, don't say 'Image is broken' in l.80,
but keep rejecting).

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Res...

[xianglu, 2016-10-22 02:19:09]

mcasas@ ptal.

https://codereview.chromium.org/2441953002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html
(right):

https://codereview.chromium.org/2441953002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html:9:
// Returns a promise that will resolve to a rejection from detect method.
On 2016/10/21 23:43:16, mcasas wrote:
> This reads a bit confusing, suggest instead:
> // Returns a Promise that is resolve()d if detect() fails.
> 
> In light of that, maybe
> s/makeTestPromiseWithImageUrl/detectFaceAndExpectError/
> or something like that? 
> 
> To avoid having the suffix ..WithImageUrl, just rename
> the parameter s/url/imageUrl/ and it'll be self explanatory :)

Done.

https://codereview.chromium.org/2441953002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right):

https://codereview.chromium.org/2441953002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:34: Image*
const blinkImage = imageResource->getImage();
On 2016/10/21 23:43:16, mcasas wrote:
> I'm not 100% sure, but I think before doing this
> we could check imageResource->errorOccurred() [1]
> which will tell us if the image is broken. If so, that'd
> be the appropriate place to reject with the 'broken
> image' error message, and not in l.80. WDYT?
> 
> (If you do this, don't say 'Image is broken' in l.80,
> but keep rejecting).
> 
> [1]
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Res...

I agree. However, the testing image ("imported/wpt/images/broken.png") is
actually one with decompression error, so it will pass this check. It seems in
this context "broken" is a little ambiguous now. I will rephrase the error
message.

For the check in l.27. I would add another test case with an appropriate testing
image in a future CL.

[xianglu, 2016-10-22 04:18:21]

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

[commit-bot: I haz the power, 2016-10-22 04:18:34]

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-10-22 06:45:37]

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

[commit-bot: I haz the power, 2016-10-22 06:45:38]

Dry run: This issue passed the CQ dry run.

[mcasas, 2016-10-24 17:38:56]

lgtm % my comment being addressed

https://codereview.chromium.org/2441953002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp (right):

https://codereview.chromium.org/2441953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp:62: <<
allocationSize << "bytes. limit = 16777216";
On 2016/10/21 23:43:16, mcasas wrote:
> On 2016/10/21 21:08:30, xianglu wrote:
> > On 2016/10/21 04:41:46, mcasas wrote:
> > > This number is [1], right? Consider pulling it here if 
> > > so, don't write a hardcoded number.
> > > 
> > > [1]
> > >
> >
>
https://cs.chromium.org/chromium/src/mojo/edk/system/broker_host.cc?q=mojo+sh...
> > 
> > Done.
> 
> Well, I meant using |kMaxSharedBufferSize|,
> but now I see that is not static and is in another
> compilation unit, what about something like:
> 
>     DLOG(ERROR) << "Requested allocation : " << allocationSize
>                 << "B, larger than |mojo::edk::kMaxSharedBufferSize| == 16MB
";
> 
> But then again, it might be better to give this info
> in the rejection text -- think about a Web developer having 
> a  gigantic HTMLImageElement, they might be able to react
> to it on their side upon seeing such rejection message.

This has not been addressed.

[xianglu, 2016-10-24 18:30:41]

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

[xianglu, 2016-10-24 18:31:02]

Patchset #3 (id:120001) has been deleted

[commit-bot: I haz the power, 2016-10-24 18:32:12]

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

[xianglu, 2016-10-24 18:33:04]

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

[xianglu, 2016-10-24 18:33:18]

Patchset #3 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-10-24 18:33:37]

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

[xianglu, 2016-10-24 18:33:39]

xianglu@chromium.org changed reviewers:
+ reillyg@chromium.org

[xianglu, 2016-10-24 18:34:38]

reillyg@ ptal

[xianglu, 2016-10-24 19:33:59]

xianglu@chromium.org changed reviewers:
+ haraken@chromium.org

[xianglu, 2016-10-24 19:34:00]

haraken@ ptal.

[haraken, 2016-10-24 19:43:07]

LGTM

I agree that we should improve the test coverage.

[commit-bot: I haz the power, 2016-10-24 20:03:03]

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

[commit-bot: I haz the power, 2016-10-24 20:03:04]

Dry run: This issue passed the CQ dry run.

[Reilly Grant (use Gerrit), 2016-10-24 20:07:23]

lgtm, though I'm worried about putting too many implementation details into
DOMExceptions. The message should be something that makes sense to a web
developer and helps them understand why their code isn't working. For example, I
don't know what I would do, as a web developer, in response to the "failed to
convert to skia" error.

https://codereview.chromium.org/2441953002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html
(right):

https://codereview.chromium.org/2441953002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html:16:
function makePromise () {
Defining this function after it is used feels weird to me. I'd write this as:

var tryFaceDetection = function() {
  ...
};
image.onload = tryFaceDetection
image.onerror = tryFaceDetection

[xianglu, 2016-10-24 22:09:04]

The CQ bit was checked by xianglu@chromium.org

[xianglu, 2016-10-24 22:09:04]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org,
haraken@chromium.org, reillyg@chromium.org
Link to the patchset: https://codereview.chromium.org/2441953002/#ps180001
(title: "reillyg@ comments on naming and description")

[commit-bot: I haz the power, 2016-10-24 22:09:46]

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

[xianglu, 2016-10-24 22:10:13]

The CQ bit was unchecked by xianglu@chromium.org

[xianglu, 2016-10-24 22:11:30]

Patchset #4 (id:180001) has been deleted

[xianglu, 2016-10-24 22:11:35]

The CQ bit was checked by xianglu@chromium.org

[xianglu, 2016-10-24 22:11:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, mcasas@chromium.org, reillyg@chromium.org
Link to the patchset: https://codereview.chromium.org/2441953002/#ps200001
(title: "reillyg@ comments on naming and description")

[commit-bot: I haz the power, 2016-10-24 22:12:26]

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-10-25 01:54:39]

Description was changed from

==========
Shape Detection: Add two layout tests for face detection.

This CL includes security tests for empty image and undecodable image.

BUG=646083
TEST=third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html
==========

to

==========
Shape Detection: Add two layout tests for face detection.

This CL includes security tests for empty image and undecodable image.

BUG=646083
TEST=third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html
==========

[commit-bot: I haz the power, 2016-10-25 01:54:41]

Committed patchset #4 (id:200001)

[commit-bot: I haz the power, 2016-10-25 01:58:05]

Description was changed from

==========
Shape Detection: Add two layout tests for face detection.

This CL includes security tests for empty image and undecodable image.

BUG=646083
TEST=third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html
==========

to

==========
Shape Detection: Add two layout tests for face detection.

This CL includes security tests for empty image and undecodable image.

BUG=646083
TEST=third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html

Committed: https://crrev.com/3995eb71799c911b01c05f8c128f8065636591c3
Cr-Commit-Position: refs/heads/master@{#427234}
==========

[commit-bot: I haz the power, 2016-10-25 01:58:06]

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