Chromium-side implemention of pepper's FileChooser.

Chromium-side implemention of pepper's FileChooser.

BUG=none
TEST=none



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=55148

[darin (slow to review), 2010-08-03 16:18:06]

http://codereview.chromium.org/3052040/diff/7001/8004
File webkit/glue/plugins/pepper_file_chooser.cc (right):

http://codereview.chromium.org/3052040/diff/7001/8004#newcode71
webkit/glue/plugins/pepper_file_chooser.cc:71: class
PepperWebFileChooserCompletionImpl :
nit: since this is in the anonymous namespace, you could get away with using a
shorter name for this class like FileChooserCompletionImpl.

http://codereview.chromium.org/3052040/diff/7001/8004#newcode72
webkit/glue/plugins/pepper_file_chooser.cc:72: public
WebKit::WebFileChooserCompletion {
please add using WebKit::WebFileChooserCompletion to the top of this file so
that you can avoid the WebKit:: prefix in the code here.  please do the same for
all of the other WebKit API types used in this file.  (this is just for
consistency with the way we use the WebKit API in chromium code.)

http://codereview.chromium.org/3052040/diff/7001/8004#newcode85
webkit/glue/plugins/pepper_file_chooser.cc:85:
files.push_back(UTF16ToASCII(file_names[i]));
i think you should use webkit_glue::WebStringToFilePath here, and change
SaveChosenFiles to take a vector of FilePath.

http://codereview.chromium.org/3052040/diff/7001/8004#newcode122
webkit/glue/plugins/pepper_file_chooser.cc:122: completion_callback_.reset();
after running the callback, you should be prepared for |this| to potentially
become invalid.  see URLLoader::RunCallback.  you should probably just copy that
code.

http://codereview.chromium.org/3052040/diff/7001/8004#newcode131
webkit/glue/plugins/pepper_file_chooser.cc:131: WebKit::WebFileChooserParams
params;
i think you also need to initialize the "directory" member.

http://codereview.chromium.org/3052040/diff/7001/8004#newcode133
webkit/glue/plugins/pepper_file_chooser.cc:133: params.acceptTypes =
ASCIIToUTF16(accept_mime_types_);
nit: it is slightly better to do:

  params.acceptTypes = WebString::fromUTF8(accept_mime_types_);

that avoids the intermediate string16 constructed by ASCIIToUTF16.

http://codereview.chromium.org/3052040/diff/7001/8005
File webkit/glue/plugins/pepper_file_chooser.h (right):

http://codereview.chromium.org/3052040/diff/7001/8005#newcode37
webkit/glue/plugins/pepper_file_chooser.h:37: void SaveChosenFiles(const
std::vector<std::string>& files);
nit: maybe Store would be a better prefix here?  when i first read it, i thought
it had something to do with a "save as" file chooser ;-)

http://codereview.chromium.org/3052040/diff/7001/8005#newcode47
webkit/glue/plugins/pepper_file_chooser.h:47: scoped_ptr<pp::CompletionCallback>
completion_callback_;
you can probably get away with not heap allocating this callback.  just null
check completion_callback_.func to determine if the callback is set or not.

also, please avoid using classes from ppapi/cpp in the implementation of pepper.
 that code is only meant to be linked into a plugin.

see for example how the PP_CompletionCallback is managed in
pepper_url_loader.{h,cc}

http://codereview.chromium.org/3052040/diff/7001/8006
File webkit/glue/plugins/pepper_plugin_delegate.h (right):

http://codereview.chromium.org/3052040/diff/7001/8006#newcode101
webkit/glue/plugins/pepper_plugin_delegate.h:101: FILE_CHOOSER_MODE_OPEN,
this is unnecessary right?

[dumi, 2010-08-03 20:57:41]

http://codereview.chromium.org/3052040/diff/7001/8004
File webkit/glue/plugins/pepper_file_chooser.cc (right):

