Add selectFileForSave method to chrome.fileBrowserHandler (issue 10387124)

[tba...@chromium.org]

Reviewers: Aaron Boodman,

Message:
Hey Aaron,
Can you take a look at this?

Thanks,
Toni

Description:
Add selectFileForSave method to chrome.fileBrowserHandler

The method shows save as dialog asking user to select a file
path that the extension can overwrite. When the file path is
selected, method creates or truncates the file (depending on
if the file previously existed).

see:
https://docs.google.com/a/google.com/document/d/1d0yvN-6E3cuchyurvnpV21hmZwhb4_MWLL5aDUDp130/edit

BUG= 126977
TEST=browser_tests:*FileHandler*

Please review this at https://chromiumcodereview.appspot.com/10387124/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
A chrome/browser/chromeos/extensions/file_browser_handler_api.h
A chrome/browser/chromeos/extensions/file_browser_handler_api.cc
A chrome/browser/chromeos/extensions/file_browser_handler_api_test.cc
M chrome/browser/extensions/extension_function_registry.cc
M chrome/chrome_browser.gypi
M chrome/chrome_tests.gypi
M chrome/common/extensions/api/file_browser_handler.json
A
chrome/test/data/extensions/api_test/file_browser/filehandler_create/background.js
A
chrome/test/data/extensions/api_test/file_browser/filehandler_create/manifest.json
A
chrome/test/data/extensions/api_test/file_browser/filehandler_create/test.html
A
chrome/test/data/extensions/api_test/file_browser/filehandler_create/test.js

[a...@chromium.org]

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc
File chrome/browser/chromeos/extensions/file_browser_handler_api.cc
(right):

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode66
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:66: //
Extension function that owns the selector.
Insert blank line.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode67
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:67:
scoped_refptr<SelectFileForSaveFunction> function_;
There is circular ownership between FileSelectorImpl and
SelectFileForSaveFunction.

