This is the IPC and bits of the browser backend for FileWriter. The rest of...

This is the IPC and bits of the browser backend for FileWriter.  The rest of
it's too tied together to include a small amount; this is the most-significant
chunk that I could put up without making too big a changelist.

The backend isn't complete, but you can see where it's going from here.

BUG=none
TEST=none


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

[dumi, 2010-09-23 04:27:09]

a few style nits.

http://codereview.chromium.org/3440021/diff/9001/10003
File chrome/browser/file_system/file_system_dispatcher_host.cc (right):

http://codereview.chromium.org/3440021/diff/9001/10003#newcode234
chrome/browser/file_system/file_system_dispatcher_host.cc:234:
fileapi::FileSystemOperation *write =
" *" --> "* "

http://codereview.chromium.org/3440021/diff/9001/10005
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/3440021/diff/9001/10005#newcode2862
chrome/common/render_messages_internal.h:2862: )
i think ')' should be at the end of the previous line.

http://codereview.chromium.org/3440021/diff/9001/10006
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/3440021/diff/9001/10006#newcode133
webkit/fileapi/file_system_operation.cc:133: DCHECK(!operation_pending_);
these 2 lines should be wrapped in #ifndef NDEBUG. operation_pending_ is defined
only in debug mode.

[ericu, 2010-09-23 17:27:56]

http://codereview.chromium.org/3440021/diff/9001/10003
File chrome/browser/file_system/file_system_dispatcher_host.cc (right):

http://codereview.chromium.org/3440021/diff/9001/10003#newcode234
chrome/browser/file_system/file_system_dispatcher_host.cc:234:
fileapi::FileSystemOperation *write =
On 2010/09/23 04:27:10, dumi wrote:
> " *" --> "* "

Done.

http://codereview.chromium.org/3440021/diff/9001/10005
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/3440021/diff/9001/10005#newcode2862
chrome/common/render_messages_internal.h:2862: )
On 2010/09/23 04:27:10, dumi wrote:
> i think ')' should be at the end of the previous line.

Done.

http://codereview.chromium.org/3440021/diff/9001/10006
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/3440021/diff/9001/10006#newcode133
webkit/fileapi/file_system_operation.cc:133: DCHECK(!operation_pending_);
On 2010/09/23 04:27:10, dumi wrote:
> these 2 lines should be wrapped in #ifndef NDEBUG. operation_pending_ is
defined
> only in debug mode.

Done.

[kinuko, 2010-09-23 21:57:44]

Generally looking good, I wonder if we want to use FileSystemOperation class for
Write/didWrite cases though.  The class is currently tightly coupled with
FileUtilProxy that we won't use for writing.  It might make more sense to have
another class that keeps ongoing write status?

http://codereview.chromium.org/3440021/diff/19001/20007
File webkit/fileapi/file_system_operation.h (right):

http://codereview.chromium.org/3440021/diff/19001/20007#newcode17
webkit/fileapi/file_system_operation.h:17: #include
"chrome/browser/net/chrome_url_request_context.h"
Is it needed?

http://codereview.chromium.org/3440021/diff/19001/20007#newcode18
webkit/fileapi/file_system_operation.h:18: #include "googleurl/src/gurl.h"
I guess forward declaration (class GURL) would be enough here.

[michaeln, 2010-09-23 21:59:12]

http://codereview.chromium.org/3440021/diff/19001/20004
File chrome/browser/file_system/file_system_dispatcher_host.h (right):

http://codereview.chromium.org/3440021/diff/19001/20004#newcode17
chrome/browser/file_system/file_system_dispatcher_host.h:17: #include
"googleurl/src/gurl.h"
i think a forward decl will do

http://codereview.chromium.org/3440021/diff/19001/20006
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/3440021/diff/19001/20006#newcode142
webkit/fileapi/file_system_operation.cc:142: // checks to see if it's been
cancelled.
Delete semantics instead of a Cancel() method and periodic checks for cancel
might work. I know URLRequests can be deleted at will, not positive about
FileStream but i think so too.

