Use ResourceRequestInfo::GetWebContents in DownloadRequestLimiter

Use ResourceRequestInfo::GetWebContents in DownloadRequestLimiter

This CL makes DownloadRequestLimiter use a the callback returned by
ResourceRequestInfo::GetWebContents instead of using RFH + RPH IDs. This allows
most download browser_tests to work with PlzNavigate enabled.

BUG=504347
Committed: https://crrev.com/b830caedd400b87a3394935f4a7304ba53f4d427
Cr-Commit-Position: refs/heads/master@{#362699}

[clamy, 2015-11-24 15:16:10]

Description was changed from

==========
Use ResourceRequestInfo::GetWebContents in DownloadRequestLimiter

This CL makes DownloadRequestLimiter use a the callback returned by
ResourceRequestInfo::GetWebContents instead of using RFH + RPH IDs. This allows
most download browser_tests to work with PlzNavigate enabled.

BUG=504347
==========

to

==========
Use ResourceRequestInfo::GetWebContents in DownloadRequestLimiter

This CL makes DownloadRequestLimiter use a the callback returned by
ResourceRequestInfo::GetWebContents instead of using RFH + RPH IDs. This allows
most download browser_tests to work with PlzNavigate enabled.

BUG=504347
==========

[clamy, 2015-11-24 15:16:10]

clamy@chromium.org changed reviewers:
+ davidben@chromium.org, nasko@chromium.org

[clamy, 2015-11-24 15:16:47]

@nasko, davidben: PTAL. This follows yesterday's patch about the WebContents
callback.

[davidben, 2015-11-24 15:51:16]

https://codereview.chromium.org/1467563002/diff/80001/chrome/browser/android/...
File chrome/browser/android/download/mock_download_controller_android.cc
(right):

https://codereview.chromium.org/1467563002/diff/80001/chrome/browser/android/...
chrome/browser/android/download/mock_download_controller_android.cc:41:
WebContents* web_contents,
content::WebContents (try bots are unhappy)

https://codereview.chromium.org/1467563002/diff/80001/chrome/browser/android/...
File chrome/browser/android/download/mock_download_controller_android.h (right):

https://codereview.chromium.org/1467563002/diff/80001/chrome/browser/android/...
chrome/browser/android/download/mock_download_controller_android.h:39:
WebContents* web_contents,
content::WebContents (try bots unhappy)

https://codereview.chromium.org/1467563002/diff/80001/chrome/browser/download...
File chrome/browser/download/download_resource_throttle.cc (right):

https://codereview.chromium.org/1467563002/diff/80001/chrome/browser/download...
chrome/browser/download/download_resource_throttle.cc:33:
info->limiter->CanDownload(info->web_contents_getter, info->url,
Since we're already on the UI thread, I'd probably just cash in the callback now
so the rest of the code needn't deal with it. (I'm thinking dealing with the
callback should be something only code that straddles thread boundaries should
care about. Beyond that, we use proper WebContents pointers and such.)

https://codereview.chromium.org/1467563002/diff/80001/chrome/browser/download...
File chrome/browser/download/download_resource_throttle_unittest.cc (right):

https://codereview.chromium.org/1467563002/diff/80001/chrome/browser/download...
chrome/browser/download/download_resource_throttle_unittest.cc:89: limiter_,
base::Bind(&GetWebContents, process_id, render_view_id),
There's a tab_util::GetWebContentsByID you could use instead.

https://codereview.chromium.org/1467563002/diff/80001/content/browser/android...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1467563002/diff/80001/content/browser/android...
content/browser/android/download_controller_android_impl.cc:187: if
(!web_contents) {
Nit: I'd probably have moved this into the caller. (Thinking about this class on
its own, it's a little weird to ask the DownloadControllerAndroidImpl "hey, I'd
like to acquire file access permission on <does not exist>".)

https://codereview.chromium.org/1467563002/diff/80001/content/public/browser/...
File content/public/browser/resource_request_info.h (right):

https://codereview.chromium.org/1467563002/diff/80001/content/public/browser/...
content/public/browser/resource_request_info.h:68: // The child process unique
ID of the requestor.
Restore the "Use GetWebContentsForRequest instead." comments here and below?

[asanka, 2015-11-24 16:13:19]

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

[asanka, 2015-11-24 16:13:19]

CC += svaldez: This is relevant to the WebContents free download requests work
you are doing.

Folks, could you keep the inner OWNERS in the loop for this sort of change?
Randy has already asked you on behalf of downloads before since there's ongoing
work here that's affected by this sort of change.

Also, this change passes around a callback that has exactly two possible return
values: nullptr, and a unique WebContents*. The choice between the two depends
on whether the WebContents is alive or not. Why aren't you using a WeakPtr<>
since this is exactly that is?

https://codereview.chromium.org/1467563002/diff/80001/chrome/browser/download...
File chrome/browser/download/download_request_limiter.h (right):

https://codereview.chromium.org/1467563002/diff/80001/chrome/browser/download...
chrome/browser/download/download_request_limiter.h:186: const
base::Callback<content::WebContents*(void)>& web_contents_getter,
Instead of a bunch of naked base::Callback<>s, could you define a type somewhere
and use it here? That way you can describe what the callback does and its
guarantees. A naked callback doesn't imply anything about thread restrictions,
lifetime guarantees, validity etc.. (E.g. a callback that's bound to a method on
an object referenced via a weak pointer will become null when the weak reference
is invalidated. It's not sufficient that the callback be bound. For a callback
to be valid, the weak reference to the object against which the method call is
indirected must also be valid).

Everywhere you use the callback, you are assume that it's non-null with no API
contract anywhere that it is.

https://codereview.chromium.org/1467563002/diff/80001/content/browser/loader/...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/1467563002/diff/80001/content/browser/loader/...
content/browser/loader/resource_request_info_impl.h:73:
base::Callback<WebContents*(void)> GetWebContentsForRequest() const override;
Nit: This method doesn't really return a WebContents for the request. If you are
going to continue to use it, could you call this something else, like
GetWebContentsGetter() or something?

[davidben, 2015-11-24 16:18:27]

On 2015/11/24 16:13:19, asanka wrote:
> Also, this change passes around a callback that has exactly two possible
return
> values: nullptr, and a unique WebContents*. The choice between the two depends
> on whether the WebContents is alive or not. Why aren't you using a WeakPtr<>
> since this is exactly that is?

The object gets minted on the wrong thread to be a WeakPtr. Short of an IO
thread map, I don't believe that's feasible.

[davidben, 2015-11-24 16:23:56]

I don't think this actually affects the downloads resumption stuff. A
WebContents-free download should never have that callback return non-null. This
isn't really any different from the child IDs in that respect. I would really
rather that the callback never return anything for a download (per the proposed
ownership rule). Right now it does, but that's only because the same would
happen for child IDs.

This is purely to abstract away the child ID lookup in a way that unblocks
PlzNavigate without having to overhaul everything else in the process.

[clamy, 2015-11-24 16:26:30]

On 2015/11/24 16:18:27, davidben (slow) wrote:
> On 2015/11/24 16:13:19, asanka wrote:
> > Also, this change passes around a callback that has exactly two possible
> return
> > values: nullptr, and a unique WebContents*. The choice between the two
depends
> > on whether the WebContents is alive or not. Why aren't you using a WeakPtr<>
> > since this is exactly that is?
> 
> The object gets minted on the wrong thread to be a WeakPtr. Short of an IO
> thread map, I don't believe that's feasible.

Also WebContents does not support WeakPtrs.

As for keeping OWNERS of download, this is a newly created patch which was sent
in review 1h ago, based on a patch that landed 5 hours ago. Since there are
changes to the content/ public interface, I like to get content/public owners
opinion first, but I would have sent it to download OWNERS right after that :).

[asanka, 2015-11-24 16:49:24]

On 2015/11/24 at 16:26:30, clamy wrote:
> On 2015/11/24 16:18:27, davidben (slow) wrote:
> > On 2015/11/24 16:13:19, asanka wrote:
> > > Also, this change passes around a callback that has exactly two possible
> > return
> > > values: nullptr, and a unique WebContents*. The choice between the two
depends
> > > on whether the WebContents is alive or not. Why aren't you using a
WeakPtr<>
> > > since this is exactly that is?
> > 
> > The object gets minted on the wrong thread to be a WeakPtr. Short of an IO
> > thread map, I don't believe that's feasible.
> 
> Also WebContents does not support WeakPtrs.

A class doesn't necessarily need to support WeakPtrs. The WeakPtrFactory doesn't
need to be a member of the class as long as the validity of the minted WeakPtr
doesn't extend beyond the lifetime of the pointer. There are valid objections to
having a public class expose support for weak references. However in this case
we are talking about weakly binding a request (on the IO thread) to a
WebContents (on the UI thread). Such a binding is outside the scope of the
public WebContents interface, but certainly it's within the scope of what you
are doing here.

Either way, if it's infeasible to mint a WeakPtr on the correct thread, then the
issue is moot.

> As for keeping OWNERS of download, this is a newly created patch which was
sent in review 1h ago, based on a patch that landed 5 hours ago. Since there are
changes to the content/ public interface, I like to get content/public owners
opinion first, but I would have sent it to download OWNERS right after that :).

