Make the UI an observer of downloads.

Make the UI an observer of downloads.

This relieves content/browser/download from knowing who/when to notify regarding a new download.

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

[asanka, 2012-12-20 18:46:11]

rdsmith: Can you take a look at {chrome,content}/browser/download ? In
particular, the download_ui_controller.* implementation?

I still need to verify the behavior and test coverage for downloads started from
tabs that are closed by the time the download is displayed, extension hosts,
started from popups or panels. Also I need to verify the CL on Android.

[benjhayden, 2012-12-20 21:09:14]

drive-by

https://codereview.chromium.org/11640007/diff/7001/chrome/browser/download/do...
File chrome/browser/download/download_ui_controller.cc (right):

https://codereview.chromium.org/11640007/diff/7001/chrome/browser/download/do...
chrome/browser/download/download_ui_controller.cc:104: // GET downloads are
delegated to and Android DownloadManager. Chrome is only
to an

https://codereview.chromium.org/11640007/diff/7001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11640007/diff/7001/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:1337: void
Browser::ShowDownload(content::DownloadItem* download) {
Can this method be moved out to the Observer? It looks like it doesn't use
anything in the Browser except window(), which is public, and CloseContents,
which could be accessed by up-casting the Browser*. If you don't want to up-cast
it, could you move the rest of this method out into the Observer?

https://codereview.chromium.org/11640007/diff/7001/content/browser/android/do...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/11640007/diff/7001/content/browser/android/do...
content/browser/android/download_controller_android_impl.cc:240: if
(download_item->GetWebContents() == NULL)
I thought the convention was if (!GetWebContents())

[asanka, 2012-12-20 22:41:19]

Ben: Thanks for the drive-by!

Randy: I changed the DownloadUIController to use the DownloadItemModel for
deciding whether to notify the UI.

https://codereview.chromium.org/11640007/diff/7001/chrome/browser/download/do...
File chrome/browser/download/download_ui_controller.cc (right):

https://codereview.chromium.org/11640007/diff/7001/chrome/browser/download/do...
chrome/browser/download/download_ui_controller.cc:104: // GET downloads are
delegated to and Android DownloadManager. Chrome is only
On 2012/12/20 21:09:14, benjhayden_chromium wrote:
> to an

Done.

https://codereview.chromium.org/11640007/diff/7001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11640007/diff/7001/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:1337: void
Browser::ShowDownload(content::DownloadItem* download) {
On 2012/12/20 21:09:14, benjhayden_chromium wrote:
> Can this method be moved out to the Observer? It looks like it doesn't use
> anything in the Browser except window(), which is public, and CloseContents,
> which could be accessed by up-casting the Browser*. If you don't want to
up-cast
> it, could you move the rest of this method out into the Observer?

The Browser class:
- Handles closing the tab that initiated the download if that was the only thing
that the tab did.
- Handles showing the download in the shelf.
- Handles displaying the "download started" animation.

The DownloadUIController class:
- Figures out which Browser to notify and notifies it.

I think this is a reasonable layering scheme. I don't think DownloadUIController
should reach into a Browser object and close its tabs or insert downloads into
the shelf or trigger animations, even if its possible with the API.

https://codereview.chromium.org/11640007/diff/7001/content/browser/android/do...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/11640007/diff/7001/content/browser/android/do...
content/browser/android/download_controller_android_impl.cc:240: if
(download_item->GetWebContents() == NULL)
On 2012/12/20 21:09:14, benjhayden_chromium wrote:
> I thought the convention was if (!GetWebContents())

Done.

[Randy Smith (Not in Mondays), 2012-12-20 23:09:25]

Wow, this is much less painful than I feared.

https://codereview.chromium.org/11640007/diff/8031/chrome/browser/download/do...
File chrome/browser/download/download_ui_controller.cc (right):

https://codereview.chromium.org/11640007/diff/8031/chrome/browser/download/do...
chrome/browser/download/download_ui_controller.cc:56: // complete.
What's the goal here?  And isn't it true that the full path is always set after
the target path, so this test renders the above test redundant?

(This comment may be equivalent to "can you flesh out the TODO() so that someone
beside you could read it and have the background to execute it?")

https://codereview.chromium.org/11640007/diff/8031/chrome/browser/download/do...
chrome/browser/download/download_ui_controller.cc:62: // in-progress and the
path is known.
What happens if the download is interrupted before we determine the target file
name?  We never see it?

https://codereview.chromium.org/11640007/diff/8031/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/11640007/diff/8031/chrome/browser/ui/browser....
chrome/browser/ui/browser.h:490: void ShowDownload(content::DownloadItem*
download);
So an alternate design would have been to have each download shelf observer the
relevant download manager and do the transition appropriate when a download
showed up (i.e. if the download belonged to a tab on that browser, show the
download shelf if not shown, then if the download shelf was shown, show the
download).  Can you say why you went in this direction?  (Not objecting, but
wanting to explore the different alternatives.)

[asanka, 2012-12-20 23:56:19]

https://codereview.chromium.org/11640007/diff/8031/chrome/browser/download/do...
File chrome/browser/download/download_ui_controller.cc (right):

https://codereview.chromium.org/11640007/diff/8031/chrome/browser/download/do...
chrome/browser/download/download_ui_controller.cc:56: // complete.
On 2012/12/20 23:09:25, rdsmith wrote:
> What's the goal here?

I suppose the TODO wasn't a giveaway.

> And isn't it true that the full path is always set after
> the target path, so this test renders the above test redundant?

No. The FullPath() isn't set for downloads that are interrupted before/during
the initial rename.

We are filtering for:
- In progress downloads:
  These are shown after they are successfully renamed to the intermediate name
(Both GetTargetFilePath() and GetFullPath() are non-empty and InProgress() is
true).

*and*

- Interrupted downloads that started out in-progress:
  These are shown after the target name is determined so that we have a name to
show for the download. If the initial rename fails, GetTargetFilePath() would be
non-empty, while GetFullPath() is empty.

Why not just show downloads after the the target is determined? That's where I'd
like to go, but I didn't want to make that change until all the callers of
GetFullPath() have been removed from chrome/browser/ui. Currently it's used to
wire up drag&drop, which doesn't affect interrupted downloads.

> (This comment may be equivalent to "can you flesh out the TODO() so that
someone
> beside you could read it and have the background to execute it?")

Right. Perhaps doing the TODO before this CL would be easier. I'll actually do
that instead.

https://codereview.chromium.org/11640007/diff/8031/chrome/browser/download/do...
chrome/browser/download/download_ui_controller.cc:62: // in-progress and the
path is known.
On 2012/12/20 23:09:25, rdsmith wrote:
> What happens if the download is interrupted before we determine the target
file
> name?  We never see it?

Target name determination happens in chrome/. If the download is interrupted, we
wait until the name is determined and then show the interrupted download.

https://codereview.chromium.org/11640007/diff/8031/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/11640007/diff/8031/chrome/browser/ui/browser....
chrome/browser/ui/browser.h:490: void ShowDownload(content::DownloadItem*
download);
On 2012/12/20 23:09:25, rdsmith wrote:
> So an alternate design would have been to have each download shelf observer
the
> relevant download manager and do the transition appropriate when a download
> showed up (i.e. if the download belonged to a tab on that browser, show the
> download shelf if not shown, then if the download shelf was shown, show the
> download).  Can you say why you went in this direction?  (Not objecting, but
> wanting to explore the different alternatives.)

a) DownloadShelf objects aren't created for every Browser. They are created
lazily. We'd have to change that so every Browser creates a shelf proactively
just to observe the DownloadManager.

b) There can be multiple Browsers and multiple DownloadShelf objects. Each will
have to decide whether a newly created download belongs on that shelf.

c) Consumers like Android will make it necessary to have a central observer that
notifies the UI anyway.

[Randy Smith (Not in Mondays), 2012-12-25 23:05:54]

LGTM (up to you what to do with the comment below).

I'd like someone with context for the WebContentsDelegate to take a look at this
before it goes in, just in case there's some reason why OnStartDownload() is
conceptually useful that we don't know about.

https://codereview.chromium.org/11640007/diff/8031/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11640007/diff/8031/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:1338: scoped_ptr<DownloadItemModel>
download_model(new DownloadItemModel(download));
Why before bailing if the window isn't there?

[asanka, 2013-01-10 22:12:13]

I'll wait until I get back to do further review rounds and land.

https://codereview.chromium.org/11640007/diff/8031/chrome/browser/download/do...
File chrome/browser/download/download_ui_controller.cc (right):

https://codereview.chromium.org/11640007/diff/8031/chrome/browser/download/do...
chrome/browser/download/download_ui_controller.cc:56: // complete.
On 2012/12/20 23:56:19, asanka wrote:
> On 2012/12/20 23:09:25, rdsmith wrote:
> > What's the goal here?
> 
> I suppose the TODO wasn't a giveaway.
> 
> > And isn't it true that the full path is always set after
> > the target path, so this test renders the above test redundant?
> 
> No. The FullPath() isn't set for downloads that are interrupted before/during
> the initial rename.
> 
> We are filtering for:
> - In progress downloads:
>   These are shown after they are successfully renamed to the intermediate name
> (Both GetTargetFilePath() and GetFullPath() are non-empty and InProgress() is
> true).
> 
> *and*
> 
> - Interrupted downloads that started out in-progress:
>   These are shown after the target name is determined so that we have a name
to
> show for the download. If the initial rename fails, GetTargetFilePath() would
be
> non-empty, while GetFullPath() is empty.
> 
> Why not just show downloads after the the target is determined? That's where
I'd
> like to go, but I didn't want to make that change until all the callers of
> GetFullPath() have been removed from chrome/browser/ui. Currently it's used to
> wire up drag&drop, which doesn't affect interrupted downloads.
> 
> > (This comment may be equivalent to "can you flesh out the TODO() so that
> someone
> > beside you could read it and have the background to execute it?")
> 
> Right. Perhaps doing the TODO before this CL would be easier. I'll actually do
> that instead.

https://codereview.chromium.org/11801022/. I'm changing this CL to just wait
until the target determination is complete.

https://codereview.chromium.org/11640007/diff/8031/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11640007/diff/8031/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:1338: scoped_ptr<DownloadItemModel>
download_model(new DownloadItemModel(download));
On 2012/12/25 23:05:54, rdsmith wrote:
> Why before bailing if the window isn't there?

Done. Note that https://codereview.chromium.org/11673004/ is removing some of
this code.

[asanka, 2013-02-27 00:52:48]

Could you take a look?

sky:
  chrome/browser/ui/
  chrome/browser/prerender/

torne:
  android_webview/

benwells:
  chrome/browser/extensions/

rdsmith: (done)
  content/browser/download/
  chrome/browser/download/

nileshagrawal:
  content/public/browser/android/
  content/browser/android/
  chrome/browser/android/

joi:
  content/public/browser/

[nilesh, 2013-02-27 01:44:34]

> nileshagrawal:
>   content/public/browser/android/
>   content/browser/android/
>   chrome/browser/android/

LGTM

[Jói, 2013-02-27 09:45:36]

LGTM for content/public.

[Torne, 2013-02-27 10:01:18]

android_webview/ LGTM

[sky, 2013-02-27 15:28:43]

LGTM

[asanka, 2013-03-01 00:10:06]

Thanks everyone!

benwells:
  Ping?

sky:

  If you haven't already, would you mind taking a look at
  DefaultUIControllerDelegate::NotifyDownloadStarting() in
  chrome/browser/download/download_ui_controller.cc ? I forgot to
  mention it the first time around.

rdsmith:

  I think the changes to add a SavePackageRequestHandle weren't
  there when you gave the LGTM. It was necessary so that
  DownloadItems associated with SavePackage downloads would
  return the correct WebContents* in response to
  GetWebContents(). Would you mind taking a look? The relevant
  changes are in:

  content/browser/download/download_item_factory.h
  content/browser/download/download_item_impl.h
  content/browser/download/download_item_impl.cc
  content/browser/download/download_manager_impl.h
  content/browser/download/download_manager_impl.cc
  content/browser/download/save_package.cc

[sky, 2013-03-01 00:42:11]

SLGTM

[Randy Smith (Not in Mondays), 2013-03-01 16:56:02]

LGTM.

(I continue to be frustrated by how we use GetWebContents(), and would like to
find a way to get it out of the pool.  At core, downloads are associated with a
profile, and the web contents is only relevant to figure out what browser
windows to display them in.  This problem is going to have to be faced head-on
for resumption after browser restart--there really isn't any natural web
contents or browser window in that context.  But that is very much not this CL;
just remarking on it in case that gets you thinking about it in the back of your
mind as you do more UI refactoring.)

[benwells, 2013-03-05 00:18:36]

lgtm

[commit-bot: I haz the power, 2013-03-06 16:11:23]

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

[commit-bot: I haz the power, 2013-03-06 17:43:04]

Change committed as 186458