Download resumption - Preliminary

Download resumption


BUG=None

[Randy Smith (Not in Mondays), 2012-08-24 20:09:11]

First, a bit of context: My comments are going to be made in the context of
http://codereview.chromium.org/10823406, which is the removal of the
DownloadFileManager.  Don't believe the "(Closed)" on that issue; it's now been
landed and reverted three times :-{.  But hopefully fourth time's the charm, or
alternatively, maybe I can hit a record for lands+reverts of the same CL (I
think the current winner is 6 :-J).

My goal here is to make download resumption look like download initiation as
much as humanly possible (or maybe a bit more :-}).  Specifically, after the
"merge" occurs, I'd like to have all the rest of the code be shared, and hence
have very few if any "are we resuming?" conditionals.  I envision that as
requiring the following things:

* We'll want to pass the download request handle into the DownloadItem along
with the new DownloadFile rather than constructing it with a DRH; I envision
this as happening in the DownloadItemImpl::Start() routine (see above CL).

* We'll need to be able to test to see if the file name determination has
occurred.  Eventually, I'd like this to be a state on the download item, but I
don't think you should do that as part of this CL.  Instead, check to see if the
target filename is non-null.  There may be an additional check needed here
involving the confirmation that the intermediate file has been renamed; we'll
have to pull in Asanka/Ben to review.  But target filename non-null will do. 
Put a TODO in there pointing at me for creating the new state.

* We need to be able to test to see if the download has already been persisted
into the history.  This problem is actually going away soon, probably before you
commit; see http://codereview.chromium.org/10665049/ .

But if you have those tests, I think the only place where you need to do the
"resuming/not-resuming" check is when you decide whether or not to create a new
download item; after that, it All Just Works.  And if we can get resumption
looking like that, I'll be very happy.

WDYT?

[ahendrickson, 2012-08-24 21:52:18]

I'll take a look at
http://codereview.chromium.**org/10823406<http://codereview.chromium.org/1082...
see
what I can do.

For the third point, I'm not allowing resumption until the download has
been persisted.  If this is not good, let's talk about what we can do.

Andy

On Fri, Aug 24, 2012 at 4:09 PM, <rdsmith@chromium.org> wrote:

> First, a bit of context: My comments are going to be made in the context of
>
http://codereview.chromium.**org/10823406<http://codereview.chromium.org/1082...,
> which is the removal of the
> DownloadFileManager.  Don't believe the "(Closed)" on that issue; it's now
> been
> landed and reverted three times :-{.  But hopefully fourth time's the
> charm, or
> alternatively, maybe I can hit a record for lands+reverts of the same CL (I
> think the current winner is 6 :-J).
>
> My goal here is to make download resumption look like download initiation
> as
> much as humanly possible (or maybe a bit more :-}).  Specifically, after
> the
> "merge" occurs, I'd like to have all the rest of the code be shared, and
> hence
> have very few if any "are we resuming?" conditionals.  I envision that as
> requiring the following things:
>
> * We'll want to pass the download request handle into the DownloadItem
> along
> with the new DownloadFile rather than constructing it with a DRH; I
> envision
> this as happening in the DownloadItemImpl::Start() routine (see above CL).
>
> * We'll need to be able to test to see if the file name determination has
> occurred.  Eventually, I'd like this to be a state on the download item,
> but I
> don't think you should do that as part of this CL.  Instead, check to see
> if the
> target filename is non-null.  There may be an additional check needed here
> involving the confirmation that the intermediate file has been renamed;
> we'll
> have to pull in Asanka/Ben to review.  But target filename non-null will
> do.
> Put a TODO in there pointing at me for creating the new state.
>
> * We need to be able to test to see if the download has already been
> persisted
> into the history.  This problem is actually going away soon, probably
> before you
> commit; see
http://codereview.chromium.**org/10665049/<http://codereview.chromium.org/106....
>
> But if you have those tests, I think the only place where you need to do
> the
> "resuming/not-resuming" check is when you decide whether or not to create
> a new
> download item; after that, it All Just Works.  And if we can get resumption
> looking like that, I'll be very happy.
>
> WDYT?
>
>
>
>
http://codereview.chromium.**org/10831302/<http://codereview.chromium.org/108...
>