Ah. Thanks. I got a bit paranoid when I saw this go by.

[asanka, 2015-11-24 17:11:51]

On 2015/11/24 at 16:23:56, davidben wrote:
> I don't think this actually affects the downloads resumption stuff. A
WebContents-free download should never have that callback return non-null.

That's technically true, but only because the method of resumption is gated on
whether the WebContents is non-null at the time the resumption is attempted. If
it's null, then we go the WebContents-free route. However, whether the
WebContents is around or not definitely affects resumption.

> This isn't really any different from the child IDs in that respect. I would
really rather that the callback never return anything for a download (per the
proposed ownership rule). Right now it does, but that's only because the same
would happen for child IDs.
> 

At least for the moment, resumption is affected by the circumstances under which
a request was issued (E.g. security policy and whether some server certificate
is allowed or not). By permanently detaching the WebContents from the request as
soon as it's discovered to be a download (which we often do at the end of a
redirect chain after the final response headers have been received etc.. as you
know) we are also detaching the request from this state. I think there's some
more reasoning that needs to happen before we are (or at least I am) convinced
that we understand the consequences of resuming a request in this manner. This
is probably a discussion that should be had elsewhere.

> This is purely to abstract away the child ID lookup in a way that unblocks
PlzNavigate without having to overhaul everything else in the process.