http://codereview.chromium.org/3052040/diff/7001/8004#newcode71
webkit/glue/plugins/pepper_file_chooser.cc:71: class
PepperWebFileChooserCompletionImpl :
On 2010/08/03 16:18:06, darin wrote:
> nit: since this is in the anonymous namespace, you could get away with using a
> shorter name for this class like FileChooserCompletionImpl.

done.

http://codereview.chromium.org/3052040/diff/7001/8004#newcode72
webkit/glue/plugins/pepper_file_chooser.cc:72: public
WebKit::WebFileChooserCompletion {
On 2010/08/03 16:18:06, darin wrote:
> please add using WebKit::WebFileChooserCompletion to the top of this file so
> that you can avoid the WebKit:: prefix in the code here.  please do the same
for
> all of the other WebKit API types used in this file.  (this is just for
> consistency with the way we use the WebKit API in chromium code.)

done.

http://codereview.chromium.org/3052040/diff/7001/8004#newcode85
webkit/glue/plugins/pepper_file_chooser.cc:85:
files.push_back(UTF16ToASCII(file_names[i]));
On 2010/08/03 16:18:06, darin wrote:
> i think you should use webkit_glue::WebStringToFilePath here, and change
> SaveChosenFiles to take a vector of FilePath.

done. i knew there was a SomeStringToFilePath() function, but couldn't find it.

http://codereview.chromium.org/3052040/diff/7001/8004#newcode122
webkit/glue/plugins/pepper_file_chooser.cc:122: completion_callback_.reset();
On 2010/08/03 16:18:06, darin wrote:
> after running the callback, you should be prepared for |this| to potentially
> become invalid.  see URLLoader::RunCallback.  you should probably just copy
that
> code.

done.

http://codereview.chromium.org/3052040/diff/7001/8004#newcode131
webkit/glue/plugins/pepper_file_chooser.cc:131: WebKit::WebFileChooserParams
params;
On 2010/08/03 16:18:06, darin wrote:
> i think you also need to initialize the "directory" member.

done.

http://codereview.chromium.org/3052040/diff/7001/8004#newcode133
webkit/glue/plugins/pepper_file_chooser.cc:133: params.acceptTypes =
ASCIIToUTF16(accept_mime_types_);
On 2010/08/03 16:18:06, darin wrote:
> nit: it is slightly better to do:
> 
>   params.acceptTypes = WebString::fromUTF8(accept_mime_types_);
> 
> that avoids the intermediate string16 constructed by ASCIIToUTF16.

done.

http://codereview.chromium.org/3052040/diff/7001/8005
File webkit/glue/plugins/pepper_file_chooser.h (right):

http://codereview.chromium.org/3052040/diff/7001/8005#newcode37
webkit/glue/plugins/pepper_file_chooser.h:37: void SaveChosenFiles(const
std::vector<std::string>& files);
On 2010/08/03 16:18:06, darin wrote:
> nit: maybe Store would be a better prefix here?  when i first read it, i
thought
> it had something to do with a "save as" file chooser ;-)

done.

http://codereview.chromium.org/3052040/diff/7001/8005#newcode47
webkit/glue/plugins/pepper_file_chooser.h:47: scoped_ptr<pp::CompletionCallback>
completion_callback_;
On 2010/08/03 16:18:06, darin wrote:
> you can probably get away with not heap allocating this callback.  just null
> check completion_callback_.func to determine if the callback is set or not.
> 
> also, please avoid using classes from ppapi/cpp in the implementation of
pepper.
>  that code is only meant to be linked into a plugin.
> 
> see for example how the PP_CompletionCallback is managed in
> pepper_url_loader.{h,cc}

done.

http://codereview.chromium.org/3052040/diff/7001/8006
File webkit/glue/plugins/pepper_plugin_delegate.h (right):

http://codereview.chromium.org/3052040/diff/7001/8006#newcode101
webkit/glue/plugins/pepper_plugin_delegate.h:101: FILE_CHOOSER_MODE_OPEN,
On 2010/08/03 16:18:06, darin wrote:
> this is unnecessary right?

yep. forgot to remove.

[brettw, 2010-08-03 21:09:40]

http://codereview.chromium.org/3052040/diff/15001/5005
File webkit/glue/plugins/pepper_file_chooser.cc (right):

