Move fileSystem API FilePicker class to own file

Move fileSystem API FilePicker class to own file

The chrome.fileSystem API impl has a FilePicker class used to create
and show a "Choose a file/directory" dialog. It's not tiny and it
manages its own lifetime, so I'm extracting it into a separate file.

This will unblock https://codereview.chromium.org/2934143002/ by
allowing us to easily override the file picking behavior in extension
tests without having to depend on Chrome-specific code.

BUG=729713
R=rdevlin.cronin@chromium.org

Review-Url: https://codereview.chromium.org/2967223002
Cr-Commit-Position: refs/heads/master@{#484673}
Committed: https://chromium.googlesource.com/chromium/src/+/b35720f5e102a25733b3dce0daa1fafda680a5bb

[michaelpg, 2017-07-05 18:31:37]

Description was changed from

==========
Move fileSystem API FilePicker class to own file

This will unblock https://codereview.chromium.org/2934143002/ by
allowing us to easily override the file picking behavior in extension
tests.

BUG=729713
R=benwells@chromium.org
==========

to

==========
Move fileSystem API FilePicker class to own file

This will unblock https://codereview.chromium.org/2934143002/ by
allowing us to easily override the file picking behavior in extension
tests.

BUG=729713
R=rdevlin.cronin@chromium.org
==========

[michaelpg, 2017-07-05 18:31:37]

michaelpg@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org
- benwells@chromium.org

[michaelpg, 2017-07-05 18:33:28]

Devlin: PTAL. This is a new part of  https://codereview.chromium.org/2934143002/
that I would rather land first as its own chunk. Later, the FileSystemDelegate
I'm adding will be creating this dialog rather than the API function itself
doing so.

You know the drill.... most of the code is cut and pasted, but Rietveld has
trouble identifying it.

[michaelpg, 2017-07-05 18:34:55]

Description was changed from

==========
Move fileSystem API FilePicker class to own file

This will unblock https://codereview.chromium.org/2934143002/ by
allowing us to easily override the file picking behavior in extension
tests.

BUG=729713
R=rdevlin.cronin@chromium.org
==========

to

==========
Move fileSystem API FilePicker class to own file

The chrome.fileSystem API impl has a FilePicker class used to create
and show a "Choose a file/directory" dialog. It's not tiny and it
manages its own lifetime, so I'm extracting it into a separate file.

This will unblock https://codereview.chromium.org/2934143002/ by
allowing us to easily override the file picking behavior in extension
tests without having to depend on Chrome-specific code.

BUG=729713
R=rdevlin.cronin@chromium.org
==========

[Devlin, 2017-07-05 19:08:05]

Nice!

https://codereview.chromium.org/2967223002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_entry_picker.cc (right):

https://codereview.chromium.org/2967223002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_entry_picker.cc:47:
MultiFilesSelected(paths, params);
nit: MultiFilesSelected(std::vector<base::FilePath>(path, 1));

https://codereview.chromium.org/2967223002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_entry_picker.h (right):

https://codereview.chromium.org/2967223002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_entry_picker.h:29:
base::OnceCallback<void(const std::vector<base::FilePath>& paths)>;
nit: \n

https://codereview.chromium.org/2967223002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_entry_picker.h:30:
FileEntryPicker(content::WebContents* web_contents,
Document, either here or above the member variables, what these variables
mean/are used for.

https://codereview.chromium.org/2967223002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_entry_picker.h:34:
FilesSelectedCallback&& files_selected_callback,
nit: maybe just pass by value here, and std::move it?

https://codereview.chromium.org/2967223002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_entry_picker.h:36:
~FileEntryPicker() override;
Can we make this private?

https://codereview.chromium.org/2967223002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_entry_picker.h:43: 
no \n between overridden methods from the same class.

https://codereview.chromium.org/2967223002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/2967223002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:251:
FileEntryPicker::FilesSelectedCallback&& files_selected_callback,
here, too, maybe pass by value.

[michaelpg, 2017-07-05 19:52:54]

Thanks, PTAL!

https://codereview.chromium.org/2967223002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_entry_picker.cc (right):

https://codereview.chromium.org/2967223002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_entry_picker.cc:47:
MultiFilesSelected(paths, params);
On 2017/07/05 19:08:04, Devlin wrote:
> nit: MultiFilesSelected(std::vector<base::FilePath>(path, 1));

Done w/ aggregate initialization, as per style guide

https://codereview.chromium.org/2967223002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_entry_picker.h (right):

https://codereview.chromium.org/2967223002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_entry_picker.h:29:
base::OnceCallback<void(const std::vector<base::FilePath>& paths)>;
On 2017/07/05 19:08:05, Devlin wrote:
> nit: \n

Done.

https://codereview.chromium.org/2967223002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_entry_picker.h:30:
FileEntryPicker(content::WebContents* web_contents,
On 2017/07/05 19:08:05, Devlin wrote:
> Document, either here or above the member variables, what these variables
> mean/are used for.

Done.

https://codereview.chromium.org/2967223002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_entry_picker.h:34:
FilesSelectedCallback&& files_selected_callback,
On 2017/07/05 19:08:05, Devlin wrote:
> nit: maybe just pass by value here, and std::move it?

Thanks for catching this. Yeah, what I had goes against the Google style guide
-- callers might forget the function takes an rvalue ref. Passing by value
requires callers to explicitly std::move.

https://codereview.chromium.org/2967223002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_entry_picker.h:36:
~FileEntryPicker() override;
On 2017/07/05 19:08:05, Devlin wrote:
> Can we make this private?

Done (was copied code).

https://codereview.chromium.org/2967223002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_entry_picker.h:43: 
On 2017/07/05 19:08:05, Devlin wrote:
> no \n between overridden methods from the same class.

Done (was copied code)

https://codereview.chromium.org/2967223002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/2967223002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:251:
FileEntryPicker::FilesSelectedCallback&& files_selected_callback,
On 2017/07/05 19:08:05, Devlin wrote:
> here, too, maybe pass by value.

Done.

[Devlin, 2017-07-05 20:32:16]

lgtm

https://codereview.chromium.org/2967223002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_entry_picker.cc (right):

https://codereview.chromium.org/2967223002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_entry_picker.cc:75: for
(std::vector<ui::SelectedFileInfo>::const_iterator it = files.begin();
optional: update this to a range-based loop.

https://codereview.chromium.org/2967223002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/2967223002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:265: return false;
If we bail out when we don't have a web contents here, can we DCHECK it in the
FileEntryPicker?  Or is FileEntryPicker used elsewhere?

[michaelpg, 2017-07-05 21:26:14]

https://codereview.chromium.org/2967223002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_entry_picker.cc (right):

https://codereview.chromium.org/2967223002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_entry_picker.cc:75: for
(std::vector<ui::SelectedFileInfo>::const_iterator it = files.begin();
On 2017/07/05 20:32:15, Devlin wrote:
> optional: update this to a range-based loop.

Done.

https://codereview.chromium.org/2967223002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/2967223002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:265: return false;
On 2017/07/05 20:32:15, Devlin wrote:
> If we bail out when we don't have a web contents here, can we DCHECK it in the
> FileEntryPicker?  Or is FileEntryPicker used elsewhere?

I'll make it CHECK -- this is in the extensions namespace and we don't want to
allow file pickers with no backing web_contents.

[michaelpg, 2017-07-05 21:26:21]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2017-07-05 21:26:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2967223002/#ps80001
(title: "CHECK web_contents")

[commit-bot: I haz the power, 2017-07-05 21:26:41]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-05 23:27:33]

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

[commit-bot: I haz the power, 2017-07-05 23:27:34]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_TIMED_OUT, build has not
started yet; builder either lacks capacity or does not exist (misspelled?))
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[michaelpg, 2017-07-06 18:11:58]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2017-07-06 18:12:23]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-06 18:16:22]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1499364718298310,
"parent_rev": "961bda534caf2a184153b1458aae16be169bc951", "commit_rev":
"b35720f5e102a25733b3dce0daa1fafda680a5bb"}