http://codereview.chromium.org/3440021/diff/19001/20007
File webkit/fileapi/file_system_operation.h (right):

http://codereview.chromium.org/3440021/diff/19001/20007#newcode17
webkit/fileapi/file_system_operation.h:17: #include
"chrome/browser/net/chrome_url_request_context.h"
We can't include src's from "chrome/" in this library. To initiate a URLRequest,
all you need is URLRequestContext which is defined in
"net/url_request/url_request_context.h"

http://codereview.chromium.org/3440021/diff/19001/20007#newcode63
webkit/fileapi/file_system_operation.h:63: void Truncate(const FilePath& path,
long long length);
In chrome-speak, long long is int64. Ditto elsewhere in this CL.

[ericu, 2010-09-23 23:10:53]

http://codereview.chromium.org/3440021/diff/19001/20004
File chrome/browser/file_system/file_system_dispatcher_host.h (right):

http://codereview.chromium.org/3440021/diff/19001/20004#newcode17
chrome/browser/file_system/file_system_dispatcher_host.h:17: #include
"googleurl/src/gurl.h"
On 2010/09/23 21:59:12, michaeln wrote:
> i think a forward decl will do

Done.

http://codereview.chromium.org/3440021/diff/19001/20006
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/3440021/diff/19001/20006#newcode142
webkit/fileapi/file_system_operation.cc:142: // checks to see if it's been
cancelled.
On 2010/09/23 21:59:12, michaeln wrote:
> Delete semantics instead of a Cancel() method and periodic checks for cancel
> might work. I know URLRequests can be deleted at will, not positive about
> FileStream but i think so too.

Gotcha.  That might work even better...I'll have to see when I get to this bit;
it'll depend on how it all works out.

http://codereview.chromium.org/3440021/diff/19001/20007
File webkit/fileapi/file_system_operation.h (right):

http://codereview.chromium.org/3440021/diff/19001/20007#newcode17
webkit/fileapi/file_system_operation.h:17: #include
"chrome/browser/net/chrome_url_request_context.h"
On 2010/09/23 21:57:45, Kinuko Yasuda wrote:
> Is it needed?

No, it leaked in from code I haven't finished yet.  Removed.

http://codereview.chromium.org/3440021/diff/19001/20007#newcode17
webkit/fileapi/file_system_operation.h:17: #include
"chrome/browser/net/chrome_url_request_context.h"
On 2010/09/23 21:59:12, michaeln wrote:
> We can't include src's from "chrome/" in this library. To initiate a
URLRequest,
> all you need is URLRequestContext which is defined in
> "net/url_request/url_request_context.h"

Thanks; I'll use that in the next CL or two.

http://codereview.chromium.org/3440021/diff/19001/20007#newcode18
webkit/fileapi/file_system_operation.h:18: #include "googleurl/src/gurl.h"
On 2010/09/23 21:57:45, Kinuko Yasuda wrote:
> I guess forward declaration (class GURL) would be enough here.

Done.

http://codereview.chromium.org/3440021/diff/19001/20007#newcode63
webkit/fileapi/file_system_operation.h:63: void Truncate(const FilePath& path,
long long length);
On 2010/09/23 21:59:12, michaeln wrote:
> In chrome-speak, long long is int64. Ditto elsewhere in this CL.

Fixed in all files, including one that wasn't my error.

[ericu, 2010-09-23 23:39:48]

On Thu, Sep 23, 2010 at 2:57 PM,  <kinuko@chromium.org> wrote:
> Generally looking good, I wonder if we want to use FileSystemOperation class
> for
> Write/didWrite cases though.  The class is currently tightly coupled with
> FileUtilProxy that we won't use for writing.  It might make more sense to
> have
> another class that keeps ongoing write status?