Sure. But the fact that the semantics of a failure to look up a WebContents* is
preserved due to an artifact of the implementation rather than an explicit API
contract. Which is why I requested that the behavior of the WebContents getter
be documented as a part of the API rather than be left to be discovered by
looking through all binding sites.

[clamy, 2015-11-25 14:02:02]

Thanks!

https://codereview.chromium.org/1467563002/diff/80001/chrome/browser/android/...
File chrome/browser/android/download/mock_download_controller_android.cc
(right):

https://codereview.chromium.org/1467563002/diff/80001/chrome/browser/android/...
chrome/browser/android/download/mock_download_controller_android.cc:41:
WebContents* web_contents,
On 2015/11/24 15:51:16, davidben (slow) wrote:
> content::WebContents (try bots are unhappy)

Done.

https://codereview.chromium.org/1467563002/diff/80001/chrome/browser/android/...
File chrome/browser/android/download/mock_download_controller_android.h (right):

https://codereview.chromium.org/1467563002/diff/80001/chrome/browser/android/...
chrome/browser/android/download/mock_download_controller_android.h:39:
WebContents* web_contents,
On 2015/11/24 15:51:16, davidben (slow) wrote:
> content::WebContents (try bots unhappy)

Done.

https://codereview.chromium.org/1467563002/diff/80001/chrome/browser/download...
File chrome/browser/download/download_request_limiter.h (right):

