Move OpenItem/ShowItemInFolder from ContentBrowserClient to DownloadManagerDelegate

Move OpenItem/ShowItemInFolder from ContentBrowserClient to DownloadManagerDelegate

ChromeDownloadManagerDelegate provides Profile which can be used to perform drive file operations.

MockDownloadOpenForTesting is moved from DownloadItem to DownloadManager.
Rename DownloadManager::DownloadOpened to OpenDownload.
Add DownloadManager::ShowDownloadInShell.

BUG=154713
TEST=git try
TBR=joth@chromium.org for android_webview

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

[hashimoto, 2013-01-18 11:41:32]

Could you review this?

This patch is a preparation for the fix of crbug.com/154713.
With ChromeDownloadManagerDelegate, we can use Profile and DownloadItem which
can be used to convert the temp file path to the drive path.

[Randy Smith (Not in Mondays), 2013-01-18 14:59:01]

Happy to take a look (later this morning) but content/public/browser changes
that aren't download related need someone other than me to look at them, and
there's no reason not to do that in parallel.  Pulling in John.

[Randy Smith (Not in Mondays), 2013-01-18 18:53:36]

Code-wise, this LGTM.  I'm a bit concerned on the architectural level of making
opening from the content layer a pure download function (which is what moving it
from ContentBrowserClient to DownloadManagerDelegate means) but the fact that
downloads is the only content level feature to make use of this function and
that I can't think of another possible use case on the horizon makes me think
that the move is actually the right thing architecturally.  

Still, I'm not sure I have the big enough picture view to make that call.  Which
dovetails nicely with my pulling John into this review :-}.  So please wait for
his review, but as far as I'm concerned, this is ok.

https://codereview.chromium.org/11941015/diff/2001/chrome/browser/download/do...
File chrome/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/11941015/diff/2001/chrome/browser/download/do...
chrome/browser/download/download_browsertest.cc:1466:
DownloadManagerForBrowser(browser())->MockDownloadOpenForTesting();
The better way to do this (given that the DMD now implements opening) is just to
mock a DMD and remove the whole "MockDownloadOpenForTesting()" concept on any
object.  You're welcome to do that or not as you'd like--it's not really related
to this CL, just goodness that's enabled by it.

[jam, 2013-01-18 21:29:28]

these functions seemed generic, which is why they weren't on
DownloadManagerDelegate.

have you considered adding a BrowserContext* parameter to these two methods
instead?

[hashimoto, 2013-01-19 03:41:54]

On 2013/01/18 21:29:28, John Abd-El-Malek wrote:
> these functions seemed generic, which is why they weren't on
> DownloadManagerDelegate.
> 
> have you considered adding a BrowserContext* parameter to these two methods
> instead?

Even though I can manage to achieve my goal if only Profile is available,
DownloadItem is also very helpful to resolve actual download paths from random
temporary file paths.

So there are three possible ways:
1) void ContentBrowserClient::OpenItem(BrowserContext*, const FilePath&);
2) void ContentBrowserClient::OpenItem(BrowserContext*, DownloadItem*);
3) void DownloadManagerDelegate::OpenDownload(DownloadItem*);

1 needs some additional work since I cannot use DownloadItem, 2 looks too
download oriented to be in ContentBrowserClient, 3 removes these methods from
ContentBrowseClient.

Since there was no other user of these methods, I thought 3 was the best. But if
we should maintain these methods in ContentBrowserClient for future use, I'd be
happy to write additional code to compensate the missing DownloadItem.
WDYT?

[jam, 2013-01-20 21:21:04]

On 2013/01/19 03:41:54, hashimoto wrote:
> On 2013/01/18 21:29:28, John Abd-El-Malek wrote:
> > these functions seemed generic, which is why they weren't on
> > DownloadManagerDelegate.
> > 
> > have you considered adding a BrowserContext* parameter to these two methods
> > instead?
> 
> Even though I can manage to achieve my goal if only Profile is available,
> DownloadItem is also very helpful to resolve actual download paths from random
> temporary file paths.
> 
> So there are three possible ways:
> 1) void ContentBrowserClient::OpenItem(BrowserContext*, const FilePath&);
> 2) void ContentBrowserClient::OpenItem(BrowserContext*, DownloadItem*);
> 3) void DownloadManagerDelegate::OpenDownload(DownloadItem*);
> 
> 1 needs some additional work since I cannot use DownloadItem, 2 looks too
> download oriented to be in ContentBrowserClient, 3 removes these methods from
> ContentBrowseClient.
> 
> Since there was no other user of these methods, I thought 3 was the best. But
if
> we should maintain these methods in ContentBrowserClient for future use, I'd
be
> happy to write additional code to compensate the missing DownloadItem.
> WDYT?

it wasn't clear to me that you need information from DownloadItem.Is that usage
happening in a future patch? if that is needed, then lgtm.

[hashimoto, 2013-01-21 04:44:20]

