Change the meaning of SelectFileDialog.support_drive: it means Drive-aware callers.

Change the meaning of SelectFileDialog.support_drive: it means Drive-aware callers.

Previously, what the |support_drive| flag means is just to show
Google Drive tab in the file dialog; it doesn't change other
behaviors.

Now, we are going to enable the "Google Drive" tab on Chrome OS
for all call sites of the dialog, by adding a feature to Drive client
to provide local snapshot so that callers need not care about Drive.

On the other hand, the "create snapshot" trick is unnecessary for
clients like Packaged app or extensions that implement fileBrowserHandler,
which access files through HTML5 FileSystem API.
So, for those clients I want to leave the |support_drive| flag to
indicate that no special handling is needed. Here's the summary:

Before:
- support_drive, folder-dialog : show Drive tab. No special handling.
- support_drive, open-dialog : show Drive tab. Create local snapshot.
- support_drive, save-dialog : show Drive tab. No special handling.
- !support_drive, folder-dialog : hide Drive tab.
- !support_drive, open-dialog : hide Drive tab. (this mode is not used)
- !support_drive, save-dialog : hide Drive tab.

After:
- support_drive, folder-dialog : show Drive tab. No special handling.
- support_drive, open-dialog : show Drive tab. No special handling.
- support_drive, save-dialog : show Drive tab. No special handling.
- !support_drive, folder-dialog : *HIDE* Drive tab. (this is not supported yet)
- !support_drive, open-dialog : Show Drive tab. Create local snapshot.
- !support_drive, save-dialog : *HIDE* Drive tab. (VERY SOON switch to => "Show Drive tab. Create local snapshot.")

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

[satorux1, 2013-08-06 02:07:17]

Finally this is coming!

https://codereview.chromium.org/22165002/diff/11001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_manager/private_api_util.cc
(right):

https://codereview.chromium.org/22165002/diff/11001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_manager/private_api_util.cc:56: // When
opening a drive file, we should get local file path.
the comment needs to be updated? now need_local_path is also checked.

https://codereview.chromium.org/22165002/diff/11001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_manager/private_api_util.h (right):

https://codereview.chromium.org/22165002/diff/11001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_manager/private_api_util.h:64: bool
need_local_path,
Maybe use a bitmask? Then, the callers would be clearer.

enum GetSelectedFileInfoOptions {
  OPTION_NONE = 0,
  OPTION_FOR_OPENING = 1,
  OPTION_NEED_LOCAL_PATH = 2,
}

https://codereview.chromium.org/22165002/diff/11001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://codereview.chromium.org/22165002/diff/11001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/file_manager.js:2028: var
clientCanHandleDrive =
Maybe return here?

if (noLocalPathResolution && this.params_.shouldReturnLocalPath)
  return false;

https://codereview.chromium.org/22165002/diff/11001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/file_manager.js:2032: (!('driveEnabled'
in this.preferences_) ||
Should we check this ('driveEnabled' not in this.preferences_)? If so, it'd be
nice to add some comment. Otherwise, can remove this.

Alternatively: this.preferences_.driveEnabled != false;

https://codereview.chromium.org/22165002/diff/11001/chrome/browser/ui/certifi...
File chrome/browser/ui/certificate_dialogs.cc (right):

https://codereview.chromium.org/22165002/diff/11001/chrome/browser/ui/certifi...
chrome/browser/ui/certificate_dialogs.cc:175: suggested_path, &file_type_info,
1,
While you are at it, could you add a comment explaining what '1' means here?

https://codereview.chromium.org/22165002/diff/11001/ui/shell_dialogs/select_f...
File ui/shell_dialogs/select_file_dialog.h (right):

https://codereview.chromium.org/22165002/diff/11001/ui/shell_dialogs/select_f...
ui/shell_dialogs/select_file_dialog.h:132: // of selected files so that the
caller can operate on it transparently.
More comment? If I understand correctly, Drive is now supported in most cases,
but there would be two ways to handle Drive.