http://codereview.chromium.org/3052040/diff/15001/5005#newcode21
webkit/glue/plugins/pepper_file_chooser.cc:21: #include <string>
These should go below this .cc file's corresponding .h but above everything
else.

http://codereview.chromium.org/3052040/diff/15001/5005#newcode77
webkit/glue/plugins/pepper_file_chooser.cc:77: public:
Style nit: indent public/private one space.

http://codereview.chromium.org/3052040/diff/15001/5005#newcode83
webkit/glue/plugins/pepper_file_chooser.cc:83: virtual
~FileChooserCompletionImpl() { }
Style nit: no space in {}

http://codereview.chromium.org/3052040/diff/15001/5005#newcode120
webkit/glue/plugins/pepper_file_chooser.cc:120: for
(std::vector<FilePath>::const_reverse_iterator it = files.rbegin();
This isn't technically safe since the vector may reallocate itself when you
erase stuff from it (I looked it up). If this happens, your allocators will
point to invalid data.

I'd use an integer index (not size_t since that's unsigned) for this iteration
to avoid the problem.

[dumi, 2010-08-03 22:25:55]

http://codereview.chromium.org/3052040/diff/15001/5005
File webkit/glue/plugins/pepper_file_chooser.cc (right):

http://codereview.chromium.org/3052040/diff/15001/5005#newcode21
webkit/glue/plugins/pepper_file_chooser.cc:21: #include <string>
On 2010/08/03 21:09:41, brettw wrote:
> These should go below this .cc file's corresponding .h but above everything
> else.

fixed.

http://codereview.chromium.org/3052040/diff/15001/5005#newcode77
webkit/glue/plugins/pepper_file_chooser.cc:77: public:
On 2010/08/03 21:09:41, brettw wrote:
> Style nit: indent public/private one space.

done.

http://codereview.chromium.org/3052040/diff/15001/5005#newcode83
webkit/glue/plugins/pepper_file_chooser.cc:83: virtual
~FileChooserCompletionImpl() { }
On 2010/08/03 21:09:41, brettw wrote:
> Style nit: no space in {}

done.

http://codereview.chromium.org/3052040/diff/15001/5005#newcode120
webkit/glue/plugins/pepper_file_chooser.cc:120: for
(std::vector<FilePath>::const_reverse_iterator it = files.rbegin();
On 2010/08/03 21:09:41, brettw wrote:
> This isn't technically safe since the vector may reallocate itself when you
> erase stuff from it (I looked it up). If this happens, your allocators will
> point to invalid data.
> 
> I'd use an integer index (not size_t since that's unsigned) for this iteration
> to avoid the problem.

done. i think we can still use a size_t index var.

[darin (slow to review), 2010-08-04 08:12:55]

LGTM

[dumi, 2010-08-05 00:56:26]

same CL, with a couple of minor changes:

1. made a small necessary change to pepper_plugin_module.cc
2. in pepper_file_chooser, passing vector<string> around instead of
vector<FilePath>, because we need to use the FileRef constructor that takes a
std::string for the path. otherwise, the file system type is assumed to be
PP_FILESYSTEMTYPE_EXTERNAL, and FileRef::GetPath() returns a null Var.

[brettw, 2010-08-05 21:52:11]

LGTM with no DEPS change

http://codereview.chromium.org/3052040/diff/18008/5008
File DEPS (right):

http://codereview.chromium.org/3052040/diff/18008/5008#newcode4
DEPS:4: "webkit_revision": "64638",
Did you mean to pull webkit in this patch?

[dumi, 2010-08-05 22:03:13]

http://codereview.chromium.org/3052040/diff/18008/5008
File DEPS (right):

http://codereview.chromium.org/3052040/diff/18008/5008#newcode4
DEPS:4: "webkit_revision": "64638",
On 2010/08/05 21:52:11, brettw wrote:
> Did you mean to pull webkit in this patch?

no. i don't see a diff here in rietveld -- it's the version that was in the
trunk when i last sync'ed my chromium client.