On 2013/01/20 21:21:04, John Abd-El-Malek wrote:
> On 2013/01/19 03:41:54, hashimoto wrote:
> > On 2013/01/18 21:29:28, John Abd-El-Malek wrote:
> > > these functions seemed generic, which is why they weren't on
> > > DownloadManagerDelegate.
> > > 
> > > have you considered adding a BrowserContext* parameter to these two
methods
> > > instead?
> > 
> > Even though I can manage to achieve my goal if only Profile is available,
> > DownloadItem is also very helpful to resolve actual download paths from
random
> > temporary file paths.
> > 
> > So there are three possible ways:
> > 1) void ContentBrowserClient::OpenItem(BrowserContext*, const FilePath&);
> > 2) void ContentBrowserClient::OpenItem(BrowserContext*, DownloadItem*);
> > 3) void DownloadManagerDelegate::OpenDownload(DownloadItem*);
> > 
> > 1 needs some additional work since I cannot use DownloadItem, 2 looks too
> > download oriented to be in ContentBrowserClient, 3 removes these methods
from
> > ContentBrowseClient.
> > 
> > Since there was no other user of these methods, I thought 3 was the best.
But
> if
> > we should maintain these methods in ContentBrowserClient for future use, I'd
> be
> > happy to write additional code to compensate the missing DownloadItem.
> > WDYT?
> 
> it wasn't clear to me that you need information from DownloadItem.Is that
usage
> happening in a future patch? if that is needed, then lgtm.

Yes, I'd like to make a patch using the DownloadItem after this.

Currently, on Chrome OS, a file Downloaded to a Google Drive directory cannot be
opened from chrome://downloads.
This is because the downloaded file is mapped to a random temporary file while
being downloaded.
To fix this, we should tell the original path (e.g. /drive/foo.jpg), instead of
the temporary file path (some randomly chosen name), to
platform_util::OpenItem().
Passing DownloadItem is the easiest solution to get the original path,
otherwise, we should implement a reverse map which restores the original path
from the temporary file path.

[jam, 2013-01-21 07:05:38]

On Sun, Jan 20, 2013 at 8:44 PM, <hashimoto@chromium.org> wrote:

> On 2013/01/20 21:21:04, John Abd-El-Malek wrote:
>
>> On 2013/01/19 03:41:54, hashimoto wrote:
>> > On 2013/01/18 21:29:28, John Abd-El-Malek wrote:
>> > > these functions seemed generic, which is why they weren't on
>> > > DownloadManagerDelegate.
>> > >
>> > > have you considered adding a BrowserContext* parameter to these two
>>
> methods
>
>> > > instead?
>> >
>> > Even though I can manage to achieve my goal if only Profile is
>> available,
>> > DownloadItem is also very helpful to resolve actual download paths from
>>
> random
>
>> > temporary file paths.
>> >
>> > So there are three possible ways:
>> > 1) void ContentBrowserClient::**OpenItem(BrowserContext*, const
>> FilePath&);
>> > 2) void ContentBrowserClient::**OpenItem(BrowserContext*,
>> DownloadItem*);
>> > 3) void DownloadManagerDelegate::**OpenDownload(DownloadItem*);
>> >
>> > 1 needs some additional work since I cannot use DownloadItem, 2 looks
>> too
>> > download oriented to be in ContentBrowserClient, 3 removes these methods
>>
> from
>
>> > ContentBrowseClient.
>> >
>> > Since there was no other user of these methods, I thought 3 was the
>> best.
>>
> But
>
>> if
>> > we should maintain these methods in ContentBrowserClient for future
>> use, I'd
>> be
>> > happy to write additional code to compensate the missing DownloadItem.
>> > WDYT?
>>
>
>  it wasn't clear to me that you need information from DownloadItem.Is that
>>
> usage
>
>> happening in a future patch? if that is needed, then lgtm.
>>
>
> Yes, I'd like to make a patch using the DownloadItem after this.
>
> Currently, on Chrome OS, a file Downloaded to a Google Drive directory
> cannot be
> opened from chrome://downloads.
> This is because the downloaded file is mapped to a random temporary file
> while
> being downloaded.
> To fix this, we should tell the original path (e.g. /drive/foo.jpg),
> instead of
> the temporary file path (some randomly chosen name), to
> platform_util::OpenItem().
> Passing DownloadItem is the easiest solution to get the original path,
> otherwise, we should implement a reverse map which restores the original
> path
> from the temporary file path.
>

i see, lgtm then (this wasn't clear to me because this patch didn't make
use of DownloadItem)

>
>
https://codereview.chromium.**org/11941015/<https://codereview.chromium.org/1...
>

[hashimoto, 2013-01-21 10:54:25]

https://codereview.chromium.org/11941015/diff/2001/chrome/browser/download/do...
File chrome/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/11941015/diff/2001/chrome/browser/download/do...
chrome/browser/download/download_browsertest.cc:1466:
DownloadManagerForBrowser(browser())->MockDownloadOpenForTesting();
On 2013/01/18 18:53:37, rdsmith wrote:
> The better way to do this (given that the DMD now implements opening) is just
to
> mock a DMD and remove the whole "MockDownloadOpenForTesting()" concept on any
> object.  You're welcome to do that or not as you'd like--it's not really
related
> to this CL, just goodness that's enabled by it.

Filed in http://crbug.com/171224.

[commit-bot: I haz the power, 2013-01-21 10:54:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/11941015/2001

[commit-bot: I haz the power, 2013-01-21 10:54:46]

Presubmit check for 11941015-2001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    android_webview

Presubmit checks took 2.3s to calculate.

[hashimoto, 2013-01-21 10:59:44]

TBR-ing joth@chromium.org for deletion of two empty methods of
AwContentBrowserClient

[commit-bot: I haz the power, 2013-01-21 10:59:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/11941015/2001

[commit-bot: I haz the power, 2013-01-21 15:32:16]

Change committed as 177920