[Randy Smith (Not in Mondays), 2012-08-25 15:23:23]

On 2012/08/24 21:52:18, ahendrickson wrote:
> I'll take a look at
>
http://codereview.chromium.**org/10823406%3Chttp://codereview.chromium.org/10...>
> see
> what I can do.
> 
> For the third point, I'm not allowing resumption until the download has
> been persisted.  If this is not good, let's talk about what we can do.

That would deal with the second point as well, but you're going to collide
rather directly with Ben's CL that is removing the history from the core
downloads system--soon you won't be able to tell when the download has been
persisted.  I think it's likely he'll commit before you, so my recommendation
would be not to rely on persistence.  See
http://codereview.chromium.org/10665049/ .

> 
> Andy
> 
> On Fri, Aug 24, 2012 at 4:09 PM, <mailto:rdsmith@chromium.org> wrote:
> 
> > First, a bit of context: My comments are going to be made in the context of
> >
>
http://codereview.chromium.**org/10823406%3Chttp://codereview.chromium.org/10...>,
> > which is the removal of the
> > DownloadFileManager.  Don't believe the "(Closed)" on that issue; it's now
> > been
> > landed and reverted three times :-{.  But hopefully fourth time's the
> > charm, or
> > alternatively, maybe I can hit a record for lands+reverts of the same CL (I
> > think the current winner is 6 :-J).
> >
> > My goal here is to make download resumption look like download initiation
> > as
> > much as humanly possible (or maybe a bit more :-}).  Specifically, after
> > the
> > "merge" occurs, I'd like to have all the rest of the code be shared, and
> > hence
> > have very few if any "are we resuming?" conditionals.  I envision that as
> > requiring the following things:
> >
> > * We'll want to pass the download request handle into the DownloadItem
> > along
> > with the new DownloadFile rather than constructing it with a DRH; I
> > envision
> > this as happening in the DownloadItemImpl::Start() routine (see above CL).
> >
> > * We'll need to be able to test to see if the file name determination has
> > occurred.  Eventually, I'd like this to be a state on the download item,
> > but I
> > don't think you should do that as part of this CL.  Instead, check to see
> > if the
> > target filename is non-null.  There may be an additional check needed here
> > involving the confirmation that the intermediate file has been renamed;
> > we'll
> > have to pull in Asanka/Ben to review.  But target filename non-null will
> > do.
> > Put a TODO in there pointing at me for creating the new state.
> >
> > * We need to be able to test to see if the download has already been
> > persisted
> > into the history.  This problem is actually going away soon, probably
> > before you
> > commit; see
>
http://codereview.chromium.**org/10665049/%3Chttp://codereview.chromium.org/1...>.
> >
> > But if you have those tests, I think the only place where you need to do
> > the
> > "resuming/not-resuming" check is when you decide whether or not to create
> > a new
> > download item; after that, it All Just Works.  And if we can get resumption
> > looking like that, I'll be very happy.
> >
> > WDYT?
> >
> >
> >
> >
>
http://codereview.chromium.**org/10831302/%3Chttp://codereview.chromium.org/1...>
> >

[ahendrickson, 2012-10-11 02:48:31]

PTAL.

[Randy Smith (Not in Mondays), 2012-10-15 00:36:07]

Hey, Andy.  Included are some high level thoughts on this, preperatory to
talking next week.  I'm adding Asanka to the review, as I'd like to do the
architectural level review and ask him to do the detailed, line-by-line review;
I'll be trying to include him in our conversation this week.  Asanka: I'll be
expressing high level architectural preferences in this CL and in the
conversation; please make sure to raise a flag early with me if you disagree,
just so that Andy has the minimal amount of being pulled between two different
reviewers that we can manage :-}.