https://codereview.chromium.org/1467563002/diff/80001/chrome/browser/download...
chrome/browser/download/download_request_limiter.h:186: const
base::Callback<content::WebContents*(void)>& web_contents_getter,
On 2015/11/24 16:13:19, asanka wrote:
> Instead of a bunch of naked base::Callback<>s, could you define a type
somewhere
> and use it here? That way you can describe what the callback does and its
> guarantees. A naked callback doesn't imply anything about thread restrictions,
> lifetime guarantees, validity etc.. (E.g. a callback that's bound to a method
on
> an object referenced via a weak pointer will become null when the weak
reference
> is invalidated. It's not sufficient that the callback be bound. For a callback
> to be valid, the weak reference to the object against which the method call is
> indirected must also be valid).
> 
> Everywhere you use the callback, you are assume that it's non-null with no API
> contract anywhere that it is.

I went back to having a typedef in ResourceRequestInfo, with additional
precision about the callback.

https://codereview.chromium.org/1467563002/diff/80001/chrome/browser/download...
File chrome/browser/download/download_resource_throttle.cc (right):

https://codereview.chromium.org/1467563002/diff/80001/chrome/browser/download...
chrome/browser/download/download_resource_throttle.cc:33:
info->limiter->CanDownload(info->web_contents_getter, info->url,
On 2015/11/24 15:51:16, davidben (slow) wrote:
> Since we're already on the UI thread, I'd probably just cash in the callback
now
> so the rest of the code needn't deal with it. (I'm thinking dealing with the
> callback should be something only code that straddles thread boundaries should
> care about. Beyond that, we use proper WebContents pointers and such.)

If I do that though I have to get back a RPH + RFH ID so that it can be bound in
the callback in DownloadRequestLimiter::CanDownload. Since there does not seem
to be guarantees on when DownloadRequestLimiter::OnCanDownloadDecided will be
executed, a pointer to WebContents may go out of scope before that. Is that
better than keeping the callback?

https://codereview.chromium.org/1467563002/diff/80001/chrome/browser/download...
File chrome/browser/download/download_resource_throttle_unittest.cc (right):

https://codereview.chromium.org/1467563002/diff/80001/chrome/browser/download...
chrome/browser/download/download_resource_throttle_unittest.cc:89: limiter_,
base::Bind(&GetWebContents, process_id, render_view_id),
On 2015/11/24 15:51:16, davidben (slow) wrote:
> There's a tab_util::GetWebContentsByID you could use instead.

Done.

