Skip foreign fetch when the skipServiceWorker flag is set on a request.

Skip foreign fetch when the skipServiceWorker flag is set on a request.

Foreign fetch used to completely ignore the skipServiceWorker flag. Just skipping
foreign fetch whenever skipServiceWorker is true would not be correct as the flag
is both set in situations where a request needs to bypass service workers, but
also when the renderer doesn't believe the client making the request is controlled
by a service worker. In that second case foreign fetch would still need to process
the request.

To fix this, this changes skipServiceWorker to an enum with three states, in
order to distinguish between skipping all service workers (for CORS preflight,
dev-tools skip service worker, force-refresh, etc) and skipping just the
controlling service worker.

This also makes sure that (for now) any request that requires a CORS preflight
will not get intercepted by foreign fetch.

BUG=540509, 604084
Committed: https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0
Cr-Commit-Position: refs/heads/master@{#403312}

[Marijn Kruisselbrink, 2016-06-28 00:29:18]

Description was changed from

==========
WIP make skipServiceWorker make sense for foreign fetch

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

to

==========
WIP make skipServiceWorker make sense for foreign fetch

Foreign fetch used to completely ignore the skipServiceWorker flag. Just
skipping
foreign fetch whenever skipServiceWorker is true would not be correct as the
flag
is both set in situations where a request needs to bypass service workers, but
also when the renderer doesn't believe the client making the request is
controlled
by a service worker. In that second case foreign fetch would still need to
process
the request.

To fix this, this changes skipServiceWorker to an enum with three states, in
order to distinguish between skipping all service workers (for CORS preflight,
dev-tools skip service worker, force-refresh, etc) and skipping just the
controlling service worker.

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

[Marijn Kruisselbrink, 2016-06-28 00:33:13]

Description was changed from

==========
WIP make skipServiceWorker make sense for foreign fetch

Foreign fetch used to completely ignore the skipServiceWorker flag. Just
skipping
foreign fetch whenever skipServiceWorker is true would not be correct as the
flag
is both set in situations where a request needs to bypass service workers, but
also when the renderer doesn't believe the client making the request is
controlled
by a service worker. In that second case foreign fetch would still need to
process
the request.

To fix this, this changes skipServiceWorker to an enum with three states, in
order to distinguish between skipping all service workers (for CORS preflight,
dev-tools skip service worker, force-refresh, etc) and skipping just the
controlling service worker.

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

to

==========
WIP make skipServiceWorker make sense for foreign fetch

Foreign fetch used to completely ignore the skipServiceWorker flag. Just
skipping
foreign fetch whenever skipServiceWorker is true would not be correct as the
flag
is both set in situations where a request needs to bypass service workers, but
also when the renderer doesn't believe the client making the request is
controlled
by a service worker. In that second case foreign fetch would still need to
process
the request.

To fix this, this changes skipServiceWorker to an enum with three states, in
order to distinguish between skipping all service workers (for CORS preflight,
dev-tools skip service worker, force-refresh, etc) and skipping just the
controlling service worker.

This also makes sure that (for now) any request that requires a CORS preflight
will not get intercepted by foreign fetch.

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

[Marijn Kruisselbrink, 2016-06-28 00:33:39]

Description was changed from

==========
WIP make skipServiceWorker make sense for foreign fetch

Foreign fetch used to completely ignore the skipServiceWorker flag. Just
skipping
foreign fetch whenever skipServiceWorker is true would not be correct as the
flag
is both set in situations where a request needs to bypass service workers, but
also when the renderer doesn't believe the client making the request is
controlled
by a service worker. In that second case foreign fetch would still need to
process
the request.

To fix this, this changes skipServiceWorker to an enum with three states, in
order to distinguish between skipping all service workers (for CORS preflight,
dev-tools skip service worker, force-refresh, etc) and skipping just the
controlling service worker.

This also makes sure that (for now) any request that requires a CORS preflight
will not get intercepted by foreign fetch.

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

to

==========
WIP make skipServiceWorker make sense for foreign fetch

Foreign fetch used to completely ignore the skipServiceWorker flag. Just
skipping
foreign fetch whenever skipServiceWorker is true would not be correct as the
flag
is both set in situations where a request needs to bypass service workers, but
also when the renderer doesn't believe the client making the request is
controlled
by a service worker. In that second case foreign fetch would still need to
process
the request.

To fix this, this changes skipServiceWorker to an enum with three states, in
order to distinguish between skipping all service workers (for CORS preflight,
dev-tools skip service worker, force-refresh, etc) and skipping just the
controlling service worker.

This also makes sure that (for now) any request that requires a CORS preflight
will not get intercepted by foreign fetch.

BUG=540509,604084
==========

[Marijn Kruisselbrink, 2016-06-28 00:34:08]

Description was changed from

==========
WIP make skipServiceWorker make sense for foreign fetch

Foreign fetch used to completely ignore the skipServiceWorker flag. Just
skipping
foreign fetch whenever skipServiceWorker is true would not be correct as the
flag
is both set in situations where a request needs to bypass service workers, but
also when the renderer doesn't believe the client making the request is
controlled
by a service worker. In that second case foreign fetch would still need to
process
the request.

To fix this, this changes skipServiceWorker to an enum with three states, in
order to distinguish between skipping all service workers (for CORS preflight,
dev-tools skip service worker, force-refresh, etc) and skipping just the
controlling service worker.

This also makes sure that (for now) any request that requires a CORS preflight
will not get intercepted by foreign fetch.

BUG=540509,604084
==========

to

==========
Skip foreign fetch when the skipServiceWorker flag is set on a request.

Foreign fetch used to completely ignore the skipServiceWorker flag. Just
skipping
foreign fetch whenever skipServiceWorker is true would not be correct as the
flag
is both set in situations where a request needs to bypass service workers, but
also when the renderer doesn't believe the client making the request is
controlled
by a service worker. In that second case foreign fetch would still need to
process
the request.

To fix this, this changes skipServiceWorker to an enum with three states, in
order to distinguish between skipping all service workers (for CORS preflight,
dev-tools skip service worker, force-refresh, etc) and skipping just the
controlling service worker.

This also makes sure that (for now) any request that requires a CORS preflight
will not get intercepted by foreign fetch.

BUG=540509,604084
==========

[Marijn Kruisselbrink, 2016-06-28 00:46:59]

mek@chromium.org changed reviewers:
+ horo@chromium.org, kinuko@chromium.org

[horo, 2016-06-28 12:10:02]

https://codereview.chromium.org/2105503002/diff/20001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2105503002/diff/20001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:588: :
blink::WebURLRequest::SkipServiceWorker::All,
What should happen in this case?
1. https://example1.com is controlled by a Service Worker (SW1).
2. <img src="//example2.com/test.png"> in https://example1.com.
3. SW1 doesn't call respondWith() for the request.
4. The server (example2.com) returns a redirect response (Location:
https://example3.com/test.png).
5. example3.com has a foreign fetch Service Worker (SW2).

Should the fetch handler of SW2 be fired?

https://codereview.chromium.org/2105503002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/2105503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:168:
request.setSkipServiceWorker(WebURLRequest::SkipServiceWorker::Controlling);
This code is executed when a CORS request was send to the SW but respondWith()
is not called.
In this case, we should skip all SW.

https://codereview.chromium.org/2105503002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/2105503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:244:
m_fallbackRequestForServiceWorker.setSkipServiceWorker(WebURLRequest::SkipServiceWorker::Controlling);
|m_fallbackRequestForServiceWorker| is used when a CORS request was send to the
SW but respondWith() is not called.
In this case, we should skip all SW.

[Marijn Kruisselbrink, 2016-06-28 17:06:13]

https://codereview.chromium.org/2105503002/diff/20001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2105503002/diff/20001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:588: :
blink::WebURLRequest::SkipServiceWorker::All,
On 2016/06/28 at 12:10:02, horo wrote:
> What should happen in this case?
> 1. https://example1.com is controlled by a Service Worker (SW1).
> 2. <img src="//example2.com/test.png"> in https://example1.com.
> 3. SW1 doesn't call respondWith() for the request.
> 4. The server (example2.com) returns a redirect response (Location:
https://example3.com/test.png).
> 5. example3.com has a foreign fetch Service Worker (SW2).
> 
> Should the fetch handler of SW2 be fired?

This reminds me that I still need to write the integration with the fetch spec
for foreign fetch. But no, I think at least initially I don't think servers
redirecting should ever end up back in a service worker (in other worse, I'm
planning on using the same "skip-service-worker-flag" the fetch spec uses to
skip regular service workers to also skip foreign fetch service workers; fetch
always sets that flag on requests that weren't initially handled by a service
worker, so no redirects will end up being handled by a service worker).

https://codereview.chromium.org/2105503002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/2105503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:168:
request.setSkipServiceWorker(WebURLRequest::SkipServiceWorker::Controlling);
On 2016/06/28 at 12:10:02, horo wrote:
> This code is executed when a CORS request was send to the SW but respondWith()
is not called.
> In this case, we should skip all SW.

Also see my reply in DocumentThreadableLoader, but no, in this case we should
not skip foreign fetch service workers since CORS requests generally are exactly
the requests we do want foreign fetch to intercept. The layout test I added that
injects a cross origin script tag in a service worker controlled page exercises
this code, and I do expect that script tag to be intercepted by foreign fetch.

https://codereview.chromium.org/2105503002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/2105503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:244:
m_fallbackRequestForServiceWorker.setSkipServiceWorker(WebURLRequest::SkipServiceWorker::Controlling);
On 2016/06/28 at 12:10:02, horo wrote:
> |m_fallbackRequestForServiceWorker| is used when a CORS request was send to
the SW but respondWith() is not called.
> In this case, we should skip all SW.

Actually that is precisely the case where I don't want to skip foreign fetch
service workers. CORS requests in general are exactly the requests foreign fetch
wants to intercept, even if made from a controlled page. It's only CORS requests
that involve preflights that for now foreign fetch won't be able to handle; so
the preflights and requests after that are separately set as skipping all
service workers (in CrossOriginAccessControl.cpp for the preflight, and below in
loadActualRequest for the actual request). Requests that don't end up with a
preflight still need to go through foreign fetch.

[horo, 2016-06-29 04:06:14]

https://codereview.chromium.org/2105503002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/2105503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:244:
m_fallbackRequestForServiceWorker.setSkipServiceWorker(WebURLRequest::SkipServiceWorker::Controlling);
On 2016/06/28 17:06:13, Marijn Kruisselbrink wrote:
> On 2016/06/28 at 12:10:02, horo wrote:
> > |m_fallbackRequestForServiceWorker| is used when a CORS request was send to
> the SW but respondWith() is not called.
> > In this case, we should skip all SW.
> 
> Actually that is precisely the case where I don't want to skip foreign fetch
> service workers. CORS requests in general are exactly the requests foreign
fetch
> wants to intercept, even if made from a controlled page. It's only CORS
requests
> that involve preflights that for now foreign fetch won't be able to handle; so
> the preflights and requests after that are separately set as skipping all
> service workers (in CrossOriginAccessControl.cpp for the preflight, and below
in
> loadActualRequest for the actual request). Requests that don't end up with a
> preflight still need to go through foreign fetch.

Ah got it.
Please write comments about it and that currently the foreign fetch event
fallback response's wasFallbackRequiredByServiceWorker flag is not set
(http://crbug.com/604084).

[Marijn Kruisselbrink, 2016-06-29 17:12:15]

https://codereview.chromium.org/2105503002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/2105503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:244:
m_fallbackRequestForServiceWorker.setSkipServiceWorker(WebURLRequest::SkipServiceWorker::Controlling);
On 2016/06/29 at 04:06:14, horo wrote:
> On 2016/06/28 17:06:13, Marijn Kruisselbrink wrote:
> > On 2016/06/28 at 12:10:02, horo wrote:
> > > |m_fallbackRequestForServiceWorker| is used when a CORS request was send
to
> > the SW but respondWith() is not called.
> > > In this case, we should skip all SW.
> > 
> > Actually that is precisely the case where I don't want to skip foreign fetch
> > service workers. CORS requests in general are exactly the requests foreign
fetch
> > wants to intercept, even if made from a controlled page. It's only CORS
requests
> > that involve preflights that for now foreign fetch won't be able to handle;
so
> > the preflights and requests after that are separately set as skipping all
> > service workers (in CrossOriginAccessControl.cpp for the preflight, and
below in
> > loadActualRequest for the actual request). Requests that don't end up with a
> > preflight still need to go through foreign fetch.
> 
> Ah got it.
> Please write comments about it and that currently the foreign fetch event
fallback response's wasFallbackRequiredByServiceWorker flag is not set
(http://crbug.com/604084).

Done

[horo, 2016-06-29 22:45:55]

lgtm

[kinuko, 2016-06-30 09:38:22]

lgtm with some nits.

https://codereview.chromium.org/2105503002/diff/40001/content/common/service_...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/2105503002/diff/40001/content/common/service_...
content/common/service_worker/service_worker_types.h:100: // Whether or not a
request should be handled by a ServiceWorker.
nit... this enum seems to indicate that whether the request should be *skipped*
by a ServiceWorker to me, if we interpret this flag as 'should it be handled? --
NONE/ALL etc' things start to feel very confusing.

(here and also for the same comment in resource_request.h)

https://codereview.chromium.org/2105503002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/ResourceRequest.h (right):

https://codereview.chromium.org/2105503002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ResourceRequest.h:200: // Indicates
if the request should be handled by a ServiceWorker.
ditto

https://codereview.chromium.org/2105503002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebURLRequest.h (right):

https://codereview.chromium.org/2105503002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebURLRequest.h:144: // Whether or not a
request should be handled by a ServiceWorker.
ditto.  Also-- started to wonder if we could have just defined single enum in
public/platform and use it in content too... having dup'ed same enums at
multiple places with static_assert is awkward.

[Marijn Kruisselbrink, 2016-06-30 17:41:54]

https://codereview.chromium.org/2105503002/diff/40001/content/common/service_...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/2105503002/diff/40001/content/common/service_...
content/common/service_worker/service_worker_types.h:100: // Whether or not a
request should be handled by a ServiceWorker.
On 2016/06/30 at 09:38:21, kinuko wrote:
> nit... this enum seems to indicate that whether the request should be
*skipped* by a ServiceWorker to me, if we interpret this flag as 'should it be
handled? -- NONE/ALL etc' things start to feel very confusing.
> 
> (here and also for the same comment in resource_request.h)

Hmm, good point. Rephrased all these comments to make it match the meaning of
the enum better.

https://codereview.chromium.org/2105503002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/ResourceRequest.h (right):

https://codereview.chromium.org/2105503002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ResourceRequest.h:200: // Indicates
if the request should be handled by a ServiceWorker.
On 2016/06/30 at 09:38:21, kinuko wrote:
> ditto

Done

https://codereview.chromium.org/2105503002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebURLRequest.h (right):

https://codereview.chromium.org/2105503002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebURLRequest.h:144: // Whether or not a
request should be handled by a ServiceWorker.
On 2016/06/30 at 09:38:22, kinuko wrote:
> ditto.  Also-- started to wonder if we could have just defined single enum in
public/platform and use it in content too... having dup'ed same enums at
multiple places with static_assert is awkward.

We could... we'd need to move the enums we'd want to share to some header file
that doesn't also declare classes for content/browser/DEPS to be able to allow
usage of them of course, but that would be easy. All the different classes we
have to represent requests (and all the conversions between them) seem pretty
awkward too, although I'm sure there are good reasons for those.

Anyway, at least the enum part seems like something that would at least change
in some way when more stuff gets mojoified. Just not sure what the some way will
be (hopefully not just adding yet another enum and more conversions). It just
doesn't seem worth the effort now to unify all these enums when it is going to
change in some way anyway. But maybe mojoifying this is far enough away that
unifying enums first is going to make sense anyway, I don't know.

[Marijn Kruisselbrink, 2016-06-30 17:42:50]

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

[Marijn Kruisselbrink, 2016-06-30 17:42:51]

+palmer for IPC owners (content/common/resource_messages.h)

[mmenke, 2016-06-30 17:48:35]

Description was changed from

==========
Skip foreign fetch when the skipServiceWorker flag is set on a request.

Foreign fetch used to completely ignore the skipServiceWorker flag. Just
skipping
foreign fetch whenever skipServiceWorker is true would not be correct as the
flag
is both set in situations where a request needs to bypass service workers, but
also when the renderer doesn't believe the client making the request is
controlled
by a service worker. In that second case foreign fetch would still need to
process
the request.

To fix this, this changes skipServiceWorker to an enum with three states, in
order to distinguish between skipping all service workers (for CORS preflight,
dev-tools skip service worker, force-refresh, etc) and skipping just the
controlling service worker.

This also makes sure that (for now) any request that requires a CORS preflight
will not get intercepted by foreign fetch.

BUG=540509,604084
==========

to

==========
Skip foreign fetch when the skipServiceWorker flag is set on a request.

Foreign fetch used to completely ignore the skipServiceWorker flag. Just
skipping
foreign fetch whenever skipServiceWorker is true would not be correct as the
flag
is both set in situations where a request needs to bypass service workers, but
also when the renderer doesn't believe the client making the request is
controlled
by a service worker. In that second case foreign fetch would still need to
process
the request.

To fix this, this changes skipServiceWorker to an enum with three states, in
order to distinguish between skipping all service workers (for CORS preflight,
dev-tools skip service worker, force-refresh, etc) and skipping just the
controlling service worker.

This also makes sure that (for now) any request that requires a CORS preflight
will not get intercepted by foreign fetch.

BUG=540509,604084
==========

[palmer, 2016-06-30 20:59:42]

IPC security LGTM

[Marijn Kruisselbrink, 2016-06-30 21:00:59]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2016-06-30 21:01:00]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
horo@chromium.org
Link to the patchset: https://codereview.chromium.org/2105503002/#ps60001
(title: "rephrase SkipServiceWorker comments.")

[commit-bot: I haz the power, 2016-06-30 21:01:50]

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-06-30 22:17:40]

Description was changed from

==========
Skip foreign fetch when the skipServiceWorker flag is set on a request.

Foreign fetch used to completely ignore the skipServiceWorker flag. Just
skipping
foreign fetch whenever skipServiceWorker is true would not be correct as the
flag
is both set in situations where a request needs to bypass service workers, but
also when the renderer doesn't believe the client making the request is
controlled
by a service worker. In that second case foreign fetch would still need to
process
the request.

To fix this, this changes skipServiceWorker to an enum with three states, in
order to distinguish between skipping all service workers (for CORS preflight,
dev-tools skip service worker, force-refresh, etc) and skipping just the
controlling service worker.

This also makes sure that (for now) any request that requires a CORS preflight
will not get intercepted by foreign fetch.

BUG=540509,604084
==========

to

==========
Skip foreign fetch when the skipServiceWorker flag is set on a request.

Foreign fetch used to completely ignore the skipServiceWorker flag. Just
skipping
foreign fetch whenever skipServiceWorker is true would not be correct as the
flag
is both set in situations where a request needs to bypass service workers, but
also when the renderer doesn't believe the client making the request is
controlled
by a service worker. In that second case foreign fetch would still need to
process
the request.

To fix this, this changes skipServiceWorker to an enum with three states, in
order to distinguish between skipping all service workers (for CORS preflight,
dev-tools skip service worker, force-refresh, etc) and skipping just the
controlling service worker.

This also makes sure that (for now) any request that requires a CORS preflight
will not get intercepted by foreign fetch.

BUG=540509,604084
==========

[commit-bot: I haz the power, 2016-06-30 22:17:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-30 22:18:02]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 22:20:18]

Description was changed from

==========
Skip foreign fetch when the skipServiceWorker flag is set on a request.

Foreign fetch used to completely ignore the skipServiceWorker flag. Just
skipping
foreign fetch whenever skipServiceWorker is true would not be correct as the
flag
is both set in situations where a request needs to bypass service workers, but
also when the renderer doesn't believe the client making the request is
controlled
by a service worker. In that second case foreign fetch would still need to
process
the request.

To fix this, this changes skipServiceWorker to an enum with three states, in
order to distinguish between skipping all service workers (for CORS preflight,
dev-tools skip service worker, force-refresh, etc) and skipping just the
controlling service worker.

This also makes sure that (for now) any request that requires a CORS preflight
will not get intercepted by foreign fetch.

BUG=540509,604084
==========

to

==========
Skip foreign fetch when the skipServiceWorker flag is set on a request.

Foreign fetch used to completely ignore the skipServiceWorker flag. Just
skipping
foreign fetch whenever skipServiceWorker is true would not be correct as the
flag
is both set in situations where a request needs to bypass service workers, but
also when the renderer doesn't believe the client making the request is
controlled
by a service worker. In that second case foreign fetch would still need to
process
the request.

To fix this, this changes skipServiceWorker to an enum with three states, in
order to distinguish between skipping all service workers (for CORS preflight,
dev-tools skip service worker, force-refresh, etc) and skipping just the
controlling service worker.

This also makes sure that (for now) any request that requires a CORS preflight
will not get intercepted by foreign fetch.

BUG=540509,604084

Committed: https://crrev.com/2afc5027b2ed43ddd6f26193ad8dc577a2d4b2f0
Cr-Commit-Position: refs/heads/master@{#403312}
==========

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

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