Why not make SelectFileForSaveFunction::file_selector_ a weak pointer?

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode71
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:71: const
scoped_refptr<SelectFileForSaveFunction>& function)
You can just pass SelectFileForSaveFunction*. It immediately goes into a
refptr anyway.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode95
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:95:
dialog_->SelectFile(SelectFileDialog::SELECT_SAVEAS_FILE,
Weird parameter wrapping. Put as many params on one line as will fit.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode98
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:98: NULL,
0,
It is nice in these cases to add an inline comment explaining what the
omitted param is.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode142
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:142:
base::DictionaryValue* select_params;
You should use the JSON Schema Compiler rather than parsing these Value
objects manually. See c/b/e/api/permissions/ for an example of how to do
that.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode149
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:149: if
(!select_params->GetString("suggestedName", &suggested_name_str)) {
The validation should guarantee that this is true, so there's no need to
provide an error. You can just do:

EXTENSION_FUNCTION_VALIDATE(select_params->GetString(..));

If you want to ensure that the name is non-empty, you should also
specify that in the JSON Schema, using the minLength property.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode157
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:157:
error_ = "SelectFileForSave must be run with user gesture.";
It would be nice to clarify this slightly since it will be shown to
developers:

This method can only be called in response to a user gesture, such a
mouse click or key press.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode162
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:162: if
(!file_selector_.get()) {
If an if statement is less than two lines total, the curly braces are
not required.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode167
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:167:
GetCurrentBrowser())) {
Under what conditions might this return false? Usually extension errors
are reserved for things the developer did wrong. In this case, it seems
outside of their control, so we should just log at most ... or perhaps
just ignore this case.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode175
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:175: void
SelectFileForSaveFunction::OnFilePathSelected(bool success,
All param lines should start at the same column.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode191
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:191: if
(!GetVirtualPathAndFileURL(full_path)) {
Again, do we expect this to actually fail with files the user chose? If
not, let's DCHECK that and ignore this case otherwise.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode197
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:197:
const bool posted = content::BrowserThread::PostTask(
No need to check the result of PostTask().

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode236
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:236: bool
created;
Always initialized primitives.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.h
File chrome/browser/chromeos/extensions/file_browser_handler_api.h
(right):

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.h#newcode45
chrome/browser/chromeos/extensions/file_browser_handler_api.h:45: //
Used in testing.
Blank line before this comment.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.h#newcode57
chrome/browser/chromeos/extensions/file_browser_handler_api.h:57: //
Calls |DoCreateFile| on file thread and invokes the callback.
Add a blank line separating each of these definitions.

http://codereview.chromium.org/10387124/diff/12001/chrome/common/extensions/api/file_browser_handler.json
File chrome/common/extensions/api/file_browser_handler.json (right):

http://codereview.chromium.org/10387124/diff/12001/chrome/common/extensions/api/file_browser_handler.json#newcode50
chrome/common/extensions/api/file_browser_handler.json:50: "name":
"selectFileForSave",
It seems like we might want to add selectFileForRead(), so perhaps it
makes more sense to name this selectFile(). In the future if we want to
be able to distinguish which access modes are needed, they can be added
to newFileParams.

http://codereview.chromium.org/10387124/diff/12001/chrome/common/extensions/api/file_browser_handler.json#newcode61
chrome/common/extensions/api/file_browser_handler.json:61:
"description": "New file name suggested by the extension."
Avoid the use of the words 'extension' or 'app' in these APIs because
they can frequently be used by either.

http://codereview.chromium.org/10387124/

[tba...@chromium.org]

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc
File chrome/browser/chromeos/extensions/file_browser_handler_api.cc
(right):

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode66
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:66: //
Extension function that owns the selector.

On 2012/05/17 21:28:24, Aaron Boodman wrote:
> Insert blank line.

Done.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode67
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:67:
scoped_refptr<SelectFileForSaveFunction> function_;

On 2012/05/17 21:28:24, Aaron Boodman wrote:
> There is circular ownership between FileSelectorImpl and
> SelectFileForSaveFunction.

> Why not make SelectFileForSaveFunction::file_selector_ a weak pointer?

Done.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode71
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:71: const
scoped_refptr<SelectFileForSaveFunction>& function)

On 2012/05/17 21:28:24, Aaron Boodman wrote:
> You can just pass SelectFileForSaveFunction*. It immediately goes into
a refptr
> anyway.

Done.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode95
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:95:
dialog_->SelectFile(SelectFileDialog::SELECT_SAVEAS_FILE,

On 2012/05/17 21:28:24, Aaron Boodman wrote:
> Weird parameter wrapping. Put as many params on one line as will fit.

Done.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode98
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:98: NULL,
0,

On 2012/05/17 21:28:24, Aaron Boodman wrote:
> It is nice in these cases to add an inline comment explaining what the
omitted
> param is.

Done.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode142
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:142:
base::DictionaryValue* select_params;

On 2012/05/17 21:28:24, Aaron Boodman wrote:
> You should use the JSON Schema Compiler rather than parsing these
Value objects
> manually. See c/b/e/api/permissions/ for an example of how to do that.

wow, this is handy..

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode149
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:149: if
(!select_params->GetString("suggestedName", &suggested_name_str)) {

On 2012/05/17 21:28:24, Aaron Boodman wrote:
> The validation should guarantee that this is true, so there's no need
to provide
> an error. You can just do:

> EXTENSION_FUNCTION_VALIDATE(select_params->GetString(..));

> If you want to ensure that the name is non-empty, you should also
specify that
> in the JSON Schema, using the minLength property.

Done.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode157
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:157:
error_ = "SelectFileForSave must be run with user gesture.";

On 2012/05/17 21:28:24, Aaron Boodman wrote:
> It would be nice to clarify this slightly since it will be shown to
developers:

> This method can only be called in response to a user gesture, such a
mouse click
> or key press.

Done.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode162
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:162: if
(!file_selector_.get()) {

On 2012/05/17 21:28:24, Aaron Boodman wrote:
> If an if statement is less than two lines total, the curly braces are
not
> required.

Yeah, I had put some debugging log here and forgot to remove braces
later..

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode167
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:167:
GetCurrentBrowser())) {

On 2012/05/17 21:28:24, Aaron Boodman wrote:
> Under what conditions might this return false? Usually extension
errors are
> reserved for things the developer did wrong. In this case, it seems
outside of
> their control, so we should just log at most ... or perhaps just
ignore this
> case.

It may fail if another dialog is already shown in the tab. But yeah, it
makes more sense not to report error (we will just return empty url).

I guess same goes for checking that the file is not on gdata (this layer
should probably not be aware of that). I'll remove that error too.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode175
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:175: void
SelectFileForSaveFunction::OnFilePathSelected(bool success,

On 2012/05/17 21:28:24, Aaron Boodman wrote:
> All param lines should start at the same column.

Done.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode191
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:191: if
(!GetVirtualPathAndFileURL(full_path)) {

On 2012/05/17 21:28:24, Aaron Boodman wrote:
> Again, do we expect this to actually fail with files the user chose?
If not,
> let's DCHECK that and ignore this case otherwise.

Done.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode197
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:197:
const bool posted = content::BrowserThread::PostTask(

On 2012/05/17 21:28:24, Aaron Boodman wrote:
> No need to check the result of PostTask().

Done.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.cc#newcode236
chrome/browser/chromeos/extensions/file_browser_handler_api.cc:236: bool
created;

On 2012/05/17 21:28:24, Aaron Boodman wrote:
> Always initialized primitives.

Done.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.h
File chrome/browser/chromeos/extensions/file_browser_handler_api.h
(right):

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.h#newcode45
chrome/browser/chromeos/extensions/file_browser_handler_api.h:45: //
Used in testing.

On 2012/05/17 21:28:24, Aaron Boodman wrote:
> Blank line before this comment.

Done.

http://codereview.chromium.org/10387124/diff/12001/chrome/browser/chromeos/extensions/file_browser_handler_api.h#newcode57
chrome/browser/chromeos/extensions/file_browser_handler_api.h:57: //
Calls |DoCreateFile| on file thread and invokes the callback.

On 2012/05/17 21:28:24, Aaron Boodman wrote:
> Add a blank line separating each of these definitions.

Done.

http://codereview.chromium.org/10387124/diff/12001/chrome/common/extensions/api/file_browser_handler.json
File chrome/common/extensions/api/file_browser_handler.json (right):

http://codereview.chromium.org/10387124/diff/12001/chrome/common/extensions/api/file_browser_handler.json#newcode50
chrome/common/extensions/api/file_browser_handler.json:50: "name":
"selectFileForSave",

On 2012/05/17 21:28:24, Aaron Boodman wrote:
> It seems like we might want to add selectFileForRead(), so perhaps it
makes more
> sense to name this selectFile(). In the future if we want to be able
to
> distinguish which access modes are needed, they can be added to
newFileParams.

Yeah, that thought crossed my mind this morning, but I forgot to update
the cl :)

http://codereview.chromium.org/10387124/diff/12001/chrome/common/extensions/api/file_browser_handler.json#newcode61
chrome/common/extensions/api/file_browser_handler.json:61:
"description": "New file name suggested by the extension."

On 2012/05/17 21:28:24, Aaron Boodman wrote:
> Avoid the use of the words 'extension' or 'app' in these APIs because
they can
> frequently be used by either.

Done.

http://codereview.chromium.org/10387124/