Add IPC plumbing code for FileSystem API's openFileSystem

Add 1st cut of IPC plumbing code for FileSystem API's openFileSystem

The dispatcher host code is just a stub.

BUG=32277
TESTS=none; will be added later

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

[Eric U., 2010-08-19 18:46:45]

I'm no expert in this area, but it looks generally good to me.

http://codereview.chromium.org/3107026/diff/6001/7006
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/3107026/diff/6001/7006#newcode879
chrome/common/render_messages.h:879: 
Please add comments for type and size.

http://codereview.chromium.org/3107026/diff/6001/7008
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/3107026/diff/6001/7008#newcode3369
chrome/renderer/render_view.cc:3369: new PendingOpenFileSystem(callback));
s/callback/callbacks/ here and below?

[michaeln, 2010-08-19 18:54:10]

LGTM modulo nits

http://codereview.chromium.org/3107026/diff/6001/7001
File chrome/browser/file_system/file_system_dispatcher_host.cc (right):

http://codereview.chromium.org/3107026/diff/6001/7001#newcode54
chrome/browser/file_system/file_system_dispatcher_host.cc:54: {
move bracket up a line

http://codereview.chromium.org/3107026/diff/6001/7007
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/3107026/diff/6001/7007#newcode1013
chrome/common/render_messages_internal.h:1013: int32 /* error_code */)
generally we prefer plain 'int' when we have the choice, can the int32s here be
plain ints?

http://codereview.chromium.org/3107026/diff/6001/7009
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/3107026/diff/6001/7009#newcode554
chrome/renderer/render_view.h:554: WebKit::WebFileSystemCallbacks*);
we put names to arg values in chromium style

