Add non-native file handling in FileSelectorHelper class.

Add non-native file handling in FileSelectorHelper class.

In chrome OS, the file chooser dialog box may return paths that does not point to a native file. The CL lets FileSelectorHelper handle for the non-native paths and generate FileChooserFileInfo that refers non-native files.

BUG=126902
TEST=manually test to upload drive/local files

Committed: https://crrev.com/275bfbeecf2d34a54ff086c40660b7bd271a4147
Cr-Commit-Position: refs/heads/master@{#300399}

[hirono, 2014-10-16 09:16:35]

hirono@chromium.org changed reviewers:
+ thakis@chromium.org

[hirono, 2014-10-16 09:16:35]

PTAL the CL? Thank you!

[hirono, 2014-10-16 09:19:34]

hirono@chromium.org changed reviewers:
+ avi@chromium.org, thestig@chromium.org
- thakis@chromium.org

[hirono, 2014-10-16 09:19:35]

PTAL the CL?

@avi - file_select_helper_mac.mm
@thestig - file_select_helper.{h,cc}

Thank you!

[Avi (use Gerrit), 2014-10-16 15:21:20]

I found the whole CL interesting :)

https://codereview.chromium.org/663473002/diff/1/chrome/browser/file_select_h...
File chrome/browser/file_select_helper.h (right):

https://codereview.chromium.org/663473002/diff/1/chrome/browser/file_select_h...
chrome/browser/file_select_helper.h:145: // Converts |files| into
FileChooserFileInfo with handling non-native files.
... with handling of non-native files.

