Pepper's FileSystem implementation.

Pepper's FileSystem implementation.

BUG=none
TEST=none


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

[dumi, 2010-09-23 06:01:14]

Depends on http://codereview.chromium.org/3417018. Pepper test to come.

[brettw, 2010-09-23 17:47:14]

http://codereview.chromium.org/3394017/diff/6001/7001
File chrome/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/3394017/diff/6001/7001#newcode650
chrome/renderer/pepper_plugin_delegate_impl.cc:650:
DCHECK(file_system_dispatcher);
I wouldn't bother with these DCHECKs. I usually say not to DCHECK pointers are
non-NULL since it will just crash and the NULL pointers are typically just as
easy to debug as the DCHECK. Of course in release, it will crash anyway so the
DCHECK doesn't help. The time when it's important to DCHECK is when you save the
pointer for later where the cause of the crash might be non-obvious.

http://codereview.chromium.org/3394017/diff/6001/7004
File webkit/glue/plugins/pepper_error_util.h (right):

http://codereview.chromium.org/3394017/diff/6001/7004#newcode14
webkit/glue/plugins/pepper_error_util.h:14: } // namespace pepper
Style nit: two spaces before end-of-line comments (same two lines below).

http://codereview.chromium.org/3394017/diff/6001/7005
File webkit/glue/plugins/pepper_file_system.cc (right):

http://codereview.chromium.org/3394017/diff/6001/7005#newcode28
webkit/glue/plugins/pepper_file_system.cc:28:
Resource::GetAs<FileRef>(directory_ref_id));
All of these GetAs calls may return NULL if the plugin passed an invalid value.
Be sure to check for this in each case and return failure if it's NULL.

http://codereview.chromium.org/3394017/diff/6001/7006
File webkit/glue/plugins/pepper_file_system.h (right):

http://codereview.chromium.org/3394017/diff/6001/7006#newcode65
webkit/glue/plugins/pepper_file_system.h:65: PluginDelegate* delegate_;
I don't think you use these variables (I assume you had non-static member
functions in a previous version of this patch).

I don't think you even need this class other than for GetInterface. You can
implement the entire thing in the C functions, which we normally do for
interfaces where there's no "object" associated with it (like pepper_char_set).

[dumi, 2010-09-23 19:12:03]

http://codereview.chromium.org/3394017/diff/6001/7001
File chrome/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/3394017/diff/6001/7001#newcode650
chrome/renderer/pepper_plugin_delegate_impl.cc:650:
DCHECK(file_system_dispatcher);
On 2010/09/23 17:47:15, brettw wrote:
> I wouldn't bother with these DCHECKs. I usually say not to DCHECK pointers are
> non-NULL since it will just crash and the NULL pointers are typically just as
> easy to debug as the DCHECK. Of course in release, it will crash anyway so the
> DCHECK doesn't help. The time when it's important to DCHECK is when you save
the
> pointer for later where the cause of the crash might be non-obvious.

removed.

http://codereview.chromium.org/3394017/diff/6001/7004
File webkit/glue/plugins/pepper_error_util.h (right):

http://codereview.chromium.org/3394017/diff/6001/7004#newcode14
webkit/glue/plugins/pepper_error_util.h:14: } // namespace pepper
On 2010/09/23 17:47:15, brettw wrote:
> Style nit: two spaces before end-of-line comments (same two lines below).

fixed.

http://codereview.chromium.org/3394017/diff/6001/7005
File webkit/glue/plugins/pepper_file_system.cc (right):

http://codereview.chromium.org/3394017/diff/6001/7005#newcode28
webkit/glue/plugins/pepper_file_system.cc:28:
Resource::GetAs<FileRef>(directory_ref_id));
On 2010/09/23 17:47:15, brettw wrote:
> All of these GetAs calls may return NULL if the plugin passed an invalid
value.
> Be sure to check for this in each case and return failure if it's NULL.

done.

http://codereview.chromium.org/3394017/diff/6001/7006
File webkit/glue/plugins/pepper_file_system.h (right):

http://codereview.chromium.org/3394017/diff/6001/7006#newcode65
webkit/glue/plugins/pepper_file_system.h:65: PluginDelegate* delegate_;
On 2010/09/23 17:47:15, brettw wrote:
> I don't think you use these variables (I assume you had non-static member
> functions in a previous version of this patch).

oops, removed.

> I don't think you even need this class other than for GetInterface. You can
> implement the entire thing in the C functions, which we normally do for
> interfaces where there's no "object" associated with it (like
pepper_char_set).

done.

[brettw, 2010-09-23 19:35:56]

LGTM

http://codereview.chromium.org/3394017/diff/17002/18005
File webkit/glue/plugins/pepper_file_system.cc (right):

http://codereview.chromium.org/3394017/diff/17002/18005#newcode60
webkit/glue/plugins/pepper_file_system.cc:60: std::swap(callback, callback_);
I don't think this swap is necessary in your case. I assume you copied this from
pepper_graphics_2d? That uses swap first because the presence or absence of the
callback indicates something and we want it to be empty when executing. In your
case, this object is standalone so I think you should just be able to call
RunCompletionCallback on callback_.

http://codereview.chromium.org/3394017/diff/17002/18005#newcode115
webkit/glue/plugins/pepper_file_system.cc:115: std::swap(callback, callback_);
Same swap comment as above.

http://codereview.chromium.org/3394017/diff/17002/18005#newcode229
webkit/glue/plugins/pepper_file_system.cc:229: if ((file_ref->file_system_type()
== PP_FILESYSTEMTYPE_EXTERNAL) ||
Remove extra blank line above here.

[dumi, 2010-09-23 22:31:40]

http://codereview.chromium.org/3394017/diff/17002/18005
File webkit/glue/plugins/pepper_file_system.cc (right):

http://codereview.chromium.org/3394017/diff/17002/18005#newcode60
webkit/glue/plugins/pepper_file_system.cc:60: std::swap(callback, callback_);
On 2010/09/23 19:35:56, brettw wrote:
> I don't think this swap is necessary in your case. I assume you copied this
from
> pepper_graphics_2d? That uses swap first because the presence or absence of
the
> callback indicates something and we want it to be empty when executing. In
your
> case, this object is standalone so I think you should just be able to call
> RunCompletionCallback on callback_.

done.

http://codereview.chromium.org/3394017/diff/17002/18005#newcode115
webkit/glue/plugins/pepper_file_system.cc:115: std::swap(callback, callback_);
On 2010/09/23 19:35:56, brettw wrote:
> Same swap comment as above.

done.

http://codereview.chromium.org/3394017/diff/17002/18005#newcode229
webkit/glue/plugins/pepper_file_system.cc:229: if ((file_ref->file_system_type()
== PP_FILESYSTEMTYPE_EXTERNAL) ||
On 2010/09/23 19:35:56, brettw wrote:
> Remove extra blank line above here.

removed.

[darin (slow to review), 2010-09-24 00:09:36]

lgtm