https://codereview.chromium.org/1467563002/diff/80001/content/browser/android...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1467563002/diff/80001/content/browser/android...
content/browser/android/download_controller_android_impl.cc:187: if
(!web_contents) {
On 2015/11/24 15:51:16, davidben (slow) wrote:
> Nit: I'd probably have moved this into the caller. (Thinking about this class
on
> its own, it's a little weird to ask the DownloadControllerAndroidImpl "hey,
I'd
> like to acquire file access permission on <does not exist>".)

Done. Note that the caller now run the callback synchronously in that case.

https://codereview.chromium.org/1467563002/diff/80001/content/browser/loader/...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/1467563002/diff/80001/content/browser/loader/...
content/browser/loader/resource_request_info_impl.h:73:
base::Callback<WebContents*(void)> GetWebContentsForRequest() const override;
On 2015/11/24 16:13:19, asanka wrote:
> Nit: This method doesn't really return a WebContents for the request. If you
are
> going to continue to use it, could you call this something else, like
> GetWebContentsGetter() or something?

Done.

https://codereview.chromium.org/1467563002/diff/80001/content/public/browser/...
File content/public/browser/resource_request_info.h (right):

https://codereview.chromium.org/1467563002/diff/80001/content/public/browser/...
content/public/browser/resource_request_info.h:68: // The child process unique
ID of the requestor.
On 2015/11/24 15:51:16, davidben (slow) wrote:
> Restore the "Use GetWebContentsForRequest instead." comments here and below?

Done.

[asanka, 2015-11-25 19:33:02]

I chatted with davidben about the scope of this CL and what to do about
WebContents references. I'm happy with this CL and hence LGTM % nits and happy
bots.

https://codereview.chromium.org/1467563002/diff/100001/chrome/browser/downloa...
File chrome/browser/download/download_resource_throttle.cc (right):

https://codereview.chromium.org/1467563002/diff/100001/chrome/browser/downloa...
chrome/browser/download/download_resource_throttle.cc:55:
OnAcquireFileAccessPermissionDone(false);
Need to pass info.

https://codereview.chromium.org/1467563002/diff/100001/content/public/browser...
File content/public/browser/resource_request_info.h (right):

https://codereview.chromium.org/1467563002/diff/100001/content/public/browser...
content/public/browser/resource_request_info.h:57: // always be used, but it may
return nullptr : if the info used to
Nit: stray ':'

https://codereview.chromium.org/1467563002/diff/100001/content/public/browser...
content/public/browser/resource_request_info.h:62: using
WebContentsGetterOnUIThread = base::Callback<WebContents*(void)>;
Nit: WebContentsGetter should also be sufficient since the thread restriction is
mentioned here.

https://codereview.chromium.org/1467563002/diff/100001/content/public/browser...
content/public/browser/resource_request_info.h:70: virtual
WebContentsGetterOnUIThread GetWebContentsGetterForRequest()
Nit: GetWebContentsGetter is also sufficient.

https://codereview.chromium.org/1467563002/diff/100001/content/public/browser...
content/public/browser/resource_request_info.h:77: // Use
GetWebContentsGetterForRequest instead.
Nit: These comments assume that the respective methods are solely used for
eventually getting at a WebContents, which appears to not be the case at least
currently.

[clamy, 2015-11-27 13:29:06]

Thanks!

https://codereview.chromium.org/1467563002/diff/100001/chrome/browser/downloa...
File chrome/browser/download/download_resource_throttle.cc (right):

https://codereview.chromium.org/1467563002/diff/100001/chrome/browser/downloa...
chrome/browser/download/download_resource_throttle.cc:55:
OnAcquireFileAccessPermissionDone(false);
On 2015/11/25 19:33:02, asanka wrote:
> Need to pass info.

Done.

https://codereview.chromium.org/1467563002/diff/100001/content/public/browser...
File content/public/browser/resource_request_info.h (right):

https://codereview.chromium.org/1467563002/diff/100001/content/public/browser...
content/public/browser/resource_request_info.h:57: // always be used, but it may
return nullptr : if the info used to
On 2015/11/25 19:33:02, asanka wrote:
> Nit: stray ':'

Done.

https://codereview.chromium.org/1467563002/diff/100001/content/public/browser...
content/public/browser/resource_request_info.h:62: using
WebContentsGetterOnUIThread = base::Callback<WebContents*(void)>;
On 2015/11/25 19:33:02, asanka wrote:
> Nit: WebContentsGetter should also be sufficient since the thread restriction
is
> mentioned here.

Done.

https://codereview.chromium.org/1467563002/diff/100001/content/public/browser...
content/public/browser/resource_request_info.h:70: virtual
WebContentsGetterOnUIThread GetWebContentsGetterForRequest()
On 2015/11/25 19:33:02, asanka wrote:
> Nit: GetWebContentsGetter is also sufficient.

Done.

https://codereview.chromium.org/1467563002/diff/100001/content/public/browser...
content/public/browser/resource_request_info.h:77: // Use
GetWebContentsGetterForRequest instead.
On 2015/11/25 19:33:02, asanka wrote:
> Nit: These comments assume that the respective methods are solely used for
> eventually getting at a WebContents, which appears to not be the case at least
> currently.

Done.

[clamy, 2015-12-01 10:46:11]

clamy@chromium.org changed reviewers:
+ yfriedman@chromium.org

[clamy, 2015-12-01 10:46:49]

@davidben: friendly ping :)
@yfriedman: PTAL at the changes in chrome/browser/android.

[davidben, 2015-12-01 22:15:15]

lgtm

https://codereview.chromium.org/1467563002/diff/120001/chrome/browser/downloa...
File chrome/browser/download/download_request_limiter.h (right):

