Implement SandboxFileWriter and rewrite FileWriterDelegate to use it

Implement SandboxFileWriter and rewrite FileWriterDelegate to use it

BUG=127529
TEST=FileWriterDelegate*, FileSystemOperationWrite*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138236

[kinuko, 2012-05-10 08:46:39]

The patch is diff'ed from http://codereview.chromium.org/10386069/.
Right now it's not using FileWriter::Cancel() for Cancel() implementation.

[kinuko, 2012-05-15 05:33:59]

I think now it's ready for review.  Can you take a look?
ericu,tzik: in general
kinaba: FileWriter related implementation/binding

[kinaba, 2012-05-15 07:40:04]

LGTM on SandboxFileWriter and its usage.

http://codereview.chromium.org/10387054/diff/10001/webkit/fileapi/sandbox_fil...
File webkit/fileapi/sandbox_file_writer.cc (right):

http://codereview.chromium.org/10387054/diff/10001/webkit/fileapi/sandbox_fil...
webkit/fileapi/sandbox_file_writer.cc:33: int64 file_size) {
Not at all related to the intent of the current CL, but can we add
DCHECK(file_offset <= file_size);
here? I believe the condition is enforced in some higher layer so no real need
for worrying, but it'd be easier for quota newbie like me :) to read the code if
it is explicit.

[kinuko, 2012-05-15 11:20:36]

http://codereview.chromium.org/10387054/diff/10001/webkit/fileapi/sandbox_fil...
File webkit/fileapi/sandbox_file_writer.cc (right):

http://codereview.chromium.org/10387054/diff/10001/webkit/fileapi/sandbox_fil...
webkit/fileapi/sandbox_file_writer.cc:33: int64 file_size) {
On 2012/05/15 07:40:04, kinaba wrote:
> Not at all related to the intent of the current CL, but can we add
> DCHECK(file_offset <= file_size);
> here? I believe the condition is enforced in some higher layer so no real need
> for worrying, but it'd be easier for quota newbie like me :) to read the code
if
> it is explicit.

Good point, done.

[tzik, 2012-05-15 12:07:29]

LGTM

[ericu, 2012-05-16 00:23:17]

I'm still digesting this, especially the finer details of cancel, but overall I
like it.

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation.cc:378: void
FileSystemOperation::Cancel(const StatusCallback& cancel_callback) {
TODO(ericu): Does cancel still work?

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/sandbox_file...
File webkit/fileapi/sandbox_file_writer.cc (right):

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/sandbox_file...
webkit/fileapi/sandbox_file_writer.cc:30: // return the new quota value.
The comments and name here aren't clear.  They should explain what and why the
adjustment is, not just that there is one.

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/sandbox_file...
webkit/fileapi/sandbox_file_writer.cc:92: DCHECK(local_file_writer_.get());
If you call Write, then quickly call Cancel before initialization completes,
won't local_file_writer_ be NULL?

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/sandbox_file...
File webkit/fileapi/sandbox_file_writer.h (right):

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/sandbox_file...
webkit/fileapi/sandbox_file_writer.h:22: class SandboxFileWriter : public
FileWriter {
Side note: FileWriter is a really confusing name for the parent class, given
that this is implementing a piece of WebCore::FileWriter and living under
src/webkit.

It's a fine name on its own, but here it's got too much competition.

[kinuko, 2012-05-16 04:10:12]

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation.cc:378: void
FileSystemOperation::Cancel(const StatusCallback& cancel_callback) {
On 2012/05/16 00:23:17, ericu wrote:
> TODO(ericu): Does cancel still work?

At least the layout tests seem to work.

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/sandbox_file...
File webkit/fileapi/sandbox_file_writer.cc (right):

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/sandbox_file...
webkit/fileapi/sandbox_file_writer.cc:30: // return the new quota value.
On 2012/05/16 00:23:17, ericu wrote:
> The comments and name here aren't clear.  They should explain what and why the
> adjustment is, not just that there is one.

Updated the comment.

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/sandbox_file...
webkit/fileapi/sandbox_file_writer.cc:92: DCHECK(local_file_writer_.get());
On 2012/05/16 00:23:17, ericu wrote:
> If you call Write, then quickly call Cancel before initialization completes,
> won't local_file_writer_ be NULL?

You're right, I just found this problem while running a layout test.  Removed
this line.

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/sandbox_file...
File webkit/fileapi/sandbox_file_writer.h (right):

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/sandbox_file...
webkit/fileapi/sandbox_file_writer.h:22: class SandboxFileWriter : public
FileWriter {
On 2012/05/16 00:23:17, ericu wrote:
> Side note: FileWriter is a really confusing name for the parent class, given
> that this is implementing a piece of WebCore::FileWriter and living under
> src/webkit.
> 
> It's a fine name on its own, but here it's got too much competition.

Right, but we also have duplicated FileReader, FileStream, WorkerThread etc... I
added an explicit comment in file_writer.h like "this is not for File API
FileWriter".

[kinuko, 2012-05-16 12:46:52]

http://codereview.chromium.org/10387054/diff/1015/webkit/fileapi/file_writer_...
File webkit/fileapi/file_writer_delegate_unittest.cc (right):

http://codereview.chromium.org/10387054/diff/1015/webkit/fileapi/file_writer_...
webkit/fileapi/file_writer_delegate_unittest.cc:244:
file_writer_delegate_.reset();
Note: these reordering is made to make sure we flush/close file before querying
the physical disk usage.

[ericu, 2012-05-17 00:04:13]

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation.cc:378: void
FileSystemOperation::Cancel(const StatusCallback& cancel_callback) {
On 2012/05/16 04:10:12, kinuko wrote:
> On 2012/05/16 00:23:17, ericu wrote:
> > TODO(ericu): Does cancel still work?
> 
> At least the layout tests seem to work.

Sorry--that wasn't supposed to be sent.  I leave myself notes on what I'm still
verifying during code reviews.

That being said, the layout tests are unfortunately not a guarantee of
correctness.  It's really hard to test all possible cases of race conditions and
such during a cancel. It looks pretty good, subject to those additions I
suggest, though.

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/sandbox_file...
File webkit/fileapi/sandbox_file_writer.cc (right):

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/sandbox_file...
webkit/fileapi/sandbox_file_writer.cc:30: // return the new quota value.
On 2012/05/16 04:10:12, kinuko wrote:
> On 2012/05/16 00:23:17, ericu wrote:
> > The comments and name here aren't clear.  They should explain what and why
the
> > adjustment is, not just that there is one.
> 
> Updated the comment.

Very nice--thanks.

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/sandbox_file...
File webkit/fileapi/sandbox_file_writer.h (right):

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/sandbox_file...
webkit/fileapi/sandbox_file_writer.h:22: class SandboxFileWriter : public
FileWriter {
On 2012/05/16 04:10:12, kinuko wrote:
> On 2012/05/16 00:23:17, ericu wrote:
> > Side note: FileWriter is a really confusing name for the parent class, given
> > that this is implementing a piece of WebCore::FileWriter and living under
> > src/webkit.
> > 
> > It's a fine name on its own, but here it's got too much competition.
> 
> Right, but we also have duplicated FileReader, FileStream, WorkerThread etc...

That's a great list of names to fix.  I think it's a bit late to fix
WorkerThread [maybe not?], but this one's new enough to change without anyone
complaining.

> I added an explicit comment in file_writer.h like "this is not for File API
> FileWriter".

That's certainly better than nothing, but as it won't appear at every use, it
won't stop the confusion.  Thank you for the TODO.

http://codereview.chromium.org/10387054/diff/1015/webkit/fileapi/sandbox_file...
File webkit/fileapi/sandbox_file_writer.cc (right):

http://codereview.chromium.org/10387054/diff/1015/webkit/fileapi/sandbox_file...
webkit/fileapi/sandbox_file_writer.cc:172: int64 usage, int64 quota) {
This looks like it needs a call to CancelIfRequested.

http://codereview.chromium.org/10387054/diff/1015/webkit/fileapi/sandbox_file...
webkit/fileapi/sandbox_file_writer.cc:186: int init_status) {
This looks like it needs a call to CancelIfRequested().

[kinuko, 2012-05-17 04:45:26]

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation.cc:378: void
FileSystemOperation::Cancel(const StatusCallback& cancel_callback) {
On 2012/05/17 00:04:13, ericu wrote:
> On 2012/05/16 04:10:12, kinuko wrote:
> > On 2012/05/16 00:23:17, ericu wrote:
> > > TODO(ericu): Does cancel still work?
> > 
> > At least the layout tests seem to work.
> 
> Sorry--that wasn't supposed to be sent.  I leave myself notes on what I'm
still
> verifying during code reviews.
> 
> That being said, the layout tests are unfortunately not a guarantee of
> correctness.  It's really hard to test all possible cases of race conditions
and
> such during a cancel. It looks pretty good, subject to those additions I
> suggest, though.

Yup, totally agreed. I'm not assuming current tests would guarantee any
correctness.

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/sandbox_file...
File webkit/fileapi/sandbox_file_writer.h (right):

http://codereview.chromium.org/10387054/diff/8011/webkit/fileapi/sandbox_file...
webkit/fileapi/sandbox_file_writer.h:22: class SandboxFileWriter : public
FileWriter {
On 2012/05/17 00:04:13, ericu wrote:
> On 2012/05/16 04:10:12, kinuko wrote:
> > On 2012/05/16 00:23:17, ericu wrote:
> > > Side note: FileWriter is a really confusing name for the parent class,
given
> > > that this is implementing a piece of WebCore::FileWriter and living under
> > > src/webkit.
> > > 
> > > It's a fine name on its own, but here it's got too much competition.
> > 
> > Right, but we also have duplicated FileReader, FileStream, WorkerThread
etc...
> 
> That's a great list of names to fix.  I think it's a bit late to fix
> WorkerThread [maybe not?], but this one's new enough to change without anyone
> complaining.
> 
> > I added an explicit comment in file_writer.h like "this is not for File API
> > FileWriter".
> 
> That's certainly better than nothing, but as it won't appear at every use, it
> won't stop the confusion.  Thank you for the TODO.

Filed a new bug. Either we change the name immediately or not we'd probably want
to do that in a separate patch.

http://codereview.chromium.org/10387054/diff/1015/webkit/fileapi/sandbox_file...
File webkit/fileapi/sandbox_file_writer.cc (right):

http://codereview.chromium.org/10387054/diff/1015/webkit/fileapi/sandbox_file...
webkit/fileapi/sandbox_file_writer.cc:172: int64 usage, int64 quota) {
On 2012/05/17 00:04:13, ericu wrote:
> This looks like it needs a call to CancelIfRequested.

Done.

http://codereview.chromium.org/10387054/diff/1015/webkit/fileapi/sandbox_file...
webkit/fileapi/sandbox_file_writer.cc:186: int init_status) {
On 2012/05/17 00:04:13, ericu wrote:
> This looks like it needs a call to CancelIfRequested().

Done.

[kinaba, 2012-05-21 09:26:58]

I realized one more thing just now. Let me add it, which is needed for
implementing GDataFileSystem::Write() by reusing the refactored
FileWriterDelegate.

http://codereview.chromium.org/10387054/diff/23001/webkit/fileapi/file_writer...
File webkit/fileapi/file_writer_delegate.h (right):

http://codereview.chromium.org/10387054/diff/23001/webkit/fileapi/file_writer...
webkit/fileapi/file_writer_delegate.h:28: FileSystemOperation* write_operation,
Can you replace the parameter with const
FileSystemOperationInterface::WriteCallback&, and thus remove the dependency on
FileSystemOperation itself? Thanks to the decoupling of quota part, it now looks
that all accesses to the parameter is for calling ->DidWrite().

[kinuko, 2012-05-21 11:20:21]

http://codereview.chromium.org/10387054/diff/23001/webkit/fileapi/file_writer...
File webkit/fileapi/file_writer_delegate.h (right):

http://codereview.chromium.org/10387054/diff/23001/webkit/fileapi/file_writer...
webkit/fileapi/file_writer_delegate.h:28: FileSystemOperation* write_operation,
On 2012/05/21 09:27:00, kinaba wrote:
> Can you replace the parameter with const
> FileSystemOperationInterface::WriteCallback&, and thus remove the dependency
on
> FileSystemOperation itself? Thanks to the decoupling of quota part, it now
looks
> that all accesses to the parameter is for calling ->DidWrite().

Sounds good, done.

[ericu, 2012-05-21 21:46:22]

LGTM

[kinaba, 2012-05-22 04:39:38]

LGTM once again. Thanks!

[commit-bot: I haz the power, 2012-05-22 06:15:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/10387054/28003