https://codereview.chromium.org/663473002/diff/1/chrome/browser/file_select_h...
chrome/browser/file_select_helper.h:150: void
ProcessSelectedFilesChromeOSAfterConvresion(
...AfterConversion

https://codereview.chromium.org/663473002/diff/1/chrome/browser/file_select_h...
chrome/browser/file_select_helper.h:154: void ProcessSelectedFiles(const
std::vector<ui::SelectedFileInfo>& files);
Is ProcessSelectedFiles mutually exclusive with ProcessSelectedFilesChromeOS?
Would the former never be called on ChromeOS? If so, we shouldn't have two
functions but one. If not, then it's not clear which one should be called over
the other.

Where is the comment explaining the purpose here?

https://codereview.chromium.org/663473002/diff/1/chrome/browser/file_select_h...
chrome/browser/file_select_helper.h:157: // file chooser.
Is this no longer supposed to be called? It seems like you've moved callers to
ProcessSelectedFiles, so when is this now supposed to be called? Move this to
the private section if it's no longer supposed to be called from the outside.

[hirono, 2014-10-17 11:42:11]

Thank you!

https://codereview.chromium.org/663473002/diff/1/chrome/browser/file_select_h...
File chrome/browser/file_select_helper.h (right):

https://codereview.chromium.org/663473002/diff/1/chrome/browser/file_select_h...
chrome/browser/file_select_helper.h:145: // Converts |files| into
FileChooserFileInfo with handling non-native files.
On 2014/10/16 15:21:20, Avi wrote:
> ... with handling of non-native files.

Done.

https://codereview.chromium.org/663473002/diff/1/chrome/browser/file_select_h...
chrome/browser/file_select_helper.h:150: void
ProcessSelectedFilesChromeOSAfterConvresion(
On 2014/10/16 15:21:20, Avi wrote:
> ...AfterConversion

Done.

https://codereview.chromium.org/663473002/diff/1/chrome/browser/file_select_h...
chrome/browser/file_select_helper.h:154: void ProcessSelectedFiles(const
std::vector<ui::SelectedFileInfo>& files);
On 2014/10/16 15:21:20, Avi wrote:
> Is ProcessSelectedFiles mutually exclusive with ProcessSelectedFilesChromeOS?
> Would the former never be called on ChromeOS? If so, we shouldn't have two
> functions but one. If not, then it's not clear which one should be called over
> the other.
> 
> Where is the comment explaining the purpose here?

I combined the two function. Added a comment.

https://codereview.chromium.org/663473002/diff/1/chrome/browser/file_select_h...
chrome/browser/file_select_helper.h:157: // file chooser.
On 2014/10/16 15:21:20, Avi wrote:
> Is this no longer supposed to be called? It seems like you've moved callers to
> ProcessSelectedFiles, so when is this now supposed to be called? Move this to
> the private section if it's no longer supposed to be called from the outside.

Done.

[Avi (use Gerrit), 2014-10-17 19:38:52]

https://codereview.chromium.org/663473002/diff/20001/chrome/browser/file_sele...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/663473002/diff/20001/chrome/browser/file_sele...
chrome/browser/file_select_helper.cc:262: chooser_files[i].display_name =
files[i].display_name;
How does this work? chooser_files starts empty, and you seem to be addressing
non-existent elements here. The old NotifyRenderViewHost used push_back; you
will need to as well.

Plus, you could use for ( : ):

for (auto& file : files) {
  content::FileChooserFileInfo chooser_file;	
  chooser_file.file_path = file.local_path;	
  chooser_file.display_name = file.display_name;	
  chooser_files.push_back(chooser_file);
}

[hirono, 2014-10-20 04:10:48]

https://codereview.chromium.org/663473002/diff/20001/chrome/browser/file_sele...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/663473002/diff/20001/chrome/browser/file_sele...
chrome/browser/file_select_helper.cc:262: chooser_files[i].display_name =
files[i].display_name;
On 2014/10/17 19:38:52, Avi wrote:
> How does this work? chooser_files starts empty, and you seem to be addressing
> non-existent elements here. The old NotifyRenderViewHost used push_back; you
> will need to as well.
> 
> Plus, you could use for ( : ):
> 
> for (auto& file : files) {
>   content::FileChooserFileInfo chooser_file;	
>   chooser_file.file_path = file.local_path;	
>   chooser_file.display_name = file.display_name;	
>   chooser_files.push_back(chooser_file);
> }

Yes, this was completely wrong...
Fixed.

[Avi (use Gerrit), 2014-10-20 14:28:11]

lgtm

[Lei Zhang, 2014-10-20 19:07:51]

lgtm stamp

https://codereview.chromium.org/663473002/diff/60001/chrome/browser/file_sele...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/663473002/diff/60001/chrome/browser/file_sele...
chrome/browser/file_select_helper.cc:276: list ? *list :
std::vector<content::FileChooserFileInfo>(),
In a later CL, maybe make GetFileSystemContextForRenderViewHost() always return
a valid scoped_ptr to simplify the callbacks?

[Lei Zhang, 2014-10-20 19:14:31]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-10-20 19:16:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/663473002/60001

[commit-bot: I haz the power, 2014-10-20 19:38:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-20 19:38:25]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[hirono, 2014-10-21 01:41:14]

Thank you reviewers!

https://codereview.chromium.org/663473002/diff/60001/chrome/browser/file_sele...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/663473002/diff/60001/chrome/browser/file_sele...
chrome/browser/file_select_helper.cc:276: list ? *list :
std::vector<content::FileChooserFileInfo>(),
On 2014/10/20 19:07:50, Lei Zhang wrote:
> In a later CL, maybe make GetFileSystemContextForRenderViewHost() always
return
> a valid scoped_ptr to simplify the callbacks?

Got it. Thanks!

[hirono, 2014-10-21 01:41:18]

The CQ bit was checked by hirono@chromium.org

[commit-bot: I haz the power, 2014-10-21 01:43:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/663473002/60001

[commit-bot: I haz the power, 2014-10-21 01:46:36]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-21 01:47:23]

Patchset 4 (id:??) landed as
https://crrev.com/275bfbeecf2d34a54ff086c40660b7bd271a4147
Cr-Commit-Position: refs/heads/master@{#300399}