ChromeOS: Support opening files with apps from the downloads page.

ChromeOS: Support opening files with apps from the downloads page.

This allows downloaded files that can be handled by apps to be opened
directly from the downloads page or bar without going through the file
browser. Previously, only extensions and built-in handlers were used.

BUG=237305
TEST=See repro in the bug.

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

[Matt Giuca, 2013-05-09 04:58:22]

https://codereview.chromium.org/14670023/diff/2001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc
(right):

https://codereview.chromium.org/14670023/diff/2001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:453: for
(ExtensionSet::const_iterator iter = service->extensions()->begin();
Explain the algorithm overall.

// If we find the default handler, we execute it immediately, but otherwise, we
remember the first handler, and if there was no default handler, simply execute
the first one.

https://codereview.chromium.org/14670023/diff/2001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:462: if
(profile->IsOffTheRecord() &&
What does this mean? Can you add a comment?

https://codereview.chromium.org/14670023/diff/2001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:497:
std::string mime_type = GetMimeTypeForPath(path);
Explain the approach you are taking in comments here.

https://codereview.chromium.org/14670023/diff/2001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:501: if
(!file_handler_util::GetTaskForURLAndPath(profile, url, path, &handler)) {
I think this could be clearer. It seems as though there are two possibilities:
1. It is handled by an extension.
2. Else, it is handled by an app.
3. Else, fail.

The code currently (in this CL) is structured like this:

if (it can not be handled by an extension) {
  do app handling stuff in a subroutine (succeed or fail)
}
do extension handling stuff right here (succeed or fail)

It seems like a more readable approach would be:

if (it can be handled by an extension) {
  do extension handling stuff in a subroutine (succeed or fail)
} else {
  do app handling stuff in a subroutine (succeed or fail)
}

This would involve moving all of the code after this if statement out into
another function, ExecuteDefaultExtensionHandler, but I think it would be more
understandable.

[Sam McNally, 2013-05-09 07:31:35]

https://codereview.chromium.org/14670023/diff/2001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc
(right):

https://codereview.chromium.org/14670023/diff/2001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:453: for
(ExtensionSet::const_iterator iter = service->extensions()->begin();
On 2013/05/09 04:58:22, Matt Giuca wrote:
> Explain the algorithm overall.
> 
> // If we find the default handler, we execute it immediately, but otherwise,
we
> remember the first handler, and if there was no default handler, simply
execute
> the first one.

Done.

https://codereview.chromium.org/14670023/diff/2001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:462: if
(profile->IsOffTheRecord() &&
On 2013/05/09 04:58:22, Matt Giuca wrote:
> What does this mean? Can you add a comment?

Done.

https://codereview.chromium.org/14670023/diff/2001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:497:
std::string mime_type = GetMimeTypeForPath(path);
On 2013/05/09 04:58:22, Matt Giuca wrote:
> Explain the approach you are taking in comments here.

Done.

https://codereview.chromium.org/14670023/diff/2001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:501: if
(!file_handler_util::GetTaskForURLAndPath(profile, url, path, &handler)) {
On 2013/05/09 04:58:22, Matt Giuca wrote:
> I think this could be clearer. It seems as though there are two possibilities:
> 1. It is handled by an extension.
> 2. Else, it is handled by an app.
> 3. Else, fail.
> 
> The code currently (in this CL) is structured like this:
> 
> if (it can not be handled by an extension) {
>   do app handling stuff in a subroutine (succeed or fail)
> }
> do extension handling stuff right here (succeed or fail)
> 
> It seems like a more readable approach would be:
> 
> if (it can be handled by an extension) {
>   do extension handling stuff in a subroutine (succeed or fail)
> } else {
>   do app handling stuff in a subroutine (succeed or fail)
> }
> 
> This would involve moving all of the code after this if statement out into
> another function, ExecuteDefaultExtensionHandler, but I think it would be more
> understandable.

Done.

[Matt Giuca, 2013-05-09 08:56:57]

lgtm

https://codereview.chromium.org/14670023/diff/2001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc
(right):

https://codereview.chromium.org/14670023/diff/2001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:462: if
(profile->IsOffTheRecord() &&
Can you put quotes around "incognito: split". It's hard to read as is.

[Sam McNally, 2013-05-09 23:27:44]

https://codereview.chromium.org/14670023/diff/2001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc
(right):

https://codereview.chromium.org/14670023/diff/2001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:462: if
(profile->IsOffTheRecord() &&
On 2013/05/09 08:56:58, Matt Giuca wrote:
> Can you put quotes around "incognito: split". It's hard to read as is.

Done.

[Sam McNally, 2013-05-15 04:34:57]

ping

[Matt Giuca, 2013-06-07 01:12:03]

On 2013/05/15 04:34:57, Sam McNally wrote:
> ping

Can we find another reviewer for this?

[Sam McNally, 2013-06-07 01:55:34]

+mtomasz

[mtomasz, 2013-06-07 02:36:00]

https://codereview.chromium.org/14670023/diff/40001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc
(right):

https://codereview.chromium.org/14670023/diff/40001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:440: // We
don't support using hosted apps to open files.
This comment is misleading. is_platform_app disallows not only hosted apps but
also legacy packaged apps? Do we want to disallow legacy packaged apps?

https://codereview.chromium.org/14670023/diff/40001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:444: // We
only support apps that specify "incognito: split" if in incognito
I'm wondering if this behavior is documented somewhere so 3rd party engineers
will know why their state-less simple app is never handling files in incognito.

https://codereview.chromium.org/14670023/diff/40001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:534: // 1.
default extension
default extensions before apps, but non-default apps before non-default
extensions? Is it correct?

[Sam McNally, 2013-06-07 05:29:13]

https://codereview.chromium.org/14670023/diff/40001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc
(right):

https://codereview.chromium.org/14670023/diff/40001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:440: // We
don't support using hosted apps to open files.
On 2013/06/07 02:36:00, mtomasz wrote:
> This comment is misleading. is_platform_app disallows not only hosted apps but
> also legacy packaged apps? Do we want to disallow legacy packaged apps?

This is copied from the check in file_manager_private_api. It looks like this
check is correct; file handlers aren't supported by legacy packaged apps.

https://codereview.chromium.org/14670023/diff/40001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:444: // We
only support apps that specify "incognito: split" if in incognito
On 2013/06/07 02:36:00, mtomasz wrote:
> I'm wondering if this behavior is documented somewhere so 3rd party engineers
> will know why their state-less simple app is never handling files in
incognito.

Same as above. This check may be unnecessary though; incognito: spanning isn't
allowed for non-legacy packaged apps.

https://codereview.chromium.org/14670023/diff/40001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:534: // 1.
default extension
On 2013/06/07 02:36:00, mtomasz wrote:
> default extensions before apps, but non-default apps before non-default
> extensions? Is it correct?

Yes, but there can be at most one default handler.

[mtomasz, 2013-06-07 05:37:00]

On 2013/06/07 05:29:13, Sam McNally wrote:
>
https://codereview.chromium.org/14670023/diff/40001/chrome/browser/chromeos/e...
> File chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc
> (right):
> 
>
https://codereview.chromium.org/14670023/diff/40001/chrome/browser/chromeos/e...
> chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:440: //
We
> don't support using hosted apps to open files.
> On 2013/06/07 02:36:00, mtomasz wrote:
> > This comment is misleading. is_platform_app disallows not only hosted apps
but
> > also legacy packaged apps? Do we want to disallow legacy packaged apps?
> 
> This is copied from the check in file_manager_private_api. It looks like this
> check is correct; file handlers aren't supported by legacy packaged apps.
> 
>
https://codereview.chromium.org/14670023/diff/40001/chrome/browser/chromeos/e...
> chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:444: //
We
> only support apps that specify "incognito: split" if in incognito
> On 2013/06/07 02:36:00, mtomasz wrote:
> > I'm wondering if this behavior is documented somewhere so 3rd party
engineers
> > will know why their state-less simple app is never handling files in
> incognito.
> 
> Same as above. This check may be unnecessary though; incognito: spanning isn't
> allowed for non-legacy packaged apps.
> 
>
https://codereview.chromium.org/14670023/diff/40001/chrome/browser/chromeos/e...
> chrome/browser/chromeos/extensions/file_manager/file_manager_util.cc:534: //
1.
> default extension
> On 2013/06/07 02:36:00, mtomasz wrote:
> > default extensions before apps, but non-default apps before non-default
> > extensions? Is it correct?
> 
> Yes, but there can be at most one default handler.

lgtm

[commit-bot: I haz the power, 2013-06-07 05:38:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/14670023/40001

[commit-bot: I haz the power, 2013-06-07 07:12:56]

Retried try job too often on win_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-06-07 07:16:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/14670023/40001

[commit-bot: I haz the power, 2013-06-07 12:30:00]

Change committed as 204788