[Downloads] Move UpdateObservers out of TransitionTo in DownloadItemImpl

[Downloads] Move UpdateObservers out of TransitionTo in DownloadItemImpl

The UpdateObservers call doesn't take into account other changes that
may have been made to the DownloadItem and may not call UpdateObservers
when it should. It's still the responsibility of the caller to invoke
UpdateObservers if ncessary. Make this requirement explicit.

Also only release the DownloadFile and cancel the request when 
interrupting an IN_PROGRESS download.

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208368

[Randy Smith (Not in Mondays), 2013-06-21 01:19:54]

So I was a bit uncomfortable with this because I think it makes it much easier
to miss an UpdateObservers() call.  I have a preference for just adding an
argument to TransitionTo() to indicate whether or not UpdateObservers() should
be called, because it makes people think about the question everywhere they
might need to.  Am I missing something?

[asanka, 2013-06-21 16:55:45]

On 2013/06/21 01:19:54, rdsmith wrote:
> So I was a bit uncomfortable with this because I think it makes it much easier
> to miss an UpdateObservers() call.  I have a preference for just adding an
> argument to TransitionTo() to indicate whether or not UpdateObservers() should
> be called, because it makes people think about the question everywhere they
> might need to.  Am I missing something?

Yeah. That's reasonable. I've made this change.

I moved UpdateObservers() out since there are other places where we make
externally visible state in DownloadItemImpl where the only option is to call
UpdateObservers(). So I thought of treating state_ the same.

[Randy Smith (Not in Mondays), 2013-06-21 18:31:28]

https://codereview.chromium.org/17515003/diff/12001/content/browser/download/...
File content/browser/download/download_item_impl.cc (left):

https://codereview.chromium.org/17515003/diff/12001/content/browser/download/...
content/browser/download/download_item_impl.cc:1314: UpdateObservers();
I think this was for the auto_opened_ transition; does it make sense to remove
it?

https://codereview.chromium.org/17515003/diff/12001/content/browser/download/...
content/browser/download/download_item_impl.cc:1538: UpdateObservers();
Under what circumstances would we not want to update the observers if we're
setting a full path?

https://codereview.chromium.org/17515003/diff/12001/content/browser/download/...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/17515003/diff/12001/content/browser/download/...
content/browser/download/download_item_impl.cc:1281: UpdateObservers();
What are we letting the observers know about here?

https://codereview.chromium.org/17515003/diff/12001/content/browser/download/...
content/browser/download/download_item_impl.cc:1370:
DCHECK(!download_file_.get());
What's the status of the request handle at this point?  Is there an available
DCHECK there?

[asanka, 2013-06-21 20:30:56]

https://codereview.chromium.org/17515003/diff/12001/content/browser/download/...
File content/browser/download/download_item_impl.cc (left):

https://codereview.chromium.org/17515003/diff/12001/content/browser/download/...
content/browser/download/download_item_impl.cc:1314: UpdateObservers();
On 2013/06/21 18:31:28, rdsmith wrote:
> I think this was for the auto_opened_ transition; does it make sense to remove
> it?

Nope. Restored.

https://codereview.chromium.org/17515003/diff/12001/content/browser/download/...
content/browser/download/download_item_impl.cc:1538: UpdateObservers();
On 2013/06/21 18:31:28, rdsmith wrote:
> Under what circumstances would we not want to update the observers if we're
> setting a full path?

Two of the three SetFullPath calls are made before a state transition. I didn't
think we need those observer updates.

https://codereview.chromium.org/17515003/diff/12001/content/browser/download/...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/17515003/diff/12001/content/browser/download/...
content/browser/download/download_item_impl.cc:1281: UpdateObservers();
On 2013/06/21 18:31:28, rdsmith wrote:
> What are we letting the observers know about here?

I removed the UpdateObservers() call in SetFullPath(). So if we passed through
the SetFullPath() call above and get here, we haven't updated the observers
regarding the new path.

https://codereview.chromium.org/17515003/diff/12001/content/browser/download/...
content/browser/download/download_item_impl.cc:1370:
DCHECK(!download_file_.get());
On 2013/06/21 18:31:28, rdsmith wrote:
> What's the status of the request handle at this point?  Is there an available
> DCHECK there?

The request_handle_ may or may not be NULL. E.g. if we restore a download from
history, try to resume it, and the resumption request fails, then there would be
no request_handle. If an in-progress download is interrupted, resumed, and the
resumption request fails, then request_handle_ would be non-NULL.

I don't think we have a readily available condition that we could use to test
that if a non-NULL request_handle_ exists, that it refers to a defunct request.

[Randy Smith (Not in Mondays), 2013-06-24 15:21:54]

LGTM.

[asanka, 2013-06-24 22:41:19]

On 2013/06/24 15:21:54, rdsmith wrote:
> LGTM.

Thanks!

[commit-bot: I haz the power, 2013-06-24 22:42:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/17515003/11003

[commit-bot: I haz the power, 2013-06-25 03:11:09]

Change committed as 208368