file_manager: Unify the file task selection logic

file_manager: Unify the file task selection logic

Previously, open_util.cc and file_tasks.cc had two separate
implementations of file task selection logic. The one in open_util.cc
was unclean had an issue with the download shelf (issue 279771).
Not to mention, having two separate implementations for doing the
same thing is not good for code health.

This patch replaces the unclean logic in open_util.cc with the one
in file_tasks.cc (FindAllTypesOfTasks()).

BUG=279771, 278037, 284194
TEST=See steps described in issue 279771
R=kinaba@chromium.org

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

[satorux1, 2013-09-03 05:19:54]

this removes lots of code.

[kinaba, 2013-09-03 07:27:38]

https://codereview.chromium.org/23857004/diff/7001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_manager/open_util.cc (right):

https://codereview.chromium.org/23857004/diff/7001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/open_util.cc:138: // taskfor the
file. Returns false if failed to open the file (i.e. no file
nit: taskfor -> task for

https://codereview.chromium.org/23857004/diff/7001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/open_util.cc:146: std::string
mime_type = GetMimeTypeForPath(file_path);
This is getting the mime type from path, so I guess the issue similar to bug
280442 (extension-less files cannot be opened) remains for the download shelf.
Could you leave some comment?

https://codereview.chromium.org/23857004/diff/7001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/open_util.cc:157:
file_tasks::FindAllTypesOfTasks(profile,
It looks FindAllTypesOfTasks always include DriveApps? What happens if the path
points to a local file in Downloads folder?

https://codereview.chromium.org/23857004/diff/7001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/open_util.cc:166: for (size_t i
=0; i < tasks.size(); ++i) {
nit: "i =0" => "i = 0"

[satorux1, 2013-09-03 08:07:30]

https://codereview.chromium.org/23857004/diff/7001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_manager/open_util.cc (right):

https://codereview.chromium.org/23857004/diff/7001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/open_util.cc:138: // taskfor the
file. Returns false if failed to open the file (i.e. no file
On 2013/09/03 07:27:39, kinaba wrote:
> nit: taskfor -> task for

Done.

https://codereview.chromium.org/23857004/diff/7001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/open_util.cc:146: std::string
mime_type = GetMimeTypeForPath(file_path);
On 2013/09/03 07:27:39, kinaba wrote:
> This is getting the mime type from path, so I guess the issue similar to bug
> 280442 (extension-less files cannot be opened) remains for the download shelf.
> Could you leave some comment?

Done.

https://codereview.chromium.org/23857004/diff/7001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/open_util.cc:157:
file_tasks::FindAllTypesOfTasks(profile,
On 2013/09/03 07:27:39, kinaba wrote:
> It looks FindAllTypesOfTasks always include DriveApps? What happens if the
path
> points to a local file in Downloads folder?

Good catch!

We have a similar problem with the file action menu in Downloads. I checked that
fileBrowserPrivate.getFileTasks() returns Drive app tasks for files on Downloads
(oops!), but these tasks are filtered in file_tasks.js:


FileTasks.prototype.processTasks_ = function(tasks) {
  ...
    // Skip Drive App if the file is not on Drive.                              
    if (!isOnDrive && task.driveApp)
      continue;


I think fileBrowserPrivate.getFileTasks() (and FindAllTypesOfTasks) should not
return Drive apps for files not on Drive. Let me file a bug and fix it before
this patch.

https://codereview.chromium.org/23857004/diff/7001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/open_util.cc:166: for (size_t i
=0; i < tasks.size(); ++i) {
On 2013/09/03 07:27:39, kinaba wrote:
> nit: "i =0" => "i = 0"

Done.

[satorux1, 2013-09-04 04:19:42]

https://codereview.chromium.org/23857004/diff/7001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_manager/open_util.cc (right):

https://codereview.chromium.org/23857004/diff/7001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/open_util.cc:157:
file_tasks::FindAllTypesOfTasks(profile,
On 2013/09/03 08:07:30, satorux1 wrote:
> On 2013/09/03 07:27:39, kinaba wrote:
> > It looks FindAllTypesOfTasks always include DriveApps? What happens if the
> path
> > points to a local file in Downloads folder?
> 
> Good catch!
> 
> We have a similar problem with the file action menu in Downloads. I checked
that
> fileBrowserPrivate.getFileTasks() returns Drive app tasks for files on
Downloads
> (oops!), but these tasks are filtered in file_tasks.js:
> 
> 
> FileTasks.prototype.processTasks_ = function(tasks) {
>   ...
>     // Skip Drive App if the file is not on Drive.                            
 
>     if (!isOnDrive && task.driveApp)
>       continue;
> 
> 
> I think fileBrowserPrivate.getFileTasks() (and FindAllTypesOfTasks) should not
> return Drive apps for files not on Drive. Let me file a bug and fix it before
> this patch.

Fixed the issue at https://codereview.chromium.org/23477036/

[kinaba, 2013-09-04 05:08:32]

lgtm

[satorux1, 2013-09-05 03:31:30]

Committed patchset #5 manually as r221336 (presubmit successful).