There will definitely be another class [which I've not yet written].
I guess the question is just where we delegate to it.  I'm set up to
delegate to it from FileSystemOperation::Write(); the other obvious
place to do it is directly from FileSystemDispatcherHost::OnWrite().
I like the way FileSystemDispatcherHost::Cancel ended up so clean this
way, but it could certainly be done the other way.  Do you have strong
feelings about where the delegation should happen?

> http://codereview.chromium.org/3440021/diff/19001/20007
> File webkit/fileapi/file_system_operation.h (right):
>
> http://codereview.chromium.org/3440021/diff/19001/20007#newcode17
> webkit/fileapi/file_system_operation.h:17: #include
> "chrome/browser/net/chrome_url_request_context.h"
> Is it needed?
>
> http://codereview.chromium.org/3440021/diff/19001/20007#newcode18
> webkit/fileapi/file_system_operation.h:18: #include
> "googleurl/src/gurl.h"
> I guess forward declaration (class GURL) would be enough here.
>
> http://codereview.chromium.org/3440021/show
>

[kinuko, 2010-09-23 23:44:11]

On 2010/09/23 23:39:48, ericu wrote:
> On Thu, Sep 23, 2010 at 2:57 PM,  <mailto:kinuko@chromium.org> wrote:
> > Generally looking good, I wonder if we want to use FileSystemOperation class
> > for
> > Write/didWrite cases though. &nbsp;The class is currently tightly coupled
with
> > FileUtilProxy that we won't use for writing. &nbsp;It might make more sense
to
> > have
> > another class that keeps ongoing write status?
> 
> There will definitely be another class [which I've not yet written].
> I guess the question is just where we delegate to it.  I'm set up to
> delegate to it from FileSystemOperation::Write(); the other obvious
> place to do it is directly from FileSystemDispatcherHost::OnWrite().
> I like the way FileSystemDispatcherHost::Cancel ended up so clean this
> way, but it could certainly be done the other way.  Do you have strong
> feelings about where the delegation should happen?

Never mind, I started feeling that having the related methods in the same
interface would be good actually.

> > http://codereview.chromium.org/3440021/diff/19001/20007
> > File webkit/fileapi/file_system_operation.h (right):
> >
> > http://codereview.chromium.org/3440021/diff/19001/20007#newcode17
> > webkit/fileapi/file_system_operation.h:17: #include
> > "chrome/browser/net/chrome_url_request_context.h"
> > Is it needed?
> >
> > http://codereview.chromium.org/3440021/diff/19001/20007#newcode18
> > webkit/fileapi/file_system_operation.h:18: #include
> > "googleurl/src/gurl.h"
> > I guess forward declaration (class GURL) would be enough here.
> >
> > http://codereview.chromium.org/3440021/show
> >
>

[ericu, 2010-09-24 00:16:15]

http://codereview.chromium.org/3440021/diff/10/19007
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/3440021/diff/10/19007#newcode152
webkit/fileapi/file_system_operation.cc:152: NOTREACHED();
I just noticed that the code in here was test code that should have been removed
pre-upload.  The actual implementation is a little more complicated than what
was here, and pulls in changes to FileUtilProxy, so I've kept that back for
later.

[kinuko, 2010-09-24 00:24:30]

LGTM.

http://codereview.chromium.org/3440021/diff/10/19005
File chrome/browser/file_system/file_system_dispatcher_host.h (right):

http://codereview.chromium.org/3440021/diff/10/19005#newcode53
chrome/browser/file_system/file_system_dispatcher_host.h:53: void OnWrite(
nits: as for indentation seems like the convention in this file (and in others)
is like (I know my code hasn't been fully following this either):

void OnBlah(type parameter1,
            type parameter2,
            ...);

[ericu, 2010-09-24 00:26:22]

http://codereview.chromium.org/3440021/diff/10/19005
File chrome/browser/file_system/file_system_dispatcher_host.h (right):

http://codereview.chromium.org/3440021/diff/10/19005#newcode53
chrome/browser/file_system/file_system_dispatcher_host.h:53: void OnWrite(
On 2010/09/24 00:24:30, Kinuko Yasuda wrote:
> nits: as for indentation seems like the convention in this file (and in
others)
> is like (I know my code hasn't been fully following this either):
> 
> void OnBlah(type parameter1,
>             type parameter2,
>             ...);

Done.