https://codereview.chromium.org/1467563002/diff/120001/chrome/browser/downloa...
chrome/browser/download/download_request_limiter.h:187: web_contents_getter,
[Hrmf. I was going to suggest this ought to be a WebContents*, but it seems
CanDownload's behavior if the WebContents is destroyed first is far from clear.
Oh well. I can sort that out afterwards I guess.]

https://codereview.chromium.org/1467563002/diff/120001/content/browser/loader...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/1467563002/diff/120001/content/browser/loader...
content/browser/loader/resource_request_info_impl.h:73:
base::Callback<WebContents*(void)> GetWebContentsGetterForRequest()
Nit: I suppose if we're using a typedef, this should return WebContentsGetter.
*shrug* Sorry about the back-and-forth there.

[Yaron, 2015-12-01 22:37:17]

lgtm

[clamy, 2015-12-02 09:33:18]

clamy@chromium.org changed reviewers:
+ jochen@chromium.org

[clamy, 2015-12-02 09:34:00]

Thanks!

@jochen: PTAL at the changes in
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc.

https://codereview.chromium.org/1467563002/diff/120001/chrome/browser/downloa...
File chrome/browser/download/download_request_limiter.h (right):

https://codereview.chromium.org/1467563002/diff/120001/chrome/browser/downloa...
chrome/browser/download/download_request_limiter.h:187: web_contents_getter,
On 2015/12/01 22:15:15, davidben (behind on reviews) wrote:
> [Hrmf. I was going to suggest this ought to be a WebContents*, but it seems
> CanDownload's behavior if the WebContents is destroyed first is far from
clear.
> Oh well. I can sort that out afterwards I guess.]

Acknowledged.

https://codereview.chromium.org/1467563002/diff/120001/content/browser/loader...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/1467563002/diff/120001/content/browser/loader...
content/browser/loader/resource_request_info_impl.h:73:
base::Callback<WebContents*(void)> GetWebContentsGetterForRequest()
On 2015/12/01 22:15:15, davidben (behind on reviews) wrote:
> Nit: I suppose if we're using a typedef, this should return WebContentsGetter.
> *shrug* Sorry about the back-and-forth there.

Done.

[jochen (gone - plz use gerrit), 2015-12-02 12:31:49]

lgtm

[clamy, 2015-12-02 13:00:34]

The CQ bit was checked by clamy@chromium.org

[clamy, 2015-12-02 13:00:34]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
yfriedman@chromium.org, davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1467563002/#ps140001
(title: "Addressed davidben's comments")

[commit-bot: I haz the power, 2015-12-02 13:00:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1467563002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1467563002/140001

[commit-bot: I haz the power, 2015-12-02 14:09:37]

Description was changed from

==========
Use ResourceRequestInfo::GetWebContents in DownloadRequestLimiter

This CL makes DownloadRequestLimiter use a the callback returned by
ResourceRequestInfo::GetWebContents instead of using RFH + RPH IDs. This allows
most download browser_tests to work with PlzNavigate enabled.

BUG=504347
==========

to

==========
Use ResourceRequestInfo::GetWebContents in DownloadRequestLimiter

This CL makes DownloadRequestLimiter use a the callback returned by
ResourceRequestInfo::GetWebContents instead of using RFH + RPH IDs. This allows
most download browser_tests to work with PlzNavigate enabled.

BUG=504347
==========

[commit-bot: I haz the power, 2015-12-02 14:09:38]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-12-02 14:10:44]

Description was changed from

==========
Use ResourceRequestInfo::GetWebContents in DownloadRequestLimiter

This CL makes DownloadRequestLimiter use a the callback returned by
ResourceRequestInfo::GetWebContents instead of using RFH + RPH IDs. This allows
most download browser_tests to work with PlzNavigate enabled.

BUG=504347
==========

to

==========
Use ResourceRequestInfo::GetWebContents in DownloadRequestLimiter

This CL makes DownloadRequestLimiter use a the callback returned by
ResourceRequestInfo::GetWebContents instead of using RFH + RPH IDs. This allows
most download browser_tests to work with PlzNavigate enabled.

BUG=504347

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

[commit-bot: I haz the power, 2015-12-02 14:10:45]

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