There are a couple of areas in which you're moving against the overall direction
I'd like to see the downloads system moving in.  These aren't "don't do this",
but instead "I'd like to explore if there are different ways to do this--if not,
we'll probably end up cleaning them up later, and there's no point in re-doing
work."
+ I'm looking more and more for the "main logic" of downloads to be in the
DownloadItemImpl, with it calling into DownloadManagerImpl via the DIIDelegate
interface when necessary.  So things that move further functionality further
away from the download item I'd like to avoid.  Two specific things fall into
this category: MaybeCompleteDownload taking an id (and I'm not sure why that
change is there anyway?), and RestartInterruptedDownload being a method on the
DownloadManagerImpl rather than the DownloadItem{Impl}.
+ I'm looking to get rid of all the different queues on the DownloadManagerImpl
and instead conditionalize on state in the DownloadItemImpl, so I'd rather not
use GetActiveDownload().
+ I'd rather not export the bound net log from the download item, just in terms
of keeping the interface small.  What you have currently makes sense, and we may
want well to keep it (I think we need to delegate creation of the DownloadFile
into the DownloadItemImpl if we don't, and that may be ugly enough not to be
worth it) (note: this comment made in the context of the DFM removal CL; see
below), but it's the kind of interface that feels like a wart, and it may be
possible within a CL or two to to create the new DownloadFile within the
DownloadItemImpl.