Maybe rename the flag to something more relevant to the new meaning?

[kinaba, 2013-08-06 06:20:56]

https://codereview.chromium.org/22165002/diff/11001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_manager/private_api_util.cc
(right):

https://codereview.chromium.org/22165002/diff/11001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_manager/private_api_util.cc:56: // When
opening a drive file, we should get local file path.
On 2013/08/06 02:07:17, satorux1 wrote:
> the comment needs to be updated? now need_local_path is also checked.

Done.

https://codereview.chromium.org/22165002/diff/11001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_manager/private_api_util.h (right):

https://codereview.chromium.org/22165002/diff/11001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_manager/private_api_util.h:64: bool
need_local_path,
On 2013/08/06 02:07:17, satorux1 wrote:
> Maybe use a bitmask? Then, the callers would be clearer.
> 
> enum GetSelectedFileInfoOptions {
>   OPTION_NONE = 0,
>   OPTION_FOR_OPENING = 1,
>   OPTION_NEED_LOCAL_PATH = 2,
> }

Done.
I changed to a simple enum rather than a bitmask,
since when |need_local_path==false| the other bit becomes irrelevant.

https://codereview.chromium.org/22165002/diff/11001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://codereview.chromium.org/22165002/diff/11001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/file_manager.js:2028: var
clientCanHandleDrive =
On 2013/08/06 02:07:17, satorux1 wrote:
> Maybe return here?
> 
> if (noLocalPathResolution && this.params_.shouldReturnLocalPath)
>   return false;

Done.

https://codereview.chromium.org/22165002/diff/11001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/file_manager.js:2032: (!('driveEnabled'
in this.preferences_) ||
On 2013/08/06 02:07:17, satorux1 wrote:
> Should we check this ('driveEnabled' not in this.preferences_)? If so, it'd be
> nice to add some comment. Otherwise, can remove this.
> 
> Alternatively: this.preferences_.driveEnabled != false;

Done.

https://codereview.chromium.org/22165002/diff/11001/chrome/browser/ui/certifi...
File chrome/browser/ui/certificate_dialogs.cc (right):

https://codereview.chromium.org/22165002/diff/11001/chrome/browser/ui/certifi...
chrome/browser/ui/certificate_dialogs.cc:175: suggested_path, &file_type_info,
1,
On 2013/08/06 02:07:17, satorux1 wrote:
> While you are at it, could you add a comment explaining what '1' means here?

Done.

https://codereview.chromium.org/22165002/diff/11001/ui/shell_dialogs/select_f...
File ui/shell_dialogs/select_file_dialog.h (right):

https://codereview.chromium.org/22165002/diff/11001/ui/shell_dialogs/select_f...
ui/shell_dialogs/select_file_dialog.h:132: // of selected files so that the
caller can operate on it transparently.
On 2013/08/06 02:07:17, satorux1 wrote:
> More comment? If I understand correctly, Drive is now supported in most cases,
> but there would be two ways to handle Drive.
> 
> Maybe rename the flag to something more relevant to the new meaning?

Added more comments.
I think the name is still relevant. Turning off the flag indicates
that the caller does not handle Drive files so the file dialog must
do something.

[satorux1, 2013-08-06 07:39:09]

LGTM

[kinaba, 2013-08-06 08:41:39]

+sky as the OWNER of chrome/ and ui/.
Could you take a quick look?

This is a part of the effort for removing a hack introduced for
supporting Google Drive integration in Chrome OS save file dialog.

The changes outside **/file_manager/ are just removing lines
".support_drive = true;" since such a distinction becomes unneeded
and without any care from the caller site it just works.

[sky, 2013-08-06 14:33:00]

LGTM

[commit-bot: I haz the power, 2013-08-06 15:10:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinaba@chromium.org/22165002/19002

[commit-bot: I haz the power, 2013-08-06 17:27:02]

Change committed as 215922