Move closing-blank-tab logic in Browser class to DownloadUIConttroller

Move closing-blank-tab logic in Browser class to DownloadUIConttroller

With download notification, Browser::ShowDownload was never called when a download starts, hence a new blank tab was not closed. This patch refactors the code around and moves the code to DownloadUIConttroller class.

BUG=510821
TEST=browser_tests and unit_tests passes

Committed: https://crrev.com/09481b7c83c939fe6e639ed500f2b635d5fcbb91
Cr-Commit-Position: refs/heads/master@{#339210}

[yoshiki, 2015-07-16 14:38:49]

Patchset #1 (id:1) has been deleted

[yoshiki, 2015-07-16 14:49:58]

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

[yoshiki, 2015-07-16 14:50:14]

Asanka, PTAL. Thanks.

[asanka, 2015-07-16 19:28:15]

LGTM % comment.

You'll need an owner for chrome/browser/

https://codereview.chromium.org/1236003005/diff/20001/chrome/browser/download...
File chrome/browser/download/download_ui_controller.cc (right):

https://codereview.chromium.org/1236003005/diff/20001/chrome/browser/download...
chrome/browser/download/download_ui_controller.cc:93: if
(DownloadItemModel(item).ShouldShowInShelf()) {
Fold this condition to the previous if().

[yoshiki, 2015-07-16 23:58:14]

Thanks!

https://codereview.chromium.org/1236003005/diff/20001/chrome/browser/download...
File chrome/browser/download/download_ui_controller.cc (right):

https://codereview.chromium.org/1236003005/diff/20001/chrome/browser/download...
chrome/browser/download/download_ui_controller.cc:93: if
(DownloadItemModel(item).ShouldShowInShelf()) {
On 2015/07/16 19:28:15, asanka wrote:
> Fold this condition to the previous if().

Done.

[yoshiki, 2015-07-17 00:00:10]

yoshiki@chromium.org changed reviewers:
+ msw@chromium.org

[yoshiki, 2015-07-17 00:00:10]

+msw, PTAL at changes in C/B/ui/browser.[cc,h]?

[msw, 2015-07-17 01:15:35]

https://codereview.chromium.org/1236003005/diff/40001/chrome/browser/download...
File chrome/browser/download/download_ui_controller.cc (right):

https://codereview.chromium.org/1236003005/diff/40001/chrome/browser/download...
chrome/browser/download/download_ui_controller.cc:92: // Some (app downloads)
are not supposed to appear on the shelf.
nit: please move this comment above the start of the conditional
(you can remove either of these comments, neither adds much value)

https://codereview.chromium.org/1236003005/diff/40001/chrome/browser/download...
chrome/browser/download/download_ui_controller.cc:139: void
DownloadUIController::OnDownloadUpdated(content::DownloadManager* manager,
OnDownloadUpdated is called (via DownloadItemImpl::UpdateObservers) for *many*
reasons; is there any reason to not keep the tab closing code in
OnNewDownloadReady, which seems like a more specific event? Beyond the change in
behavior that might lead to a regression, I worry that this code might now run
every time a download's progress advances (or similar), where it might even
introduce a [minor?] performance regression (think with N=10+ downloads...).
Sorry if my concerns are unfounded, but I'm not super familiar with this code.

https://codereview.chromium.org/1236003005/diff/40001/chrome/browser/download...
chrome/browser/download/download_ui_controller.cc:168: web_contents->Close();
Will changing Browser::CloseContents to WebContents::Close have any impact?
(I'd expect that there isn't any meaningful difference, but thought I'd ask)

[yoshiki, 2015-07-17 01:40:04]

msw, PTAL. Thanks.

https://codereview.chromium.org/1236003005/diff/40001/chrome/browser/download...
File chrome/browser/download/download_ui_controller.cc (right):

https://codereview.chromium.org/1236003005/diff/40001/chrome/browser/download...
chrome/browser/download/download_ui_controller.cc:92: // Some (app downloads)
are not supposed to appear on the shelf.
On 2015/07/17 01:15:34, msw wrote:
> nit: please move this comment above the start of the conditional
> (you can remove either of these comments, neither adds much value)

Done. Removed the first comment.

https://codereview.chromium.org/1236003005/diff/40001/chrome/browser/download...
chrome/browser/download/download_ui_controller.cc:139: void
DownloadUIController::OnDownloadUpdated(content::DownloadManager* manager,
On 2015/07/17 01:15:35, msw wrote:
> OnDownloadUpdated is called (via DownloadItemImpl::UpdateObservers) for *many*
> reasons; is there any reason to not keep the tab closing code in
> OnNewDownloadReady, which seems like a more specific event? Beyond the change
in
> behavior that might lead to a regression, I worry that this code might now run
> every time a download's progress advances (or similar), where it might even
> introduce a [minor?] performance regression (think with N=10+ downloads...).
> Sorry if my concerns are unfounded, but I'm not super familiar with this code.

Thank you for pointing out.

But any new performance problem is not introduced by this patch. Because
WasUINotified() check (l.145 in new code) skips the further logic if it's
non-first update. And OnNewDownloadReady is called at the last of this method,
so the number of call has not been changed by this patch.

The motivation of moving this to here is that OnNewDownloadReady is one of the
delegate methods. The delegate is specialized for kind of UI so we want to write
the common code in this class.

https://codereview.chromium.org/1236003005/diff/40001/chrome/browser/download...
chrome/browser/download/download_ui_controller.cc:168: web_contents->Close();
On 2015/07/17 01:15:34, msw wrote:
> Will changing Browser::CloseContents to WebContents::Close have any impact?
> (I'd expect that there isn't any meaningful difference, but thought I'd ask)

WebContents::Close calls Browser::CloseContents internally. The only difference
is a RenderViewHost check
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...).
Hence I think there is no diffirence.

[msw, 2015-07-17 02:11:33]

lgtm

https://codereview.chromium.org/1236003005/diff/40001/chrome/browser/download...
File chrome/browser/download/download_ui_controller.cc (right):

https://codereview.chromium.org/1236003005/diff/40001/chrome/browser/download...
chrome/browser/download/download_ui_controller.cc:139: void
DownloadUIController::OnDownloadUpdated(content::DownloadManager* manager,
On 2015/07/17 01:40:04, yoshiki wrote:
> On 2015/07/17 01:15:35, msw wrote:
> > OnDownloadUpdated is called (via DownloadItemImpl::UpdateObservers) for
*many*
> > reasons; is there any reason to not keep the tab closing code in
> > OnNewDownloadReady, which seems like a more specific event? Beyond the
change
> in
> > behavior that might lead to a regression, I worry that this code might now
run
> > every time a download's progress advances (or similar), where it might even
> > introduce a [minor?] performance regression (think with N=10+ downloads...).
> > Sorry if my concerns are unfounded, but I'm not super familiar with this
code.
> 
> Thank you for pointing out.
> 
> But any new performance problem is not introduced by this patch. Because
> WasUINotified() check (l.145 in new code) skips the further logic if it's
> non-first update. And OnNewDownloadReady is called at the last of this method,
> so the number of call has not been changed by this patch.
> 
> The motivation of moving this to here is that OnNewDownloadReady is one of the
> delegate methods. The delegate is specialized for kind of UI so we want to
write
> the common code in this class. 

Ah, I totally missed that, thanks for the explanation.

https://codereview.chromium.org/1236003005/diff/40001/chrome/browser/download...
chrome/browser/download/download_ui_controller.cc:168: web_contents->Close();
On 2015/07/17 01:40:04, yoshiki wrote:
> On 2015/07/17 01:15:34, msw wrote:
> > Will changing Browser::CloseContents to WebContents::Close have any impact?
> > (I'd expect that there isn't any meaningful difference, but thought I'd ask)
> 
> WebContents::Close calls Browser::CloseContents internally. The only
difference
> is a RenderViewHost check
>
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...).
> Hence I think there is no diffirence. 

Acknowledged.

[yoshiki, 2015-07-17 02:31:22]

Thanks!

[yoshiki, 2015-07-17 02:31:27]

The CQ bit was checked by yoshiki@chromium.org

[yoshiki, 2015-07-17 02:31:27]

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/1236003005/#ps60001
(title: "Addressed comments")

[commit-bot: I haz the power, 2015-07-17 02:33:32]

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

[commit-bot: I haz the power, 2015-07-17 03:12:47]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-07-17 03:13:48]

Patchset 3 (id:??) landed as
https://crrev.com/09481b7c83c939fe6e639ed500f2b635d5fcbb91
Cr-Commit-Position: refs/heads/master@{#339210}