http://codereview.chromium.org/3107026/diff/6001/7009#newcode557
chrome/renderer/render_view.h:557: virtual void OnFileSystemOpened(int32
operation_id,
int vs int32?

http://codereview.chromium.org/3107026/diff/6001/7009#newcode1302
chrome/renderer/render_view.h:1302: 
extra blank line here

[michaeln, 2010-08-19 19:17:25]

couple other minor comments... the response message handler rename we talked
about sounds good... OnOpenFileSystemComplete.

http://codereview.chromium.org/3107026/diff/6001/7008
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/3107026/diff/6001/7008#newcode3385
chrome/renderer/render_view.cc:3385: void RenderView::OnFileSystemOpened(
Also please relocate this method to line up with the order listed in the .h
file. The two related methods will end up being in different locations... but
that's how its done in this file.

http://codereview.chromium.org/3107026/diff/6001/7009
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/3107026/diff/6001/7009#newcode557
chrome/renderer/render_view.h:557: virtual void OnFileSystemOpened(int32
operation_id,
Does this need to be virtual (don't think it does) and can you relocate it IPC
message handlers section down below?

[kinuko, 2010-08-19 19:40:59]

Thanks!  I updated the patch.

http://codereview.chromium.org/3107026/diff/6001/7001
File chrome/browser/file_system/file_system_dispatcher_host.cc (right):

http://codereview.chromium.org/3107026/diff/6001/7001#newcode54
chrome/browser/file_system/file_system_dispatcher_host.cc:54: {
On 2010/08/19 18:54:11, michaeln wrote:
> move bracket up a line

Done.

http://codereview.chromium.org/3107026/diff/6001/7006
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/3107026/diff/6001/7006#newcode879
chrome/common/render_messages.h:879: 
On 2010/08/19 18:46:45, Eric U. wrote:
> Please add comments for type and size.

Done.

http://codereview.chromium.org/3107026/diff/6001/7007
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/3107026/diff/6001/7007#newcode1013
chrome/common/render_messages_internal.h:1013: int32 /* error_code */)
On 2010/08/19 18:54:11, michaeln wrote:
> generally we prefer plain 'int' when we have the choice, can the int32s here
be
> plain ints?

Done.

Also removed not-yet-used response messages (Success and Error).

http://codereview.chromium.org/3107026/diff/6001/7008
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/3107026/diff/6001/7008#newcode3369
chrome/renderer/render_view.cc:3369: new PendingOpenFileSystem(callback));
On 2010/08/19 18:46:45, Eric U. wrote:
> s/callback/callbacks/ here and below?

Done.

http://codereview.chromium.org/3107026/diff/6001/7008#newcode3385
chrome/renderer/render_view.cc:3385: void RenderView::OnFileSystemOpened(
On 2010/08/19 19:17:25, michaeln wrote:
> Also please relocate this method to line up with the order listed in the .h
> file. The two related methods will end up being in different locations... but
> that's how its done in this file.

Done.

The private handlers methods in .cc do not seem to be sorted, so I put mine at
the end of the file.  Let me know if this doesn't sound good.

http://codereview.chromium.org/3107026/diff/6001/7009
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/3107026/diff/6001/7009#newcode554
chrome/renderer/render_view.h:554: WebKit::WebFileSystemCallbacks*);
On 2010/08/19 18:54:11, michaeln wrote:
> we put names to arg values in chromium style

Done.

http://codereview.chromium.org/3107026/diff/6001/7009#newcode557
chrome/renderer/render_view.h:557: virtual void OnFileSystemOpened(int32
operation_id,
On 2010/08/19 18:54:11, michaeln wrote:
> int vs int32?

Done.

http://codereview.chromium.org/3107026/diff/6001/7009#newcode557
chrome/renderer/render_view.h:557: virtual void OnFileSystemOpened(int32
operation_id,
On 2010/08/19 19:17:25, michaeln wrote:
> Does this need to be virtual (don't think it does) and can you relocate it IPC
> message handlers section down below?

Done.

http://codereview.chromium.org/3107026/diff/6001/7009#newcode1302
chrome/renderer/render_view.h:1302: 
On 2010/08/19 18:54:11, michaeln wrote:
> extra blank line here

Done.

[Eric U., 2010-08-19 19:54:33]

http://codereview.chromium.org/3107026/diff/27001/28006
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/3107026/diff/27001/28006#newcode880
chrome/common/render_messages.h:880: // The requesting FileSystem type. This
must be one of the enum values
s/requesting/requested/

http://codereview.chromium.org/3107026/diff/27001/28006#newcode884
chrome/common/render_messages.h:884: // The requesting
?

[kinuko, 2010-08-19 21:55:49]

http://codereview.chromium.org/3107026/diff/27001/28006
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/3107026/diff/27001/28006#newcode880
chrome/common/render_messages.h:880: // The requesting FileSystem type. This
must be one of the enum values
On 2010/08/19 19:54:33, Eric U. wrote:
> s/requesting/requested/

Done.

http://codereview.chromium.org/3107026/diff/27001/28006#newcode884
chrome/common/render_messages.h:884: // The requesting
On 2010/08/19 19:54:33, Eric U. wrote:
> ?

Oops... completed the sentence.

[Eric U., 2010-08-19 22:01:55]

Great, thanks.  LGTM.

On Thu, Aug 19, 2010 at 2:55 PM,  <kinuko@chromium.org> wrote:
>
> http://codereview.chromium.org/3107026/diff/27001/28006
> File chrome/common/render_messages.h (right):
>
> http://codereview.chromium.org/3107026/diff/27001/28006#newcode880
> chrome/common/render_messages.h:880: // The requesting FileSystem type.
> This must be one of the enum values
> On 2010/08/19 19:54:33, Eric U. wrote:
>>
>> s/requesting/requested/
>
> Done.
>
> http://codereview.chromium.org/3107026/diff/27001/28006#newcode884
> chrome/common/render_messages.h:884: // The requesting
> On 2010/08/19 19:54:33, Eric U. wrote:
>>
>> ?
>
> Oops... completed the sentence.
>
> http://codereview.chromium.org/3107026/show
>

[darin (slow to review), 2010-08-20 04:52:11]

Looks good... just some nits:

http://codereview.chromium.org/3107026/diff/33001/34006
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/3107026/diff/33001/34006#newcode872
chrome/common/render_messages.h:872: int routing_id_;
nit: public member variables should not end with a "_"

http://codereview.chromium.org/3107026/diff/33001/34006#newcode882
chrome/common/render_messages.h:882: int type_;
nit: please just use the WebKit enum name here.  add ParamTraits for that enum
to webkit_param_traits.h.  this way we avoid error prone static_casts.

http://codereview.chromium.org/3107026/diff/33001/34006#newcode885
chrome/common/render_messages.h:885: int64 size_;
nit: how about giving this parameter a name like requested_size, requested_space
or requested_quota?

http://codereview.chromium.org/3107026/diff/33001/34007
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/3107026/diff/33001/34007#newcode2633
chrome/common/render_messages_internal.h:2633: // FileSystem API messages
nit: fix indentation

http://codereview.chromium.org/3107026/diff/33001/34008
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/3107026/diff/33001/34008#newcode3354
chrome/renderer/render_view.cc:3354: struct RenderView::PendingOpenFileSystem {
nit: please move this struct definition to the top of the file before the start
of the RenderView method definitions.

it is good to avoid interrupting the flow of method definitions with something
else as it can hurt readability (especially when the something else grows in
size, which has a tendency to happen).

http://codereview.chromium.org/3107026/diff/33001/34008#newcode3369
chrome/renderer/render_view.cc:3369: scoped_ptr<PendingOpenFileSystem> request(
nit: since you use the variable name "request", perhaps the struct should be
named OpenFileSystemRequest?

http://codereview.chromium.org/3107026/diff/33001/34008#newcode5554
chrome/renderer/render_view.cc:5554: if (request) {
DCHECK if operation_id is not found in the map?

http://codereview.chromium.org/3107026/diff/33001/34008#newcode5554
chrome/renderer/render_view.cc:5554: if (request) {
nit: perhaps it would be better to use the term "request_id" instead of
"operation_id" since "request" is a bit more specific than "operation".

http://codereview.chromium.org/3107026/diff/33001/34009
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/3107026/diff/33001/34009#newcode122
chrome/renderer/render_view.h:122: struct WebFileSystemCallbacks;
doesn't WebFileSystem.h have this forward declare already?

[kinuko, 2010-08-20 18:39:26]

Thanks, all fixed.

http://codereview.chromium.org/3107026/diff/33001/34006
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/3107026/diff/33001/34006#newcode872
chrome/common/render_messages.h:872: int routing_id_;
On 2010/08/20 04:52:11, darin wrote:
> nit: public member variables should not end with a "_"

Done.

http://codereview.chromium.org/3107026/diff/33001/34006#newcode882
chrome/common/render_messages.h:882: int type_;
On 2010/08/20 04:52:11, darin wrote:
> nit: please just use the WebKit enum name here.  add ParamTraits for that enum
> to webkit_param_traits.h.  this way we avoid error prone static_casts.

Done.

http://codereview.chromium.org/3107026/diff/33001/34006#newcode885
chrome/common/render_messages.h:885: int64 size_;
On 2010/08/20 04:52:11, darin wrote:
> nit: how about giving this parameter a name like requested_size,
requested_space
> or requested_quota?

Done.

http://codereview.chromium.org/3107026/diff/33001/34007
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/3107026/diff/33001/34007#newcode2633
chrome/common/render_messages_internal.h:2633: // FileSystem API messages
On 2010/08/20 04:52:11, darin wrote:
> nit: fix indentation

Done.

http://codereview.chromium.org/3107026/diff/33001/34008
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/3107026/diff/33001/34008#newcode3354
chrome/renderer/render_view.cc:3354: struct RenderView::PendingOpenFileSystem {
On 2010/08/20 04:52:11, darin wrote:
> nit: please move this struct definition to the top of the file before the
start
> of the RenderView method definitions.
> 
> it is good to avoid interrupting the flow of method definitions with something
> else as it can hurt readability (especially when the something else grows in
> size, which has a tendency to happen).

Done.

http://codereview.chromium.org/3107026/diff/33001/34008#newcode3369
chrome/renderer/render_view.cc:3369: scoped_ptr<PendingOpenFileSystem> request(
On 2010/08/20 04:52:11, darin wrote:
> nit: since you use the variable name "request", perhaps the struct should be
> named OpenFileSystemRequest?

Done.

http://codereview.chromium.org/3107026/diff/33001/34008#newcode5554
chrome/renderer/render_view.cc:5554: if (request) {
On 2010/08/20 04:52:11, darin wrote:
> DCHECK if operation_id is not found in the map?

Done.

http://codereview.chromium.org/3107026/diff/33001/34008#newcode5554
chrome/renderer/render_view.cc:5554: if (request) {
On 2010/08/20 04:52:11, darin wrote:
> nit: perhaps it would be better to use the term "request_id" instead of
> "operation_id" since "request" is a bit more specific than "operation".

Done.

http://codereview.chromium.org/3107026/diff/33001/34009
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/3107026/diff/33001/34009#newcode122
chrome/renderer/render_view.h:122: struct WebFileSystemCallbacks;
On 2010/08/20 04:52:11, darin wrote:
> doesn't WebFileSystem.h have this forward declare already?

Removed.

[darin (slow to review), 2010-08-20 23:26:12]

LGTM w/ nits:

http://codereview.chromium.org/3107026/diff/49001/46018
File chrome/common/webkit_param_traits.h (right):

http://codereview.chromium.org/3107026/diff/49001/46018#newcode332
chrome/common/webkit_param_traits.h:332: struct
ParamTraits<WebKit::WebFileSystem::Type> {
There's a new way to write this that is not yet widely used:

template <>
struct SimilarTypeTraits<WebKit::WebFileSystem::Type> {
  typedef int Type;
};

This is a lot simpler.  You don't get the nice logging, but that's not a big
deal.  (We should really use SimilarTypeTraits elsewhere!)

[kinuko, 2010-08-21 00:06:24]

On 2010/08/20 23:26:12, darin wrote:
> LGTM w/ nits:
> 
> http://codereview.chromium.org/3107026/diff/49001/46018
> File chrome/common/webkit_param_traits.h (right):
> 
> http://codereview.chromium.org/3107026/diff/49001/46018#newcode332
> chrome/common/webkit_param_traits.h:332: struct
> ParamTraits<WebKit::WebFileSystem::Type> {
> There's a new way to write this that is not yet widely used:
> 
> template <>
> struct SimilarTypeTraits<WebKit::WebFileSystem::Type> {
>   typedef int Type;
> };
> 
> This is a lot simpler.  You don't get the nice logging, but that's not a big
> deal.  (We should really use SimilarTypeTraits elsewhere!)

Oh I had wondered if I could do so because no one was doing that for WebKit
types.  Thanks I'll do that!