Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10126004:
fileapi: FileWriter and LocalFileWriter. (Closed)
|
Created:
8 years, 8 months ago by kinaba Modified:
8 years, 8 months ago Reviewers:
kinuko CC:
chromium-reviews, pam+watch_chromium.org, kinuko+watch, darin-cc_chromium.org, Dai Mikurube (NOT FULLTIME), tzik, zel Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionfileapi: FileWriter and LocalFileWriter.
We need these write stream interface for implementing generic cross-filesystem operation and to eventually deprecate OpenFileSystem::Write and PlatformFile related interface.
BUG=123993
TEST=unit_tests --gtest_filter='*LocalFileWriter*'
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133892
Patch Set 1 : First working version. #
Total comments: 24
Patch Set 2 : Drop Seek(), loosen Cancel() and destructor's requirements, and add constuctor parameters. #
Total comments: 12
Patch Set 3 : Reflect review. #
Total comments: 10
Patch Set 4 : Drop CREAET_OR_TRUNCATE mode. Reflect review. #
Total comments: 4
Patch Set 5 : Review. #Patch Set 6 : #include #
Messages
Total messages: 13 (0 generated)
Thanks! Added my first comments. http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/file_writer.h File webkit/fileapi/file_writer.h (right): http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/file_writer.... webkit/fileapi/file_writer.h:22: // Cancels the in-flight operation (if any) and closes. This comment sounds like inconsistent from what we do in LocalFileWriter. http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/file_writer.... webkit/fileapi/file_writer.h:38: // Move the current cursor position to |offset| asynchronously. nit: Moves http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/file_writer.... webkit/fileapi/file_writer.h:44: // It is invalid to call Seek while there is an in-flight async operation. I said we could keep this as is for now, but on the second thought I may vote to not adding this. Callers can be happy without this method if Writer accepts initial offset value, and some writer may not need asynchronous Seek. And CreateFileReader takes offset parameter, so we may want to make this aligned with that. (I could flip my opinion later, but I usually do not like adding methods while they may not be necessary) http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... File webkit/fileapi/local_file_writer.cc (right): http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer.cc:22: } // anonymous namespace nit: just 'namespace' would be ok (and more common) http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer.cc:70: DCHECK(has_pending_operation_); Could we simply run the callback (asynchronously) when we have no pending operations? Then the caller could call Cancel() at any time and could do the final cleanup in its callback (e.g. delete the instance etc). http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... File webkit/fileapi/local_file_writer.h (right): http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer.h:14: #include "base/memory/ref_counted.h" Do we need this? http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer.h:31: // operations. Can we loosen it? I think it's ok (if we make it clear on FileWriter interface) but might feel a bit strict when we do not have any obstacles to allow it (and we do not seem so). http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer.h:42: // Opens the file stream and then call |main_operation|. |main_operation| is nit: call -> calls http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer.h:71: base::WeakPtrFactory<LocalFileWriter> weak_factory_; If we don't allow deleting this while there's an async operation do we need weak_factory? http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... File webkit/fileapi/local_file_writer_unittest.cc (right): http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer_unittest.cc:26: } Everything below this could be protected methods. nit: please put an empty line between non-trivial methods. http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer_unittest.cc:52: std::string FileContent(const FilePath& path) { naming-nit: GetFileContent http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer_unittest.cc:59: file_util::WriteFile(path, data.c_str(), data.size()); 'Empty'? :)
Thanks for many feedbacks! Seek, Cancel and destructors' are changed per your suggestions. In addition, I added one more constructor to LocalFileWriter that specifies the file open mode (CREATE or OPEN). I think we need CREATE for implemention file Move() and OPEN for Write(). Correct me if there can be a better option :). http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/file_writer.h File webkit/fileapi/file_writer.h (right): http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/file_writer.... webkit/fileapi/file_writer.h:22: // Cancels the in-flight operation (if any) and closes. On 2012/04/20 11:26:37, kinuko wrote: > This comment sounds like inconsistent from what we do in LocalFileWriter. Adjusted the LocalFileWriter's implementation. http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/file_writer.... webkit/fileapi/file_writer.h:38: // Move the current cursor position to |offset| asynchronously. On 2012/04/20 11:26:37, kinuko wrote: > nit: Moves Done. http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/file_writer.... webkit/fileapi/file_writer.h:44: // It is invalid to call Seek while there is an in-flight async operation. On 2012/04/20 11:26:37, kinuko wrote: > I said we could keep this as is for now, but on the second thought I may vote to > not adding this. Callers can be happy without this method if Writer accepts > initial offset value, and some writer may not need asynchronous Seek. And > CreateFileReader takes offset parameter, so we may want to make this aligned > with that. > > (I could flip my opinion later, but I usually do not like adding methods while > they may not be necessary) I'm now inclined to drop Seek() and restrict seeking only to the initial one, which should specified when a Writer is created. Let's drop it now. I think we can add it later without problem if it turned out to be needed (as done in this patch set :)). http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... File webkit/fileapi/local_file_writer.cc (right): http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer.cc:22: } // anonymous namespace On 2012/04/20 11:26:37, kinuko wrote: > nit: just 'namespace' would be ok (and more common) Done. http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer.cc:70: DCHECK(has_pending_operation_); On 2012/04/20 11:26:37, kinuko wrote: > Could we simply run the callback (asynchronously) when we have no pending > operations? > Then the caller could call Cancel() at any time and could do the final cleanup > in its callback (e.g. delete the instance etc). I agree. Done. http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... File webkit/fileapi/local_file_writer.h (right): http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer.h:14: #include "base/memory/ref_counted.h" On 2012/04/20 11:26:37, kinuko wrote: > Do we need this? No. Removed. http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer.h:31: // operations. Can we loosen it? On 2012/04/20 11:26:37, kinuko wrote: > I think it's ok (if we make it clear on FileWriter interface) but might feel a > bit strict when we do not have any obstacles to allow it (and we do not seem > so). Removed the restriction (removed DCHECK()'s for watching the conditions). http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer.h:42: // Opens the file stream and then call |main_operation|. |main_operation| is On 2012/04/20 11:26:37, kinuko wrote: > nit: call -> calls Done. http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer.h:71: base::WeakPtrFactory<LocalFileWriter> weak_factory_; On 2012/04/20 11:26:37, kinuko wrote: > If we don't allow deleting this while there's an async operation do we need > weak_factory? Changed the semantics to allow delete while alive operations. So we need it again. http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... File webkit/fileapi/local_file_writer_unittest.cc (right): http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer_unittest.cc:26: } On 2012/04/20 11:26:37, kinuko wrote: > Everything below this could be protected methods. > > nit: please put an empty line between non-trivial methods. Done. http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer_unittest.cc:52: std::string FileContent(const FilePath& path) { On 2012/04/20 11:26:37, kinuko wrote: > naming-nit: GetFileContent Done. http://codereview.chromium.org/10126004/diff/2001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer_unittest.cc:59: file_util::WriteFile(path, data.c_str(), data.size()); On 2012/04/20 11:26:37, kinuko wrote: > 'Empty'? :) Surely not!
The code looking good to me. Comments on the test code will come later. http://codereview.chromium.org/10126004/diff/8001/webkit/fileapi/file_writer.h File webkit/fileapi/file_writer.h (right): http://codereview.chromium.org/10126004/diff/8001/webkit/fileapi/file_writer.... webkit/fileapi/file_writer.h:44: // that case, |callback| is called back with net::ERR_FAILED. ERR_UNEXPECTED? Hmm on the second thought it might be better to just make this function return an integer error code (as well as Write), and make it return ERR_UNEXPECTED if we have no async operations or ERR_IO_PENDING if we have async operations and do/can not cancel them immediately. http://codereview.chromium.org/10126004/diff/8001/webkit/fileapi/local_file_w... File webkit/fileapi/local_file_writer.cc (right): http://codereview.chromium.org/10126004/diff/8001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer.cc:116: main_operation.Run(); nit: could we return here earlier? http://codereview.chromium.org/10126004/diff/8001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer.cc:143: Seek returns the new offset and in most cases we'd also want to see if the returned |result| is equal to what we've given to Seek (i.e. initial_offset_). http://codereview.chromium.org/10126004/diff/8001/webkit/fileapi/local_file_w... File webkit/fileapi/local_file_writer.h (right): http://codereview.chromium.org/10126004/diff/8001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer.h:25: // Thin wrapper around net::FileStream nit: Please make comments look like sentence and end them with period. http://codereview.chromium.org/10126004/diff/8001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer.h:34: // |initial_offset|. If |mode| is |CREATE|, |file_path| is set to an empty This might feel a bit ambiguous-- does this mean TRUNC (in posix open)? Is it ok to create a LocalFileWriter with mode==CREATE?
Updated. Other than that, the win_rel trybot failure is a real failure. I'll fix it in the next update. http://codereview.chromium.org/10126004/diff/8001/webkit/fileapi/file_writer.h File webkit/fileapi/file_writer.h (right): http://codereview.chromium.org/10126004/diff/8001/webkit/fileapi/file_writer.... webkit/fileapi/file_writer.h:44: // that case, |callback| is called back with net::ERR_FAILED. On 2012/04/23 09:29:57, kinuko wrote: > ERR_UNEXPECTED? > > Hmm on the second thought it might be better to just make this function return > an integer error code (as well as Write), and make it return ERR_UNEXPECTED if > we have no async operations or ERR_IO_PENDING if we have async operations and > do/can not cancel them immediately. Sounds much nicer! Done. http://codereview.chromium.org/10126004/diff/8001/webkit/fileapi/local_file_w... File webkit/fileapi/local_file_writer.cc (right): http://codereview.chromium.org/10126004/diff/8001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer.cc:116: main_operation.Run(); On 2012/04/23 09:29:57, kinuko wrote: > nit: could we return here earlier? Done. http://codereview.chromium.org/10126004/diff/8001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer.cc:143: On 2012/04/23 09:29:57, kinuko wrote: > Seek returns the new offset and in most cases we'd also want to see if the > returned |result| is equal to what we've given to Seek (i.e. initial_offset_). Modified to check the equality and to return net::ERR_INVALID_ARGUMENT when they do not match. http://codereview.chromium.org/10126004/diff/8001/webkit/fileapi/local_file_w... File webkit/fileapi/local_file_writer.h (right): http://codereview.chromium.org/10126004/diff/8001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer.h:25: // Thin wrapper around net::FileStream On 2012/04/23 09:29:57, kinuko wrote: > nit: Please make comments look like sentence and end them with period. Done. http://codereview.chromium.org/10126004/diff/8001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer.h:34: // |initial_offset|. If |mode| is |CREATE|, |file_path| is set to an empty On 2012/04/23 09:29:57, kinuko wrote: > This might feel a bit ambiguous-- does this mean TRUNC (in posix open)? What I intend is O_CREAT|O_WRONLY|O_TRUNC, which is posix creat(), but I agree it sounds ambiguous. Give me some time to come up with a better clearer name (to come in the next patch set)... > Is it ok to create a LocalFileWriter with mode==CREATE? Yes, I think. The motivation for this is dependent on how we implement the generic cross-fs Copy/Move. What I had in my mind was the following one: void GenericCopy(fileA, fileB) { FileReader* r = MountPointProviderForFileA()->CreateFileReader(fileA, offset=0); FileWriter* w = MountPointProviderForFileB()->CreateFileWriter(fileB, offset=0, mode=CREATE /*or better name*/); DoTheCopyByRepeatingReadAndWrite(r, w); } Another option is to keep file writer for modifications on existing files, and add something like EnsureEmptyFileExists() in somewhere in the fileapi layer, to be called before CreateFileWriter() in GenericCopy(). I thought adding mode=CREATE like stuff to each file writer would be simler, but I'm not so sure nor want to insist much on it. It is ok to drop the mode for now and consider it later.
As for the win_rel failure please see the inline comment about message loop type. http://codereview.chromium.org/10126004/diff/8001/webkit/fileapi/local_file_w... File webkit/fileapi/local_file_writer.h (right): http://codereview.chromium.org/10126004/diff/8001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer.h:34: // |initial_offset|. If |mode| is |CREATE|, |file_path| is set to an empty On 2012/04/23 10:55:14, kinaba wrote: > On 2012/04/23 09:29:57, kinuko wrote: > > This might feel a bit ambiguous-- does this mean TRUNC (in posix open)? > > What I intend is O_CREAT|O_WRONLY|O_TRUNC, which is posix creat(), but I agree > it sounds ambiguous. Give me some time to come up with a better clearer name (to > come in the next patch set)... > > > Is it ok to create a LocalFileWriter with mode==CREATE? > > Yes, I think. The motivation for this is dependent on how we implement the > generic cross-fs Copy/Move. What I had in my mind was the following one: > void GenericCopy(fileA, fileB) { > FileReader* r = MountPointProviderForFileA()->CreateFileReader(fileA, > offset=0); > FileWriter* w = MountPointProviderForFileB()->CreateFileWriter(fileB, > offset=0, mode=CREATE /*or better name*/); > DoTheCopyByRepeatingReadAndWrite(r, w); > } > > Another option is to keep file writer for modifications on existing files, and > add something like EnsureEmptyFileExists() in somewhere in the fileapi layer, to > be called before CreateFileWriter() in GenericCopy(). > > I thought adding mode=CREATE like stuff to each file writer would be simler, but > I'm not so sure nor want to insist much on it. It is ok to drop the mode for now > and consider it later. I can imagine the situation. I think it's ok to have the mode if we could clarify the naming / mode enough. Let's think about the naming a bit more.. CREATE_OR_TRUNCATE_IF_EXISTS, FAIL_IF_NOT_EXISTS hmm. http://codereview.chromium.org/10126004/diff/15002/webkit/fileapi/local_file_... File webkit/fileapi/local_file_writer.cc (right): http://codereview.chromium.org/10126004/diff/15002/webkit/fileapi/local_file_... webkit/fileapi/local_file_writer.cc:138: result = net::ERR_INVALID_ARGUMENT; This is not necessarily the given argument is invalid. (It's just a file and it could be changed at any time) Hmm... let's use net::ERR_FAILED for now, but please add a TODO comment to add a better, more specific error code. http://codereview.chromium.org/10126004/diff/15002/webkit/fileapi/local_file_... File webkit/fileapi/local_file_writer_unittest.cc (right): http://codereview.chromium.org/10126004/diff/15002/webkit/fileapi/local_file_... webkit/fileapi/local_file_writer_unittest.cc:64: private: I think you'll need to run this test in unit_tests or in other tests than test_shell_tests, since test_shell_tests usually run each test in UI thread (on TYPE_UI message loop), while FileStream needs to be created on IO thread (on TYPE_IO message loop). You can move this test to unit_tests and explicitly create an IO message loop as a default one like following: class LocalFileWriterTest : ... { public: LocalFileWriterTest() : message_loop_(MessageLoop::TYPE_IO) {} ... private: MessageLoop message_loop_; ... } http://codereview.chromium.org/10126004/diff/15002/webkit/fileapi/local_file_... webkit/fileapi/local_file_writer_unittest.cc:83: } You may want to run RunAllPending() after resetting the writer pointer to force running asynchronous Close. http://codereview.chromium.org/10126004/diff/15002/webkit/fileapi/local_file_... webkit/fileapi/local_file_writer_unittest.cc:110: EXPECT_NE(net::OK, WriteStringToWriter(writer.get(), "foo")); Would be nice to compare the return code with a specific error code if possible. http://codereview.chromium.org/10126004/diff/15002/webkit/fileapi/local_file_... webkit/fileapi/local_file_writer_unittest.cc:134: EXPECT_EQ(net::OK, cancel_result); Could we also test calling Cancel when there's no inflight operation?
http://codereview.chromium.org/10126004/diff/8001/webkit/fileapi/local_file_w... File webkit/fileapi/local_file_writer.h (right): http://codereview.chromium.org/10126004/diff/8001/webkit/fileapi/local_file_w... webkit/fileapi/local_file_writer.h:34: // |initial_offset|. If |mode| is |CREATE|, |file_path| is set to an empty On 2012/04/23 10:55:14, kinaba wrote: > On 2012/04/23 09:29:57, kinuko wrote: > > This might feel a bit ambiguous-- does this mean TRUNC (in posix open)? > > What I intend is O_CREAT|O_WRONLY|O_TRUNC, which is posix creat(), but I agree > it sounds ambiguous. Give me some time to come up with a better clearer name (to > come in the next patch set)... > > > Is it ok to create a LocalFileWriter with mode==CREATE? > > Yes, I think. The motivation for this is dependent on how we implement the > generic cross-fs Copy/Move. What I had in my mind was the following one: > void GenericCopy(fileA, fileB) { > FileReader* r = MountPointProviderForFileA()->CreateFileReader(fileA, > offset=0); > FileWriter* w = MountPointProviderForFileB()->CreateFileWriter(fileB, > offset=0, mode=CREATE /*or better name*/); > DoTheCopyByRepeatingReadAndWrite(r, w); > } > > Another option is to keep file writer for modifications on existing files, and > add something like EnsureEmptyFileExists() in somewhere in the fileapi layer, to > be called before CreateFileWriter() in GenericCopy(). By the way we'll also need to check if the given path is a directory or not etc. You may want to take a look at CrossFileUtilHelper::PerformErrorCheckAndPreparation in webkit/fileapi/file_util_helper.cc to see what checks we'd need to perform and what could be hidden in the writer code. (Or as you say we could put off adding the mode in this version) > I thought adding mode=CREATE like stuff to each file writer would be simler, but > I'm not so sure nor want to insist much on it. It is ok to drop the mode for now > and consider it later.
Cc'ing Zel.
Addressed all comments. Thank you for the advises! For open/creation mode, let me put it off until we really need it. Again, I think we can add it later easily. Sorry about not being deterministic (^^;. > By the way we'll also need to check if the given path is a directory or not etc. Right, but I think it's somehow what we already have in a filesystem-independent form that we can reuse. On the other hand, truncate_if_exists_and_create_otherwise stuff is not there yet (or maybe GetMetadata() + Truncate() once GData filesystem implemented Truncate()?), so we need to add the new stuff at some point. http://codereview.chromium.org/10126004/diff/15002/webkit/fileapi/local_file_... File webkit/fileapi/local_file_writer.cc (right): http://codereview.chromium.org/10126004/diff/15002/webkit/fileapi/local_file_... webkit/fileapi/local_file_writer.cc:138: result = net::ERR_INVALID_ARGUMENT; On 2012/04/23 12:50:29, kinuko wrote: > This is not necessarily the given argument is invalid. (It's just a file and it > could be changed at any time) > Hmm... let's use net::ERR_FAILED for now, but please add a TODO comment to add a > better, more specific error code. Done. http://codereview.chromium.org/10126004/diff/15002/webkit/fileapi/local_file_... File webkit/fileapi/local_file_writer_unittest.cc (right): http://codereview.chromium.org/10126004/diff/15002/webkit/fileapi/local_file_... webkit/fileapi/local_file_writer_unittest.cc:64: private: On 2012/04/23 12:50:29, kinuko wrote: > I think you'll need to run this test in unit_tests or in other tests than > test_shell_tests, since test_shell_tests usually run each test in UI thread (on > TYPE_UI message loop), while FileStream needs to be created on IO thread (on > TYPE_IO message loop). > > You can move this test to unit_tests and explicitly create an IO message loop as > a default one like following: > > class LocalFileWriterTest : ... { > public: > LocalFileWriterTest() : message_loop_(MessageLoop::TYPE_IO) {} > ... > > private: > MessageLoop message_loop_; > ... > } Done. Thanks for the advice! http://codereview.chromium.org/10126004/diff/15002/webkit/fileapi/local_file_... webkit/fileapi/local_file_writer_unittest.cc:83: } On 2012/04/23 12:50:29, kinuko wrote: > You may want to run RunAllPending() after resetting the writer pointer to force > running asynchronous Close. Done. http://codereview.chromium.org/10126004/diff/15002/webkit/fileapi/local_file_... webkit/fileapi/local_file_writer_unittest.cc:110: EXPECT_NE(net::OK, WriteStringToWriter(writer.get(), "foo")); On 2012/04/23 12:50:29, kinuko wrote: > Would be nice to compare the return code with a specific error code if possible. Done. http://codereview.chromium.org/10126004/diff/15002/webkit/fileapi/local_file_... webkit/fileapi/local_file_writer_unittest.cc:134: EXPECT_EQ(net::OK, cancel_result); On 2012/04/23 12:50:29, kinuko wrote: > Could we also test calling Cancel when there's no inflight operation? I meant CancelWhenNoInflightOperation (Line 115) to be the one. I added one more case to check calling Cancel after an operation is completed.
lgtm. http://codereview.chromium.org/10126004/diff/27001/webkit/fileapi/file_writer.h File webkit/fileapi/file_writer.h (right): http://codereview.chromium.org/10126004/diff/27001/webkit/fileapi/file_writer... webkit/fileapi/file_writer.h:11: #include "net/base/net_errors.h" This file is probably not needed in .h http://codereview.chromium.org/10126004/diff/27001/webkit/fileapi/file_writer... webkit/fileapi/file_writer.h:44: // where Cancel() was called when the cancel has completed. Could you also mention that it returns with ERR_UNEXPECTED if it has no async operations to cancel? Also please add 'it is invalid to call Cancel more than once'.
http://codereview.chromium.org/10126004/diff/27001/webkit/fileapi/file_writer.h File webkit/fileapi/file_writer.h (right): http://codereview.chromium.org/10126004/diff/27001/webkit/fileapi/file_writer... webkit/fileapi/file_writer.h:11: #include "net/base/net_errors.h" On 2012/04/24 09:26:50, kinuko wrote: > This file is probably not needed in .h Done. http://codereview.chromium.org/10126004/diff/27001/webkit/fileapi/file_writer... webkit/fileapi/file_writer.h:44: // where Cancel() was called when the cancel has completed. On 2012/04/24 09:26:50, kinuko wrote: > Could you also mention that it returns with ERR_UNEXPECTED if it has no async > operations to cancel? Also please add 'it is invalid to call Cancel more than > once'. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kinaba@chromium.org/10126004/33002
Change committed as 133892 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