I'd also like to request a change in the interaction with the filename
determination.  I think the code would actually be cleaner if we unilaterally
(meaning on both start and resume pathways) conditionalized file name
determination on whether it had already been done.  Asanka can guide you here
(and can also guide you on the subject of whether there's anything that needs to
be done on the filename determination path if it has already been done--there
may be reservations made since the download was cancelled/interrupted that are
relevant to it now that it's been restarted).  

You might also want to think about re-basing this CL on top of my
DownloadFileManager removal CL (http://codereview.chromium.org/10912173/).  That
CL has been an incredibly long time going into the code base :-{ :-{, but
substantially changes the logic you'll be rewriting, and may make it a bit
simpler (because it gives direct access to the DownloadFile and incorporates
OffThreadCancel into the DownloadItemImpl).  Unless I'm really lame, you'll have
to merge with it anyway, and merging earlier may well be easier.  I can give you
a sketch of how to do the rebase in git if you want.

http://codereview.chromium.org/10831302/diff/68002/content/browser/download/d...
File content/browser/download/download_item_impl.h (left):

http://codereview.chromium.org/10831302/diff/68002/content/browser/download/d...
content/browser/download/download_item_impl.h:46:
scoped_ptr<DownloadRequestHandleInterface> request_handle,
Why?  (This question applies to DownloadItemFactory::CreateActiveItem as well). 
It looks as if Resume() takes a request_handle, so I don't see the reason to
remove this from the constructor (and thus require the addition of
SetRequest()).  

One possible design would be to create the DownloadItemImpl in (effectively) an
already cancelled/interrupted state, and use the same accessor on it (presumably
Start() or something like it) to start it, passing in a request_handle.  I'm
open to such designs, but they're a little tricky, as the set of state
transitions that the DII would go through would look weird to an outsider
observer (CANCELLED -> IN_PROGRESS).  That might be soluble by an
internal/external state dichotomy (I recently split the DII internal state away
from the external state, so this wouldn't be hard to do), but I'm not sure it's
worth picking up that particular can of worms right now.  So I'd be more
inclined to have the creation pathway be as it currently is, and have the
resumption pathway pass in the request handle.

http://codereview.chromium.org/10831302/diff/68002/content/browser/download/d...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/10831302/diff/68002/content/browser/download/d...
content/browser/download/download_item_impl.h:144: bool IsResuming() const;
This (including the underlying state variable) doesn't appear to be used
anywhere; can we delete it?

http://codereview.chromium.org/10831302/diff/68002/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10831302/diff/68002/content/browser/download/d...
content/browser/download/download_manager_impl.cc:786: const
content::DownloadUrlParameters::OnStartedCallback& callback) {
If we've already got a download item, let's not used OnStartedCallback--let's
report the error through the DI.

http://codereview.chromium.org/10831302/diff/68002/content/public/browser/dow...
File content/public/browser/download_item.h (right):

http://codereview.chromium.org/10831302/diff/68002/content/public/browser/dow...
content/public/browser/download_item.h:99: };
I'm uncomfortable about exposing this at the DownloadItem level, at least before
we have some need of it at that level.  All that the DownloadItem consumers need
is != INVALID, which is most of what CanResumeDownload() gives.  Can we use just
that, and leave this (and CanResumeInterrupted()) out of the DownloadItem?

[benjhayden, 2012-10-15 17:59:37]

http://codereview.chromium.org/10831302/diff/68002/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10831302/diff/68002/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:254: int64
ChromeDownloadManagerDelegate::GetIntermediateFileLength(
Could this move to download_utils?

http://codereview.chromium.org/10831302/diff/68002/chrome/browser/download/do...
File chrome/browser/download/download_item_model.cc (right):

http://codereview.chromium.org/10831302/diff/68002/chrome/browser/download/do...
chrome/browser/download/download_item_model.cc:249:
DownloadItem::RESUME_MODE_INVALID)) {
Can this go on the previous line?

http://codereview.chromium.org/10831302/diff/68002/chrome/browser/ui/views/do...
File chrome/browser/ui/views/download/download_item_view.cc (right):

http://codereview.chromium.org/10831302/diff/68002/chrome/browser/ui/views/do...
chrome/browser/ui/views/download/download_item_view.cc:815: } else if
(download_->IsInterrupted()) {
Woah, nice catch!
I presume this is purely a bug fix. If so, can you split it out into a separate
CL so that it doesn't have to wait?

http://codereview.chromium.org/10831302/diff/68002/content/browser/download/d...
File content/browser/download/download_item_impl.cc (left):

http://codereview.chromium.org/10831302/diff/68002/content/browser/download/d...
content/browser/download/download_item_impl.cc:479: return  external_state ==
CANCELLED || external_state == INTERRUPTED;
Have you audited all the callers of IsCancelled() to see if any of them want to
also call IsInterrupted()?
Would you mind splitting this out into a separate CL?
I'm sensitive to subtle changes like this because I've just had a run of subtle
CLs that broke things.

http://codereview.chromium.org/10831302/diff/68002/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10831302/diff/68002/content/browser/download/d...
content/browser/download/download_item_impl.cc:567: return (state ==
IN_PROGRESS) || (state == INTERRUPTED);
Have you audited all the callers of IsPartialDownload() to see if any of them
actually mean to call IsInProgress()?
Would you mind splitting this out into a separate CL?

http://codereview.chromium.org/10831302/diff/68002/content/browser/download/d...
content/browser/download/download_item_impl.cc:919:
delegate_->MaybeCompleteDownload(download_id_.local());
Sorry, what's this doing? I looked in download_manager_impl.cc and didn't see an
explanation there, either.

[ahendrickson, 2012-11-26 02:29:19]

Hey Randy,

You can take this over now.

The download_item_impl_unittest.cc file could use some love.  Note that all
tests in that file passed on my machine, despite what the win_rel trybot says.

Andy

On 2012/10/15 00:36:07, rdsmith wrote:
> Hey, Andy.  Included are some high level thoughts on this, preperatory to
> talking next week.  I'm adding Asanka to the review, as I'd like to do the
> architectural level review and ask him to do the detailed, line-by-line
review;
> I'll be trying to include him in our conversation this week.  Asanka: I'll be
> expressing high level architectural preferences in this CL and in the
> conversation; please make sure to raise a flag early with me if you disagree,
> just so that Andy has the minimal amount of being pulled between two different
> reviewers that we can manage :-}.
> 
> There are a couple of areas in which you're moving against the overall
direction
> I'd like to see the downloads system moving in.  These aren't "don't do this",
> but instead "I'd like to explore if there are different ways to do this--if
not,
> we'll probably end up cleaning them up later, and there's no point in re-doing
> work."
> + I'm looking more and more for the "main logic" of downloads to be in the
> DownloadItemImpl, with it calling into DownloadManagerImpl via the DIIDelegate
> interface when necessary.  So things that move further functionality further
> away from the download item I'd like to avoid.  Two specific things fall into
> this category: MaybeCompleteDownload taking an id (and I'm not sure why that
> change is there anyway?), and RestartInterruptedDownload being a method on the
> DownloadManagerImpl rather than the DownloadItem{Impl}.
> + I'm looking to get rid of all the different queues on the
DownloadManagerImpl
> and instead conditionalize on state in the DownloadItemImpl, so I'd rather not
> use GetActiveDownload().
> + I'd rather not export the bound net log from the download item, just in
terms
> of keeping the interface small.  What you have currently makes sense, and we
may
> want well to keep it (I think we need to delegate creation of the DownloadFile
> into the DownloadItemImpl if we don't, and that may be ugly enough not to be
> worth it) (note: this comment made in the context of the DFM removal CL; see
> below), but it's the kind of interface that feels like a wart, and it may be
> possible within a CL or two to to create the new DownloadFile within the
> DownloadItemImpl.
> 
> I'd also like to request a change in the interaction with the filename
> determination.  I think the code would actually be cleaner if we unilaterally
> (meaning on both start and resume pathways) conditionalized file name
> determination on whether it had already been done.  Asanka can guide you here
> (and can also guide you on the subject of whether there's anything that needs
to
> be done on the filename determination path if it has already been done--there
> may be reservations made since the download was cancelled/interrupted that are
> relevant to it now that it's been restarted).  
> 
> You might also want to think about re-basing this CL on top of my
> DownloadFileManager removal CL (http://codereview.chromium.org/10912173/). 
That
> CL has been an incredibly long time going into the code base :-{ :-{, but
> substantially changes the logic you'll be rewriting, and may make it a bit
> simpler (because it gives direct access to the DownloadFile and incorporates
> OffThreadCancel into the DownloadItemImpl).  Unless I'm really lame, you'll
have
> to merge with it anyway, and merging earlier may well be easier.  I can give
you
> a sketch of how to do the rebase in git if you want.
> 
>
http://codereview.chromium.org/10831302/diff/68002/content/browser/download/d...
> File content/browser/download/download_item_impl.h (left):
> 
>
http://codereview.chromium.org/10831302/diff/68002/content/browser/download/d...
> content/browser/download/download_item_impl.h:46:
> scoped_ptr<DownloadRequestHandleInterface> request_handle,
> Why?  (This question applies to DownloadItemFactory::CreateActiveItem as
well). 
> It looks as if Resume() takes a request_handle, so I don't see the reason to
> remove this from the constructor (and thus require the addition of
> SetRequest()).  
> 
> One possible design would be to create the DownloadItemImpl in (effectively)
an
> already cancelled/interrupted state, and use the same accessor on it
(presumably
> Start() or something like it) to start it, passing in a request_handle.  I'm
> open to such designs, but they're a little tricky, as the set of state
> transitions that the DII would go through would look weird to an outsider
> observer (CANCELLED -> IN_PROGRESS).  That might be soluble by an
> internal/external state dichotomy (I recently split the DII internal state
away
> from the external state, so this wouldn't be hard to do), but I'm not sure
it's
> worth picking up that particular can of worms right now.  So I'd be more
> inclined to have the creation pathway be as it currently is, and have the
> resumption pathway pass in the request handle.
> 
>
http://codereview.chromium.org/10831302/diff/68002/content/browser/download/d...
> File content/browser/download/download_item_impl.h (right):
> 
>
http://codereview.chromium.org/10831302/diff/68002/content/browser/download/d...
> content/browser/download/download_item_impl.h:144: bool IsResuming() const;
> This (including the underlying state variable) doesn't appear to be used
> anywhere; can we delete it?
> 
>
http://codereview.chromium.org/10831302/diff/68002/content/browser/download/d...
> File content/browser/download/download_manager_impl.cc (right):
> 
>
http://codereview.chromium.org/10831302/diff/68002/content/browser/download/d...
> content/browser/download/download_manager_impl.cc:786: const
> content::DownloadUrlParameters::OnStartedCallback& callback) {
> If we've already got a download item, let's not used OnStartedCallback--let's
> report the error through the DI.
> 
>
http://codereview.chromium.org/10831302/diff/68002/content/public/browser/dow...
> File content/public/browser/download_item.h (right):
> 
>
http://codereview.chromium.org/10831302/diff/68002/content/public/browser/dow...
> content/public/browser/download_item.h:99: };
> I'm uncomfortable about exposing this at the DownloadItem level, at least
before
> we have some need of it at that level.  All that the DownloadItem consumers
need
> is != INVALID, which is most of what CanResumeDownload() gives.  Can we use
just
> that, and leave this (and CanResumeInterrupted()) out of the DownloadItem?

[Randy Smith (Not in Mondays), 2012-12-14 16:34:12]

I've created a new issue (owned by me) and uploaded it with the contents of
PS19: https://codereview.chromium.org/11571025/.  I will close this issue.