Simplify DownloadRequestHandle by using WebContentsGetter

Simplify DownloadRequestHandle by using a WebContentsGetter

This patch removes storing child ids / rvids in favor of a
callback. The callback also removes the need for some duplicate
logic.

BUG=472869
Committed: https://crrev.com/cb2bdfabf238ccd33584a26a15a7650678e1949c
Cr-Commit-Position: refs/heads/master@{#377880}

[Charlie Harrison, 2016-02-23 15:10:53]

Description was changed from

==========
[WIP] Simplify DownloadRequestHandle

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

to

==========
Simplify DownloadRequestHandle

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

[Charlie Harrison, 2016-02-23 19:22:48]

csharrison@chromium.org changed reviewers:
+ asanka@chromium.org, clamy@chromium.org

[Charlie Harrison, 2016-02-23 19:22:49]

PTAL :)

https://codereview.chromium.org/1728583002/diff/1/content/browser/download/do...
File content/browser/download/download_request_handle.cc (left):

https://codereview.chromium.org/1728583002/diff/1/content/browser/download/do...
content/browser/download/download_request_handle.cc:70: if
(IsBrowserSideNavigationEnabled()) {
clamy@, this is all obsolete with the web contents getter right?

https://codereview.chromium.org/1728583002/diff/1/content/browser/download/do...
File content/browser/download/download_request_handle.cc (right):

https://codereview.chromium.org/1728583002/diff/1/content/browser/download/do...
content/browser/download/download_request_handle.cc:17: WebContents*
NullContents() {
There may be a better way of doing this (base::Bind([]() { return NULL; }) ? Not
sure if lambdas work with Bind.

https://codereview.chromium.org/1728583002/diff/1/content/browser/download/do...
content/browser/download/download_request_handle.cc:70: return "{}";
The request_id_ was only used for this DebugString, should we keep it?

[Charlie Harrison, 2016-02-23 19:25:35]

Description was changed from

==========
Simplify DownloadRequestHandle

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

to

==========
Simplify DownloadRequestHandle by using a WebContentsGetter

This patch removes storing child ids / rvids in favor of a
callback. The callback also removes the need for some duplicate
logic.

BUG=472869
==========

[asanka, 2016-02-24 20:02:16]

https://codereview.chromium.org/1728583002/diff/1/content/browser/download/do...
File content/browser/download/download_request_handle.cc (right):

https://codereview.chromium.org/1728583002/diff/1/content/browser/download/do...
content/browser/download/download_request_handle.cc:17: WebContents*
NullContents() {
On 2016/02/23 at 19:22:49, csharrison wrote:
> There may be a better way of doing this (base::Bind([]() { return NULL; }) ?
Not sure if lambdas work with Bind.

Or you could make GetWebContents() check for !web_contents_getter.is_null()
before calling Run(). That way you can pass an empty callback for the default
constructor.

Checking for is_null() isn't a bad idea by itself since callbacks don't confer
any guarantee of lifetime.

We could also get rid of the default constructor, which appears to be only used
by a DownloadManagerImpl test.

https://codereview.chromium.org/1728583002/diff/1/content/browser/download/do...
content/browser/download/download_request_handle.cc:18: return NULL;
NULL -> nullptr here and elsewhere.

https://codereview.chromium.org/1728583002/diff/1/content/browser/download/do...
content/browser/download/download_request_handle.cc:70: return "{}";
On 2016/02/23 at 19:22:49, csharrison wrote:
> The request_id_ was only used for this DebugString, should we keep it?

Not necessary. Looks like this is only called from
DownloadCreateInfo::DebugString(), which in turn is called by nobody. We should
be able to nuke this method + the corresponding method in
DownloadRequestHandleInterface + the corresponding method in
UrlDownloader::RequestHandle, and DownloadCreateInfo::DebugString().

[Charlie Harrison, 2016-02-25 15:49:55]

Thanks!

https://codereview.chromium.org/1728583002/diff/1/content/browser/download/do...
File content/browser/download/download_request_handle.cc (right):

https://codereview.chromium.org/1728583002/diff/1/content/browser/download/do...
content/browser/download/download_request_handle.cc:17: WebContents*
NullContents() {
On 2016/02/24 at 20:02:16, asanka wrote:
> On 2016/02/23 at 19:22:49, csharrison wrote:
> > There may be a better way of doing this (base::Bind([]() { return NULL; }) ?
Not sure if lambdas work with Bind.
> 
> Or you could make GetWebContents() check for !web_contents_getter.is_null()
before calling Run(). That way you can pass an empty callback for the default
constructor.
> 
> Checking for is_null() isn't a bad idea by itself since callbacks don't confer
any guarantee of lifetime.
> 
> We could also get rid of the default constructor, which appears to be only
used by a DownloadManagerImpl test.

I went with using the null callback.

https://codereview.chromium.org/1728583002/diff/1/content/browser/download/do...
content/browser/download/download_request_handle.cc:18: return NULL;
On 2016/02/24 at 20:02:16, asanka wrote:
> NULL -> nullptr here and elsewhere.

Done.

https://codereview.chromium.org/1728583002/diff/1/content/browser/download/do...
content/browser/download/download_request_handle.cc:70: return "{}";
On 2016/02/24 at 20:02:16, asanka wrote:
> On 2016/02/23 at 19:22:49, csharrison wrote:
> > The request_id_ was only used for this DebugString, should we keep it?
> 
> Not necessary. Looks like this is only called from
DownloadCreateInfo::DebugString(), which in turn is called by nobody. We should
be able to nuke this method + the corresponding method in
DownloadRequestHandleInterface + the corresponding method in
UrlDownloader::RequestHandle, and DownloadCreateInfo::DebugString().

Nice. Done.

[asanka, 2016-02-25 16:19:10]

lgtm % nits

https://codereview.chromium.org/1728583002/diff/40001/content/browser/downloa...
File content/browser/download/download_request_handle.cc (right):

https://codereview.chromium.org/1728583002/diff/40001/content/browser/downloa...
content/browser/download/download_request_handle.cc:23: :
web_contents_getter_(content::ResourceRequestInfo::WebContentsGetter()) {}
Nit: Don't explicitly initialize this. You want the default constructor anyway.

[Charlie Harrison, 2016-02-25 16:53:58]

Thanks asanka@!

clamy@, could you sanity check the PlzNavigate changes?

https://codereview.chromium.org/1728583002/diff/40001/content/browser/downloa...
File content/browser/download/download_request_handle.cc (right):

https://codereview.chromium.org/1728583002/diff/40001/content/browser/downloa...
content/browser/download/download_request_handle.cc:23: :
web_contents_getter_(content::ResourceRequestInfo::WebContentsGetter()) {}
On 2016/02/25 at 16:19:10, asanka wrote:
> Nit: Don't explicitly initialize this. You want the default constructor
anyway.

Done.

[clamy, 2016-02-26 13:56:12]

Thanks! Lgtm, nice to see this specialized code going away.

[Charlie Harrison, 2016-02-26 14:03:39]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-02-26 14:03:40]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org
Link to the patchset: https://codereview.chromium.org/1728583002/#ps60001
(title: "default callback constructor")

[commit-bot: I haz the power, 2016-02-26 14:04:00]

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

[commit-bot: I haz the power, 2016-02-26 14:08:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-26 14:09:34]

Description was changed from

==========
Simplify DownloadRequestHandle by using a WebContentsGetter

This patch removes storing child ids / rvids in favor of a
callback. The callback also removes the need for some duplicate
logic.

BUG=472869
==========

to

==========
Simplify DownloadRequestHandle by using a WebContentsGetter

This patch removes storing child ids / rvids in favor of a
callback. The callback also removes the need for some duplicate
logic.

BUG=472869

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

[commit-bot: I haz the power, 2016-02-26 14:09:35]

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