[commit-bot: I haz the power, 2017-07-06 18:16:41]

Description was changed from

==========
Move fileSystem API FilePicker class to own file

The chrome.fileSystem API impl has a FilePicker class used to create
and show a "Choose a file/directory" dialog. It's not tiny and it
manages its own lifetime, so I'm extracting it into a separate file.

This will unblock https://codereview.chromium.org/2934143002/ by
allowing us to easily override the file picking behavior in extension
tests without having to depend on Chrome-specific code.

BUG=729713
R=rdevlin.cronin@chromium.org
==========

to

==========
Move fileSystem API FilePicker class to own file

The chrome.fileSystem API impl has a FilePicker class used to create
and show a "Choose a file/directory" dialog. It's not tiny and it
manages its own lifetime, so I'm extracting it into a separate file.

This will unblock https://codereview.chromium.org/2934143002/ by
allowing us to easily override the file picking behavior in extension
tests without having to depend on Chrome-specific code.

BUG=729713
R=rdevlin.cronin@chromium.org

Review-Url: https://codereview.chromium.org/2967223002
Cr-Commit-Position: refs/heads/master@{#484673}
Committed:
https://chromium.googlesource.com/chromium/src/+/b35720f5e102a25733b3dce0daa1...
==========

[commit-bot: I haz the power, 2017-07-06 18:16:42]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/b35720f5e102a25733b3dce0daa1...