File manager drive API: handle case where no default task is associated.

File manager drive API: handle case where no default task is associated.

In GetDriveEntryPropertiesFunction::OnGetFileInfo(), we should not return failure when no
default task is associated with a particular MIME type. Instead we should still append all
apps that support the file type.

BUG=271570
TEST=manually on device, pass trybot
R=satorux@chromium.org, tbarzic@chromium.org

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

[hshi1, 2013-08-12 20:28:56]

Hi Satoru and Tomasz - PTAL

I believe it is incorrect to fail this call when no default task ID is found for
the given MIME type. This actually can happen if user has no default task set in
his Prefs, in this case we should still populate a list of app ID's that
supports the file.

[hshi1, 2013-08-12 20:36:10]

+Toni in case he's familiar with the file manager default task logic.

[tbarzic, 2013-08-12 21:09:49]

lgtm

https://codereview.chromium.org/22931003/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc
(right):

https://codereview.chromium.org/22931003/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:145:
file_tasks::ParseTaskID(default_task_id, &default_task);
afaik, ParseTaskID should never fail for non-empty default_task_id, so it may be
useful to add something along the line of:

DCHECK(default_task_id.empty() || !default_task.app_id.empty());

[hshi1, 2013-08-12 21:13:40]

https://codereview.chromium.org/22931003/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc
(right):

https://codereview.chromium.org/22931003/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:145:
file_tasks::ParseTaskID(default_task_id, &default_task);
On 2013/08/12 21:09:49, tbarzic wrote:
> afaik, ParseTaskID should never fail for non-empty default_task_id, so it may
be
> useful to add something along the line of:
> 
> DCHECK(default_task_id.empty() || !default_task.app_id.empty());

Done.

[commit-bot: I haz the power, 2013-08-12 21:23:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/22931003/22001

[satorux1, 2013-08-12 22:13:15]

oops. sorry for the regression. LGTM. we really need tests for this stuff.

[hshi1, 2013-08-12 22:26:12]

Committed patchset #2 manually as r217098 (presubmit successful).