Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
502973005:
[fsp] Buffer consecutive Write() calls. (Closed)
|
Created:
6 years, 3 months ago by mtomasz Modified:
6 years, 2 months ago CC:
chromium-reviews, nkostylev+watch_chromium.org, nhiroki, oshima+watch_chromium.org, stevenjb+watch_chromium.org, kinuko+fileapi, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[fsp] Buffer consecutive Write() calls.
Similarly to BufferingFileStreamReader, this CL introduces a analogical
BufferingFileStreamWriter class which groups consecutive Write() calls in order
to limit number of IPC calls.
TEST=browser_tests: *FileSystemProvider*BufferingFileStreamWriter*
BUG=398338
Committed: https://crrev.com/b75566966d7efcbb5f544f4488bae12ec7aed7c1
Cr-Commit-Position: refs/heads/master@{#300043}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fixed. #
Total comments: 27
Patch Set 3 : Fixed. #Patch Set 4 : Rebased. #Patch Set 5 : Fixed. #Patch Set 6 : Fixed compile issues. #Patch Set 7 : Addressed comments. #Patch Set 8 : Revert to the previous solution. #Patch Set 9 : Added a comment. #
Total comments: 8
Patch Set 10 : Addressed comments. #Messages
Total messages: 36 (2 generated)
mtomasz@chromium.org changed reviewers: + hashimoto@chromium.org
@hashimoto: PTAL. Thanks.
q1: How much performance improvement can be expected from this mechanism? This is not needed if something similar to crbug.com/399513 is done. What happened to crbug.com/399513? q2: With LocalFileStreamWriter, calling Flush() is optional because the local file system eventually flushes the buffer even when Flush() is not called. But this doesn't work with this implementation. Maybe we should flush the buffer in the dtor, or use timer to flush the data after some amount of time? q3: Can't this be implemented in FileStreamWriter or FileStreamWriter::OperationRunner? There is no need to deal with FileStreamWriter::Flush() whose implementation is empty. Filling the empty FileStreamWriter::Flush() would be simpler. https://codereview.chromium.org/502973005/diff/1/chrome/browser/chromeos/file... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc (right): https://codereview.chromium.org/502973005/diff/1/chrome/browser/chromeos/file... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:174: DCHECK_EQ(net::ERR_IO_PENDING, flush_result); It seems FileStreamWriter::Flush doesn't return ERR_IO_PENDING. Did you test this code with a debug build or GYP_DEFINES=dcheck_always_on=1?
https://codereview.chromium.org/502973005/diff/1/chrome/browser/chromeos/file... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc (right): https://codereview.chromium.org/502973005/diff/1/chrome/browser/chromeos/file... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:174: DCHECK_EQ(net::ERR_IO_PENDING, flush_result); On 2014/08/28 08:32:17, hashimoto wrote: > It seems FileStreamWriter::Flush doesn't return ERR_IO_PENDING. > Did you test this code with a debug build or GYP_DEFINES=dcheck_always_on=1? Done.
> q1: How much performance improvement can be expected from this mechanism? 25x faster for Dropbox (measured). For archives 2x faster (estimated based on reading from zip with 32kb and 512kb chunks). > This is not needed if something similar to crbug.com/399513 is done. The crbug.com/399513 issue would only fix copying and moving. We would still have very slow performance when writing to a file from the renderer process via fileapi. > What happened to crbug.com/399513? The link seems incorrect. > > q2: With LocalFileStreamWriter, calling Flush() is optional because the local > file system eventually flushes the buffer even when Flush() is not called. But > this doesn't work with this implementation. > Maybe we should flush the buffer in the dtor, or use timer to flush the data > after some amount of time? Flush() is always called on completion for non-sandboxed file systems. https://code.google.com/p/chromium/codesearch#chromium/src/webkit/browser/fil... > q3: Can't this be implemented in FileStreamWriter or > FileStreamWriter::OperationRunner? > There is no need to deal with FileStreamWriter::Flush() whose implementation is > empty. > Filling the empty FileStreamWriter::Flush() would be simpler. Do you mean calling Flush() in FileStreamWriter/OperationRunner on completion? It's already implemented: https://code.google.com/p/chromium/codesearch#chromium/src/webkit/browser/fil... > > https://codereview.chromium.org/502973005/diff/1/chrome/browser/chromeos/file... > File > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc > (right): > > https://codereview.chromium.org/502973005/diff/1/chrome/browser/chromeos/file... > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:174: > DCHECK_EQ(net::ERR_IO_PENDING, flush_result); > It seems FileStr#include "base/thread_task_runner_handle.h"eamWriter::Flush doesn't return ERR_IO_PENDING. > Did you test this code with a debug build or GYP_DEFINES=dcheck_always_on=1?
On 2014/08/29 02:49:37, mtomasz wrote: > > q1: How much performance improvement can be expected from this mechanism? > 25x faster for Dropbox (measured). For archives 2x faster (estimated based on > reading from zip with 32kb and 512kb chunks). > > > This is not needed if something similar to crbug.com/399513 is done. > The crbug.com/399513 issue would only fix copying and moving. We would still > have very slow performance when writing to a file from the renderer process via > fileapi. > > > What happened to crbug.com/399513? > The link seems incorrect. > > > > > q2: With LocalFileStreamWriter, calling Flush() is optional because the local > > file system eventually flushes the buffer even when Flush() is not called. But > > this doesn't work with this implementation. > > Maybe we should flush the buffer in the dtor, or use timer to flush the data > > after some amount of time? > Flush() is always called on completion for non-sandboxed file systems. > https://code.google.com/p/chromium/codesearch#chromium/src/webkit/browser/fil... > > > q3: Can't this be implemented in FileStreamWriter or > > FileStreamWriter::OperationRunner? > > There is no need to deal with FileStreamWriter::Flush() whose implementation > is > > empty. > > Filling the empty FileStreamWriter::Flush() would be simpler. > Do you mean calling Flush() in FileStreamWriter/OperationRunner on completion? > It's already implemented: > https://code.google.com/p/chromium/codesearch#chromium/src/webkit/browser/fil... > > > > > > https://codereview.chromium.org/502973005/diff/1/chrome/browser/chromeos/file... > > File > > > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc > > (right): > > > > > https://codereview.chromium.org/502973005/diff/1/chrome/browser/chromeos/file... > > > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:174: > > DCHECK_EQ(net::ERR_IO_PENDING, flush_result); > > It seems FileStr#include "base/thread_task_runner_handle.h"eamWriter::Flush > doesn't return ERR_IO_PENDING. > > Did you test this code with a debug build or GYP_DEFINES=dcheck_always_on=1? @hashimoto: Ping.
On 2014/08/29 02:49:37, mtomasz wrote: > > q1: How much performance improvement can be expected from this mechanism? > 25x faster for Dropbox (measured). For archives 2x faster (estimated based on > reading from zip with 32kb and 512kb chunks). > > > This is not needed if something similar to crbug.com/399513 is done. > The crbug.com/399513 issue would only fix copying and moving. We would still > have very slow performance when writing to a file from the renderer process via > fileapi. > > > What happened to crbug.com/399513? > The link seems incorrect. I'm asking about the status of crbug.com/399513 which may lead to generally increase the data chunk size which makes this change unnecessary. > > > > > q2: With LocalFileStreamWriter, calling Flush() is optional because the local > > file system eventually flushes the buffer even when Flush() is not called. But > > this doesn't work with this implementation. > > Maybe we should flush the buffer in the dtor, or use timer to flush the data > > after some amount of time? > Flush() is always called on completion for non-sandboxed file systems. > https://code.google.com/p/chromium/codesearch#chromium/src/webkit/browser/fil... Seems ShouldFlushOnWriteCompletion() was added to ensure that Flush() is not called when inappropriate, not to ensure that Flush() is called. (https://codereview.chromium.org/264993002) I'm asking here whether you checked that there is no possibility where the user's data gets lost because it stays in the buffer. > > > q3: Can't this be implemented in FileStreamWriter or > > FileStreamWriter::OperationRunner? > > There is no need to deal with FileStreamWriter::Flush() whose implementation > is > > empty. > > Filling the empty FileStreamWriter::Flush() would be simpler. > Do you mean calling Flush() in FileStreamWriter/OperationRunner on completion? > It's already implemented: > https://code.google.com/p/chromium/codesearch#chromium/src/webkit/browser/fil... I'm asking about chromeos::file_system_provider::FileStreamWriter and its inner class OperationRunner. Dealing with its empty Flush() implementation can only be a cause of bugs like you did in the patch set #1. > > > > > > https://codereview.chromium.org/502973005/diff/1/chrome/browser/chromeos/file... > > File > > > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc > > (right): > > > > > https://codereview.chromium.org/502973005/diff/1/chrome/browser/chromeos/file... > > > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:174: > > DCHECK_EQ(net::ERR_IO_PENDING, flush_result); > > It seems FileStr#include "base/thread_task_runner_handle.h"eamWriter::Flush > doesn't return ERR_IO_PENDING. > > Did you test this code with a debug build or GYP_DEFINES=dcheck_always_on=1?
On 2014/09/01 10:52:27, hashimoto wrote: > On 2014/08/29 02:49:37, mtomasz wrote: > > > q1: How much performance improvement can be expected from this mechanism? > > 25x faster for Dropbox (measured). For archives 2x faster (estimated based on > > reading from zip with 32kb and 512kb chunks). > > > > > This is not needed if something similar to crbug.com/399513 is done. > > The crbug.com/399513 issue would only fix copying and moving. We would still > > have very slow performance when writing to a file from the renderer process > via > > fileapi. > > > > > What happened to crbug.com/399513? > > The link seems incorrect. > I'm asking about the status of crbug.com/399513 which may lead to generally > increase the data chunk size which makes this change unnecessary. Fixing crbug.com/399513 would only increase the buffer size for copying and moving, but not for direct writing, so this change is still necessary. Hence, I dropped the idea in crbug.com/399513 in favor of this one, which solves the problem for all cases. > > > > > > > > q2: With LocalFileStreamWriter, calling Flush() is optional because the > local > > > file system eventually flushes the buffer even when Flush() is not called. > But > > > this doesn't work with this implementation. > > > Maybe we should flush the buffer in the dtor, or use timer to flush the data > > > after some amount of time? > > Flush() is always called on completion for non-sandboxed file systems. > > > https://code.google.com/p/chromium/codesearch#chromium/src/webkit/browser/fil... > Seems ShouldFlushOnWriteCompletion() was added to ensure that Flush() is not > called when inappropriate, not to ensure that Flush() is called. > (https://codereview.chromium.org/264993002) > I'm asking here whether you checked that there is no possibility where the > user's data gets lost because it stays in the buffer. ShouldFlushOnWriteCompletion() disables flushing on completion for sandboxed backends only. By default flushing on completion is enabled for all other backends. FSP backend always get a flush on completion, and it's a documented requirement of the BufferingFileStreamWriter. It's tested. If there is no flushing, then browser tests will fail. > > > > > q3: Can't this be implemented in FileStreamWriter or > > > FileStreamWriter::OperationRunner? > > > There is no need to deal with FileStreamWriter::Flush() whose implementation > > is > > > empty. > > > Filling the empty FileStreamWriter::Flush() would be simpler. > > Do you mean calling Flush() in FileStreamWriter/OperationRunner on completion? > > It's already implemented: > > > https://code.google.com/p/chromium/codesearch#chromium/src/webkit/browser/fil... > I'm asking about chromeos::file_system_provider::FileStreamWriter and its inner > class OperationRunner. > Dealing with its empty Flush() implementation can only be a cause of bugs like > you did in the patch set #1. We already have a fsp::BufferingFileStreamReader, which moves the buffering logic outside of the fsp::FileStreamReader. I also want to keep it separate for the writer. The fsp::FileStreamWriter is already complex, so I don't want to add more logic and responsibility directly to it. > > > > > > > > > > > https://codereview.chromium.org/502973005/diff/1/chrome/browser/chromeos/file... > > > File > > > > > > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/502973005/diff/1/chrome/browser/chromeos/file... > > > > > > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:174: > > > DCHECK_EQ(net::ERR_IO_PENDING, flush_result); > > > It seems FileStr#include "base/thread_task_runner_handle.h"eamWriter::Flush > > doesn't return ERR_IO_PENDING. > > > Did you test this code with a debug build or GYP_DEFINES=dcheck_always_on=1?
On 2014/09/02 00:30:18, mtomasz wrote: > On 2014/09/01 10:52:27, hashimoto wrote: > > On 2014/08/29 02:49:37, mtomasz wrote: > > > > q1: How much performance improvement can be expected from this mechanism? > > > 25x faster for Dropbox (measured). For archives 2x faster (estimated based > on > > > reading from zip with 32kb and 512kb chunks). > > > > > > > This is not needed if something similar to crbug.com/399513 is done. > > > The crbug.com/399513 issue would only fix copying and moving. We would still > > > have very slow performance when writing to a file from the renderer process > > via > > > fileapi. > > > > > > > What happened to crbug.com/399513? > > > The link seems incorrect. > > I'm asking about the status of crbug.com/399513 which may lead to generally > > increase the data chunk size which makes this change unnecessary. > > Fixing crbug.com/399513 would only increase the buffer size for copying and > moving, but not for direct writing, so this change is still necessary. > Hence, I dropped the idea in crbug.com/399513 in favor of this one, which solves > the problem for all cases. > > > > > > > > > > > > q2: With LocalFileStreamWriter, calling Flush() is optional because the > > local > > > > file system eventually flushes the buffer even when Flush() is not called. > > But > > > > this doesn't work with this implementation. > > > > Maybe we should flush the buffer in the dtor, or use timer to flush the > data > > > > after some amount of time? > > > Flush() is always called on completion for non-sandboxed file systems. > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/webkit/browser/fil... > > Seems ShouldFlushOnWriteCompletion() was added to ensure that Flush() is not > > called when inappropriate, not to ensure that Flush() is called. > > (https://codereview.chromium.org/264993002) > > I'm asking here whether you checked that there is no possibility where the > > user's data gets lost because it stays in the buffer. > > ShouldFlushOnWriteCompletion() disables flushing on completion for sandboxed > backends only. By default flushing on completion is enabled for all other > backends. > FSP backend always get a flush on completion, and it's a documented requirement > of the BufferingFileStreamWriter. It's meaningless to add a documentation to the header of your FileStreamWriter subclass because it's used with a pointer to the storage::FileStreamWriter interface. If you want to force user code to call Flush(), the documentation should be added to the interface (webkit/browser/fileapi/file_stream_writer.h). > It's tested. If there is no flushing, then browser tests will fail. I don't think it's possible to maintain tests which cover all use cases of your FileStreamWriter. Other people can freely add a new code which uses storage::FileStreamWriter without consulting you, and the code may lack Flush() call and still work nicely with file systems backed by the native filesystem. > > > > > > > > q3: Can't this be implemented in FileStreamWriter or > > > > FileStreamWriter::OperationRunner? > > > > There is no need to deal with FileStreamWriter::Flush() whose > implementation > > > is > > > > empty. > > > > Filling the empty FileStreamWriter::Flush() would be simpler. > > > Do you mean calling Flush() in FileStreamWriter/OperationRunner on > completion? > > > It's already implemented: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/webkit/browser/fil... > > I'm asking about chromeos::file_system_provider::FileStreamWriter and its > inner > > class OperationRunner. > > Dealing with its empty Flush() implementation can only be a cause of bugs like > > you did in the patch set #1. > > We already have a fsp::BufferingFileStreamReader, which moves the buffering > logic outside of the fsp::FileStreamReader. I also want to keep it separate for > the writer. The fsp::FileStreamWriter is already complex, so I don't want to add > more logic and responsibility directly to it. Apparently a class which owns file_system_provider::FileStreamWriter is not a good way to implement this feature. There are other places where this feature can creep in without bloating FileStreamWriter and without dealing with the empty Flush() implementation. (e.g. Inside OperationRunner, a class which owns OperationRunner, a class which is owned by OperationRunner, or etc.) > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/502973005/diff/1/chrome/browser/chromeos/file... > > > > File > > > > > > > > > > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/502973005/diff/1/chrome/browser/chromeos/file... > > > > > > > > > > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:174: > > > > DCHECK_EQ(net::ERR_IO_PENDING, flush_result); > > > > It seems FileStr#include > "base/thread_task_runner_handle.h"eamWriter::Flush > > > doesn't return ERR_IO_PENDING. > > > > Did you test this code with a debug build or > GYP_DEFINES=dcheck_always_on=1?
> > ShouldFlushOnWriteCompletion() disables flushing on completion for sandboxed > > backends only. By default flushing on completion is enabled for all other > > backends. > > FSP backend always get a flush on completion, and it's a documented > requirement > > of the BufferingFileStreamWriter. > It's meaningless to add a documentation to the header of your FileStreamWriter > subclass because it's used with a pointer to the storage::FileStreamWriter > interface. > If you want to force user code to call Flush(), the documentation should be > added to the interface (webkit/browser/fileapi/file_stream_writer.h). > > It's tested. If there is no flushing, then browser tests will fail. > I don't think it's possible to maintain tests which cover all use cases of your > FileStreamWriter. > Other people can freely add a new code which uses storage::FileStreamWriter > without consulting you, and the code may lack Flush() call and still work nicely > with file systems backed by the native filesystem. Isn't the purpose of Flush() to flush buffers? That's exactly what my implementation is doing. If you don't call Flush() then there is no guarantee that bytes written to that FileStreamWriter contents are committed. And that's what happens here. That's true that for LocalFileStreamWriter, we're flushing automatically, but there is no such requirement in the storage::FileStreamWriter interface. There is already a comment: https://code.google.com/p/chromium/codesearch#chromium/src/webkit/browser/fil... "Flushes the data written so far." That sounds pretty obvious. But I can add a comment, that if Flush() is not called, then the written contents may not be committed. As for adding new code which calls FileStreamWriter, how about adding a DCHECK in fsp::BufferingFileStreamWriter destructor to ensure, that Flush() is always called? Alternatively, I can add extra (just in case) code to fsp::BufferingFileStreamWriter which flushes the buffer from the destructor if there was (1) no Cancel() call and (2) there is no in-flight operation in progress. Let me know what you prefer. > Apparently a class which owns file_system_provider::FileStreamWriter is not a > good way to implement this feature. > There are other places where this feature can creep in without bloating > FileStreamWriter and without dealing with the empty Flush() implementation. > (e.g. Inside OperationRunner, a class which owns OperationRunner, a class which > is owned by OperationRunner, or etc.) I'm not sure if I understand the problem with the empty Flush() implementation. What's wrong with it that it requires changing the entire buffering logic upside-down? As for implementing the buffering logic in OperationRunner, I don't want to end up on having the buffering logic for reading in the fsp implementation, but buffering for writing in the class shared among *all* backends.
On 2014/09/03 14:06:30, mtomasz wrote: > > > ShouldFlushOnWriteCompletion() disables flushing on completion for sandboxed > > > backends only. By default flushing on completion is enabled for all other > > > backends. > > > FSP backend always get a flush on completion, and it's a documented > > requirement > > > of the BufferingFileStreamWriter. > > It's meaningless to add a documentation to the header of your FileStreamWriter > > subclass because it's used with a pointer to the storage::FileStreamWriter > > interface. > > If you want to force user code to call Flush(), the documentation should be > > added to the interface (webkit/browser/fileapi/file_stream_writer.h). > > > It's tested. If there is no flushing, then browser tests will fail. > > I don't think it's possible to maintain tests which cover all use cases of > your > > FileStreamWriter. > > Other people can freely add a new code which uses storage::FileStreamWriter > > without consulting you, and the code may lack Flush() call and still work > nicely > > with file systems backed by the native filesystem. > > Isn't the purpose of Flush() to flush buffers? That's exactly what my > implementation is doing. If you don't call Flush() then there is no guarantee > that bytes written to that FileStreamWriter contents are committed. And that's > what happens here. > > That's true that for LocalFileStreamWriter, we're flushing automatically, but > there is no such requirement in the storage::FileStreamWriter interface. > > There is already a comment: > https://code.google.com/p/chromium/codesearch#chromium/src/webkit/browser/fil... > "Flushes the data written so far." > > That sounds pretty obvious. But I can add a comment, that if Flush() is not > called, then the written contents may not be committed. > > As for adding new code which calls FileStreamWriter, how about adding a DCHECK > in fsp::BufferingFileStreamWriter destructor to ensure, that Flush() is always > called? > > Alternatively, I can add extra (just in case) code to > fsp::BufferingFileStreamWriter which flushes the buffer from the destructor if > there was (1) no Cancel() call and (2) there is no in-flight operation in > progress. > > Let me know what you prefer. Either way should be fine. Please choose the way you think which should makes the sense the best. If you add a check to the dtor, it should be CHECK instead of DCHECK because losing users' data is a fatal failure. > > > Apparently a class which owns file_system_provider::FileStreamWriter is not a > > good way to implement this feature. > > There are other places where this feature can creep in without bloating > > FileStreamWriter and without dealing with the empty Flush() implementation. > > (e.g. Inside OperationRunner, a class which owns OperationRunner, a class > which > > is owned by OperationRunner, or etc.) > > I'm not sure if I understand the problem with the empty Flush() implementation. Letting BufferingFileStreamWriter call file_system_provider::FileStreamWriter::Flush() adds nothing good to the product. It just makes the code complicated. > What's wrong with it that it requires changing the entire buffering logic > upside-down? As for implementing the buffering logic in OperationRunner, I don't > want to end up on having the buffering logic for reading in the fsp > implementation, but buffering for writing in the class shared among *all* > backends. I cannot understand what is said here. Why changing file_system_provider::FileStreamWriter::OperationRunner results in affecting all fileapi backend implementations?
On 2014/09/04 11:34:36, hashimoto wrote: > On 2014/09/03 14:06:30, mtomasz wrote: > > > > ShouldFlushOnWriteCompletion() disables flushing on completion for > sandboxed > > > > backends only. By default flushing on completion is enabled for all other > > > > backends. > > > > FSP backend always get a flush on completion, and it's a documented > > > requirement > > > > of the BufferingFileStreamWriter. > > > It's meaningless to add a documentation to the header of your > FileStreamWriter > > > subclass because it's used with a pointer to the storage::FileStreamWriter > > > interface. > > > If you want to force user code to call Flush(), the documentation should be > > > added to the interface (webkit/browser/fileapi/file_stream_writer.h). > > > > It's tested. If there is no flushing, then browser tests will fail. > > > I don't think it's possible to maintain tests which cover all use cases of > > your > > > FileStreamWriter. > > > Other people can freely add a new code which uses storage::FileStreamWriter > > > without consulting you, and the code may lack Flush() call and still work > > nicely > > > with file systems backed by the native filesystem. > > > > Isn't the purpose of Flush() to flush buffers? That's exactly what my > > implementation is doing. If you don't call Flush() then there is no guarantee > > that bytes written to that FileStreamWriter contents are committed. And that's > > what happens here. > > > > That's true that for LocalFileStreamWriter, we're flushing automatically, but > > there is no such requirement in the storage::FileStreamWriter interface. > > > > There is already a comment: > > > https://code.google.com/p/chromium/codesearch#chromium/src/webkit/browser/fil... > > "Flushes the data written so far." > > > > That sounds pretty obvious. But I can add a comment, that if Flush() is not > > called, then the written contents may not be committed. > > > > As for adding new code which calls FileStreamWriter, how about adding a DCHECK > > in fsp::BufferingFileStreamWriter destructor to ensure, that Flush() is always > > called? > > > > Alternatively, I can add extra (just in case) code to > > fsp::BufferingFileStreamWriter which flushes the buffer from the destructor if > > there was (1) no Cancel() call and (2) there is no in-flight operation in > > progress. > > > > Let me know what you prefer. > Either way should be fine. > Please choose the way you think which should makes the sense the best. > If you add a check to the dtor, it should be CHECK instead of DCHECK because > losing users' data is a fatal failure. I was told that DCHECK should be used for programmer errors, which must not happen unless there is a bug in code. And I believe not calling Flush() here is a programmer error. Crashing the browser may actually make things worse. Instead of losing contents of one file, we would interrupt all other ongoing operations and possibly lose more data. Let's go with the second solution then. Flushing the buffers from the destructor. > > > > > Apparently a class which owns file_system_provider::FileStreamWriter is not > a > > > good way to implement this feature. > > > There are other places where this feature can creep in without bloating > > > FileStreamWriter and without dealing with the empty Flush() implementation. > > > (e.g. Inside OperationRunner, a class which owns OperationRunner, a class > > which > > > is owned by OperationRunner, or etc.) > > > > I'm not sure if I understand the problem with the empty Flush() > implementation. > Letting BufferingFileStreamWriter call > file_system_provider::FileStreamWriter::Flush() adds nothing good to the > product. > It just makes the code complicated. I agree it adds some lines of code, but the extra code is trivial. It's just forwarding the Flush(). Calling fsp::FSW::Flush() from fsp::BFSW makes us not depend on the implementation of fsp::FSW. We don't care what's the implementation of fsp::FSW. If someone adds logic fsp::FSW::Flush() then fsp::BFSW will still work properly. I think blindly forwarding the Flush() call is just easier than adding assumption and later maintaining it while code changes. But I can simply remove this call. The side effect is that now fsp::BufferingFileStreamWriter needs one more assumption on fsp::FileStreamWriter (that it doesn't need to flush). If someone adds logic to fsp::FileStreamWriter::Flush(), then BufferingFileStreamWriter will never call it and we may lose users data. Let me know if you want me to remove this call. > > What's wrong with it that it requires changing the entire buffering logic > > upside-down? As for implementing the buffering logic in OperationRunner, I > don't > > want to end up on having the buffering logic for reading in the fsp > > implementation, but buffering for writing in the class shared among *all* > > backends. > I cannot understand what is said here. > Why changing file_system_provider::FileStreamWriter::OperationRunner results in > affecting all fileapi backend implementations? Ah my bad, I thought you're talking about storage::FileSystem*OperationRunner*, which completely didn't make sense.
On 2014/09/04 12:12:42, mtomasz wrote: > On 2014/09/04 11:34:36, hashimoto wrote: > > On 2014/09/03 14:06:30, mtomasz wrote: > > > > > ShouldFlushOnWriteCompletion() disables flushing on completion for > > sandboxed > > > > > backends only. By default flushing on completion is enabled for all > other > > > > > backends. > > > > > FSP backend always get a flush on completion, and it's a documented > > > > requirement > > > > > of the BufferingFileStreamWriter. > > > > It's meaningless to add a documentation to the header of your > > FileStreamWriter > > > > subclass because it's used with a pointer to the storage::FileStreamWriter > > > > interface. > > > > If you want to force user code to call Flush(), the documentation should > be > > > > added to the interface (webkit/browser/fileapi/file_stream_writer.h). > > > > > It's tested. If there is no flushing, then browser tests will fail. > > > > I don't think it's possible to maintain tests which cover all use cases of > > > your > > > > FileStreamWriter. > > > > Other people can freely add a new code which uses > storage::FileStreamWriter > > > > without consulting you, and the code may lack Flush() call and still work > > > nicely > > > > with file systems backed by the native filesystem. > > > > > > Isn't the purpose of Flush() to flush buffers? That's exactly what my > > > implementation is doing. If you don't call Flush() then there is no > guarantee > > > that bytes written to that FileStreamWriter contents are committed. And > that's > > > what happens here. > > > > > > That's true that for LocalFileStreamWriter, we're flushing automatically, > but > > > there is no such requirement in the storage::FileStreamWriter interface. > > > > > > There is already a comment: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/webkit/browser/fil... > > > "Flushes the data written so far." > > > > > > That sounds pretty obvious. But I can add a comment, that if Flush() is not > > > called, then the written contents may not be committed. > > > > > > As for adding new code which calls FileStreamWriter, how about adding a > DCHECK > > > in fsp::BufferingFileStreamWriter destructor to ensure, that Flush() is > always > > > called? > > > > > > Alternatively, I can add extra (just in case) code to > > > fsp::BufferingFileStreamWriter which flushes the buffer from the destructor > if > > > there was (1) no Cancel() call and (2) there is no in-flight operation in > > > progress. > > > > > > Let me know what you prefer. > > Either way should be fine. > > Please choose the way you think which should makes the sense the best. > > If you add a check to the dtor, it should be CHECK instead of DCHECK because > > losing users' data is a fatal failure. > > I was told that DCHECK should be used for programmer errors, which must not > happen unless there is a bug in code. > And I believe not calling Flush() here is a programmer error. Crashing the > browser may actually make things worse. Instead of losing contents of one file, > we would interrupt all other ongoing operations and possibly lose more data. I think you shouldn't worry about it because crashes in dev/beta are much better than a bug going to the stable uncaught. > > Let's go with the second solution then. Flushing the buffers from the > destructor. Sounds fine. > > > > > > > > Apparently a class which owns file_system_provider::FileStreamWriter is > not > > a > > > > good way to implement this feature. > > > > There are other places where this feature can creep in without bloating > > > > FileStreamWriter and without dealing with the empty Flush() > implementation. > > > > (e.g. Inside OperationRunner, a class which owns OperationRunner, a class > > > which > > > > is owned by OperationRunner, or etc.) > > > > > > I'm not sure if I understand the problem with the empty Flush() > > implementation. > > Letting BufferingFileStreamWriter call > > file_system_provider::FileStreamWriter::Flush() adds nothing good to the > > product. > > It just makes the code complicated. > > I agree it adds some lines of code, but the extra code is trivial. It's just > forwarding the Flush(). > > Calling fsp::FSW::Flush() from fsp::BFSW makes us not depend on the > implementation of fsp::FSW. We don't care what's the implementation of fsp::FSW. > If someone adds logic fsp::FSW::Flush() then fsp::BFSW will still work properly. Why anyone wants to implement buffering/flush logic in two places (FSW::Flush and BFSW::Flush)? What I am suggesting here is implementing the logic as FSW::Flush so there is no need to worry about buffering logic fragmented in two places. (this doesn't necessarily mean bloating FSW itself. the buffering logic can be split into separate class like OperationRunner) What is the benefit of implementing this logic outside of FSW? > I think blindly forwarding the Flush() call is just easier than adding > assumption and later maintaining it while code changes. > > But I can simply remove this call. The side effect is that now > fsp::BufferingFileStreamWriter needs one more assumption on > fsp::FileStreamWriter (that it doesn't need to flush). If someone adds logic to > fsp::FileStreamWriter::Flush(), then BufferingFileStreamWriter will never call > it and we may lose users data. > > Let me know if you want me to remove this call. > > > > What's wrong with it that it requires changing the entire buffering logic > > > upside-down? As for implementing the buffering logic in OperationRunner, I > > don't > > > want to end up on having the buffering logic for reading in the fsp > > > implementation, but buffering for writing in the class shared among *all* > > > backends. > > I cannot understand what is said here. > > Why changing file_system_provider::FileStreamWriter::OperationRunner results > in > > affecting all fileapi backend implementations? > > Ah my bad, I thought you're talking about storage::FileSystem*OperationRunner*, > which completely didn't make sense.
(cut) > Why anyone wants to implement buffering/flush logic in two places (FSW::Flush > and BFSW::Flush)? FSW::Flushing would simply emit an event to a providing extension. While BFSW::Flush works on buffers, and doesn't depend on FSP API. > What I am suggesting here is implementing the logic as FSW::Flush so there is no > need to worry about buffering logic fragmented in two places. > (this doesn't necessarily mean bloating FSW itself. the buffering logic can be > split into separate class like OperationRunner) > OK, I understand your point of view. You want to move the buffering to FSW (or its classes), while I want to keep things separate. Let me tell you why. fsp::FSW is a thin wrapper between fileapi::FSW and fsp::ProvidedFileSystem. It's responsibility is to be a simple bridge between these two classes. fsp::FSW::OperationRunner performs such conversion. Saying that, buffering is not the responsibility of fileapi::FSW. I could add it, but I'd prefer to have two simple classes rather than one complex one. This pattern is commonly used eg. in Java: java.io.FileOutputStream extends java.io.OutputStream java.io.BufferedOutputStream extends java.io.OutputStream To write without buffering you call: OutputStream outputStream = new java.io.FileOutputStream(...) If you want buffering you do: OutputStream outputStream = new java.io.BufferedOutputStream(new java.io.FileOutputStream(...)); In this patch: java.io.FileOutputStream is file_system_provider::FileStreamWriter java.io.BufferedOutputtream is file_system_provider::BufferedFileStreamWriter > What is the benefit of implementing this logic outside of FSW? - Having two simple classes instead of one complex by separating responsibilities. - Simpler testing. Tests for buffering do not rely on FSP code. Tests for communication with FSP do not rely on buffering. - Consistency with FileStreamReader + BufferingFileStreamReader. - If IPC is optimized in the future and/or small buffer sizes are not a problem anymore, then we can simply remove buffering logic, by deleting BFSW and using FSW directly (no other change necessary).
Regarding the flush in the dtor, I thought it would work but now I think it won't because extensions can handle nothing during the system shutdown. I think we can do nothing but just CHECK for this. On 2014/09/09 05:48:59, mtomasz wrote: > (cut) > > Why anyone wants to implement buffering/flush logic in two places (FSW::Flush > > and BFSW::Flush)? > > FSW::Flushing would simply emit an event to a providing extension. While > BFSW::Flush works on buffers, and doesn't depend on FSP API. > > > What I am suggesting here is implementing the logic as FSW::Flush so there is > no > > need to worry about buffering logic fragmented in two places. > > (this doesn't necessarily mean bloating FSW itself. the buffering logic can be > > split into separate class like OperationRunner) > > > > OK, I understand your point of view. You want to move the buffering to FSW (or > its classes), while I want to keep things separate. Let me tell you why. > > fsp::FSW is a thin wrapper between fileapi::FSW and fsp::ProvidedFileSystem. > It's responsibility is to be a simple bridge between these two classes. > fsp::FSW::OperationRunner performs such conversion. Saying that, buffering is > not the responsibility of fileapi::FSW. I could add it, but I'd prefer to have > two simple classes rather than one complex one. > > This pattern is commonly used eg. in Java: > > java.io.FileOutputStream extends java.io.OutputStream > java.io.BufferedOutputStream extends java.io.OutputStream > > To write without buffering you call: > OutputStream outputStream = new java.io.FileOutputStream(...) > > If you want buffering you do: > OutputStream outputStream = new java.io.BufferedOutputStream(new > java.io.FileOutputStream(...)); > > In this patch: > java.io.FileOutputStream is file_system_provider::FileStreamWriter > java.io.BufferedOutputtream is file_system_provider::BufferedFileStreamWriter > > > What is the benefit of implementing this logic outside of FSW? > > - Having two simple classes instead of one complex by separating > responsibilities. > - Simpler testing. Tests for buffering do not rely on FSP code. Tests for > communication with FSP do not rely on buffering. > - Consistency with FileStreamReader + BufferingFileStreamReader. > - If IPC is optimized in the future and/or small buffer sizes are not a problem > anymore, then we can simply remove buffering logic, by deleting BFSW and using > FSW directly (no other change necessary). Thank you for explaining your intention clearly. These merits can also be achieved with a class which wraps OperationRunner (like BufferedFileStreamWriter is wrapping FileStreamWriter), but this code might make sense if there is a plan to implement something inside FileStreamWriter::Flush(). Is there any plan to add Flush() event to the file system provider API to allow provider extensions to buffer written data?
On 2014/09/10 07:56:15, hashimoto wrote: (cut) > Regarding the flush in the dtor, > I thought it would work but now I think it won't because extensions can handle > nothing during the system shutdown. > I think we can do nothing but just CHECK for this. Added a CHECK. (cut) > Thank you for explaining your intention clearly. > These merits can also be achieved with a class which wraps OperationRunner (like > BufferedFileStreamWriter is wrapping FileStreamWriter), > but this code might make sense if there is a plan to implement something inside > FileStreamWriter::Flush(). > Is there any plan to add Flush() event to the file system provider API to allow > provider extensions to buffer written data? I'm still thinking about it. For local providers 512KB chunk size works very well and there is not much difference after increasing the buffer size any further. For Dropbox 512KB chunk size pretty well, however the documentation suggests a chunk of 4MB in size (https://www.dropbox.com/developers/core/docs). If we created an onFlushRequested event, then extensions could perform further optimizations if needed.
Where did you put the CHECK to detect data loss? https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc (right): https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:76: return file_stream_writer_->Cancel(callback); Could you add a comment to describe how this implementation is justified? IIUC this implementation is correct as long as this class does not perform any asynchronous operations other than performed by |file_stream_writer_|. https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:91: int offset, It's ambiguous whether |offset| is for the argument buffer or the intermediate buffer. How about passing char* instead of |buffer| and |offset| so that there is no need to pass the offset? https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:93: DCHECK_GE(intermediate_buffer_length_, offset + length + buffered_bytes_); What does the value of "offset + length + buffered_bytes_" correspond to? https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:139: DCHECK_EQ(net::OK, result); nit: Why are you checking this here and in the method below? Can Flush() return any non-negative value other than net::OK? https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.h (right): https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.h:27: // The underlying inner file stream writer *must not* return any values nit: Sometimes the writer is referred as "inner" writer, but sometimes as "internal". Please stick to one word. https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.h:31: // Flush() must be called on completion to ensure that all bytes are written This description should be put in storage::FileStreamWriter's header. This class is used mainly with storage::FileStreamWriter*, not with BufferingFileStreamWriter*. So the must-be-flushed contract should be forced to all users of FileStreamWriter. https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc (right): https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc:66: callback.Run(net::OK); nit: Use PostTask? https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc:74: callback.Run(net::OK); ditto. https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc:104: scoped_refptr<net::StringIOBuffer> buffer( How about having this buffer and a StringBuffer made with kLongTextToWrite as members of the test fixture? The same buffers are constructed in many places. https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc:317: EXPECT_EQ(net::OK, cancel_log[0]); How about counting how many times the fake's Cancel() method was called?
PTAL. Thanks. https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc (right): https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:76: return file_stream_writer_->Cancel(callback); On 2014/09/11 07:52:16, hashimoto wrote: > Could you add a comment to describe how this implementation is justified? Done. > IIUC this implementation is correct as long as this class does not perform any > asynchronous operations other than performed by |file_stream_writer_|. It is valid to call Cancel() if there is no ongoing in-flight operation on file_stream_writer_. Saying that, we don't have any PostTasks in buffering_file_stream_writer.cc, so we will not get any Cancel() invocation if there is no in-flight operation on file_stream_writer_. https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:91: int offset, On 2014/09/11 07:52:16, hashimoto wrote: > It's ambiguous whether |offset| is for the argument buffer or the intermediate > buffer. It's clearly described in the header file. > How about passing char* instead of |buffer| and |offset| so that there is no > need to pass the offset? I'd like to keep buffer as net::IOBuffer as I want to have all the ugly pointer arithmetics in one place. I renamed the |offset| to |buffer_offset| and |length| to |buffer_length|. https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:93: DCHECK_GE(intermediate_buffer_length_, offset + length + buffered_bytes_); On 2014/09/11 07:52:16, hashimoto wrote: > What does the value of "offset + length + buffered_bytes_" correspond to? Position of the last byte to be written. This DCHECK checks if we are not writing more than size of the intermediate buffer. https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:139: DCHECK_EQ(net::OK, result); On 2014/09/11 07:52:16, hashimoto wrote: > nit: Why are you checking this here and in the method below? Can Flush() return > any non-negative value other than net::OK? It's not possible, but since FlushIntermediateBuffer internally calls file_stream_writer_->Write, this DCHECK makes sure that the |result| is properly replaced with net::OK in #124. Hence I think it's worth keeping it, as it's only a DCHECK. https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.h (right): https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.h:27: // The underlying inner file stream writer *must not* return any values On 2014/09/11 07:52:16, hashimoto wrote: > nit: Sometimes the writer is referred as "inner" writer, but sometimes as > "internal". > Please stick to one word. Done. https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.h:31: // Flush() must be called on completion to ensure that all bytes are written On 2014/09/11 07:52:16, hashimoto wrote: > This description should be put in storage::FileStreamWriter's header. > > This class is used mainly with storage::FileStreamWriter*, not with > BufferingFileStreamWriter*. > So the must-be-flushed contract should be forced to all users of > FileStreamWriter. I tried a CHECK() and I think this is not the best idea. We would need always to call Flush() after writing some data to a writer, including on shutdown. Currently we *do not* call Flush() on shutdown in CopyOrMoveOperationDelegate. As a result, we'd have a CHECK crash. To avoid, we could add Flush() calls in the destructor, however this is iffy, since Flush() may be asynchronous. Especially for this fsp::BufferingFileStreamWriter implementation, flushing is asynchronous, so it basically can't complete, and that data will be lost. Yet another issue is tests. If we fail on ASSERT, then the test case will abort without calling Flush(). We'd crash. To avoid it we would have to create a wrapper over BufferingFileStreamWriter, which would call Flush() from the destructor. It is overcomplicated. Instead, I went in the end with with the second solution we discussed before. I call fsp::BufferingFileStreamWriter::FlushIntermediateBuffer() from the destructor of fsp::BufferingFileStreamWriter. As a result, we don't need to add a special assumption to storage::FileStreamWriter about flushing. The data will be always automatically flushed when a fsp::BufferingFileStreamWriter object is destroyed. As for shutdown, we'd lose data anyway, since it's impossible to flush at the shutdown time. https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc (right): https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc:66: callback.Run(net::OK); On 2014/09/11 07:52:17, hashimoto wrote: > nit: Use PostTask? Done. https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc:74: callback.Run(net::OK); On 2014/09/11 07:52:16, hashimoto wrote: > ditto. Done. https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc:104: scoped_refptr<net::StringIOBuffer> buffer( On 2014/09/11 07:52:17, hashimoto wrote: > How about having this buffer and a StringBuffer made with kLongTextToWrite as > members of the test fixture? > The same buffers are constructed in many places. Good idea. Done. https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc:317: EXPECT_EQ(net::OK, cancel_log[0]); On 2014/09/11 07:52:17, hashimoto wrote: > How about counting how many times the fake's Cancel() method was called? Done.
On 2014/09/17 04:59:41, mtomasz wrote: > PTAL. Thanks. > > https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... > File > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc > (right): > > https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:76: > return file_stream_writer_->Cancel(callback); > On 2014/09/11 07:52:16, hashimoto wrote: > > Could you add a comment to describe how this implementation is justified? > Done. > > > IIUC this implementation is correct as long as this class does not perform any > > asynchronous operations other than performed by |file_stream_writer_|. > It is valid to call Cancel() if there is no ongoing in-flight operation on > file_stream_writer_. Saying that, we don't have any PostTasks in > buffering_file_stream_writer.cc, so we will not get any Cancel() invocation if > there is no in-flight operation on file_stream_writer_. > > https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:91: > int offset, > On 2014/09/11 07:52:16, hashimoto wrote: > > It's ambiguous whether |offset| is for the argument buffer or the intermediate > > buffer. > It's clearly described in the header file. > > > How about passing char* instead of |buffer| and |offset| so that there is no > > need to pass the offset? > > I'd like to keep buffer as net::IOBuffer as I want to have all the ugly pointer > arithmetics in one place. I renamed the |offset| to |buffer_offset| and |length| > to |buffer_length|. > > https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:93: > DCHECK_GE(intermediate_buffer_length_, offset + length + buffered_bytes_); > On 2014/09/11 07:52:16, hashimoto wrote: > > What does the value of "offset + length + buffered_bytes_" correspond to? > > Position of the last byte to be written. This DCHECK checks if we are not > writing more than size of the intermediate buffer. > > https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:139: > DCHECK_EQ(net::OK, result); > On 2014/09/11 07:52:16, hashimoto wrote: > > nit: Why are you checking this here and in the method below? Can Flush() > return > > any non-negative value other than net::OK? > > It's not possible, but since FlushIntermediateBuffer internally calls > file_stream_writer_->Write, this DCHECK makes sure that the |result| is properly > replaced with net::OK in #124. Hence I think it's worth keeping it, as it's only > a DCHECK. > > https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... > File > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.h > (right): > > https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.h:27: > // The underlying inner file stream writer *must not* return any values > On 2014/09/11 07:52:16, hashimoto wrote: > > nit: Sometimes the writer is referred as "inner" writer, but sometimes as > > "internal". > > Please stick to one word. > > Done. > > https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.h:31: > // Flush() must be called on completion to ensure that all bytes are written > On 2014/09/11 07:52:16, hashimoto wrote: > > This description should be put in storage::FileStreamWriter's header. > > > > This class is used mainly with storage::FileStreamWriter*, not with > > BufferingFileStreamWriter*. > > So the must-be-flushed contract should be forced to all users of > > FileStreamWriter. > > I tried a CHECK() and I think this is not the best idea. We would need always to > call Flush() after writing some data to a writer, including on shutdown. > > Currently we *do not* call Flush() on shutdown in CopyOrMoveOperationDelegate. > As a result, we'd have a CHECK crash. To avoid, we could add Flush() calls in > the destructor, however this is iffy, since Flush() may be asynchronous. > Especially for this fsp::BufferingFileStreamWriter implementation, flushing is > asynchronous, so it basically can't complete, and that data will be lost. > > Yet another issue is tests. If we fail on ASSERT, then the test case will abort > without calling Flush(). We'd crash. To avoid it we would have to create a > wrapper over BufferingFileStreamWriter, which would call Flush() from the > destructor. It is overcomplicated. > > Instead, I went in the end with with the second solution we discussed before. I > call fsp::BufferingFileStreamWriter::FlushIntermediateBuffer() from the > destructor of fsp::BufferingFileStreamWriter. As a result, we don't need to add > a special assumption to storage::FileStreamWriter about flushing. The data will > be always automatically flushed when a fsp::BufferingFileStreamWriter object is > destroyed. As for shutdown, we'd lose data anyway, since it's impossible to > flush at the shutdown time. > > https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... > File > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc > (right): > > https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc:66: > callback.Run(net::OK); > On 2014/09/11 07:52:17, hashimoto wrote: > > nit: Use PostTask? > > Done. > > https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc:74: > callback.Run(net::OK); > On 2014/09/11 07:52:16, hashimoto wrote: > > ditto. > > Done. > > https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc:104: > scoped_refptr<net::StringIOBuffer> buffer( > On 2014/09/11 07:52:17, hashimoto wrote: > > How about having this buffer and a StringBuffer made with kLongTextToWrite as > > members of the test fixture? > > The same buffers are constructed in many places. > > Good idea. Done. > > https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc:317: > EXPECT_EQ(net::OK, cancel_log[0]); > On 2014/09/11 07:52:17, hashimoto wrote: > > How about counting how many times the fake's Cancel() method was called? > > Done. @hashimoto: Ping. Thanks.
https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc (right): https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:93: DCHECK_GE(intermediate_buffer_length_, offset + length + buffered_bytes_); On 2014/09/17 04:59:40, mtomasz wrote: > On 2014/09/11 07:52:16, hashimoto wrote: > > What does the value of "offset + length + buffered_bytes_" correspond to? > > Position of the last byte to be written. This DCHECK checks if we are not > writing more than size of the intermediate buffer. Then shouldn't this be buffer_length + buffered_bytes_? https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:139: DCHECK_EQ(net::OK, result); On 2014/09/17 04:59:40, mtomasz wrote: > On 2014/09/11 07:52:16, hashimoto wrote: > > nit: Why are you checking this here and in the method below? Can Flush() > return > > any non-negative value other than net::OK? > > It's not possible, but since FlushIntermediateBuffer internally calls > file_stream_writer_->Write, this DCHECK makes sure that the |result| is properly > replaced with net::OK in #124. Hence I think it's worth keeping it, as it's only > a DCHECK. I still don't see any reason to check |result| which is never used after the "result < 0" conditional above. Anyways, it seems harmless as long as this is DCHECK. https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.h (right): https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.h:31: // Flush() must be called on completion to ensure that all bytes are written On 2014/09/17 04:59:40, mtomasz wrote: > On 2014/09/11 07:52:16, hashimoto wrote: > > This description should be put in storage::FileStreamWriter's header. > > > > This class is used mainly with storage::FileStreamWriter*, not with > > BufferingFileStreamWriter*. > > So the must-be-flushed contract should be forced to all users of > > FileStreamWriter. > > I tried a CHECK() and I think this is not the best idea. We would need always to > call Flush() after writing some data to a writer, including on shutdown. > > Currently we *do not* call Flush() on shutdown in CopyOrMoveOperationDelegate. > As a result, we'd have a CHECK crash. To avoid, we could add Flush() calls in > the destructor, however this is iffy, since Flush() may be asynchronous. > Especially for this fsp::BufferingFileStreamWriter implementation, flushing is > asynchronous, so it basically can't complete, and that data will be lost. > > Yet another issue is tests. If we fail on ASSERT, then the test case will abort > without calling Flush(). We'd crash. To avoid it we would have to create a > wrapper over BufferingFileStreamWriter, which would call Flush() from the > destructor. It is overcomplicated. You can replace CHECK with something harmless like LOG(ERROR) with #if defined(UNIT_TEST). > > Instead, I went in the end with with the second solution we discussed before. I > call fsp::BufferingFileStreamWriter::FlushIntermediateBuffer() from the > destructor of fsp::BufferingFileStreamWriter. As a result, we don't need to add > a special assumption to storage::FileStreamWriter about flushing. The data will > be always automatically flushed when a fsp::BufferingFileStreamWriter object is > destroyed. As for shutdown, we'd lose data anyway, since it's impossible to > flush at the shutdown time. Calling Flush() in the dtor is not helpful especially when the system is shutdown just after the Flush() call. (it might result in a tricky race bug) Can't we do something to distinguish intended data loss (e.g. shutdown during copy) from a bug where the developer forgets to call Flush()? (e.g. Calling Flush() in the dtor of CopyOrMoveOperationDelegate())
https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc (right): https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:93: DCHECK_GE(intermediate_buffer_length_, offset + length + buffered_bytes_); On 2014/09/22 12:10:31, hashimoto wrote: > On 2014/09/17 04:59:40, mtomasz wrote: > > On 2014/09/11 07:52:16, hashimoto wrote: > > > What does the value of "offset + length + buffered_bytes_" correspond to? > > > > Position of the last byte to be written. This DCHECK checks if we are not > > writing more than size of the intermediate buffer. > > Then shouldn't this be buffer_length + buffered_bytes_? That's right. Fixed. https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.cc:139: DCHECK_EQ(net::OK, result); On 2014/09/22 12:10:31, hashimoto wrote: > On 2014/09/17 04:59:40, mtomasz wrote: > > On 2014/09/11 07:52:16, hashimoto wrote: > > > nit: Why are you checking this here and in the method below? Can Flush() > > return > > > any non-negative value other than net::OK? > > > > It's not possible, but since FlushIntermediateBuffer internally calls > > file_stream_writer_->Write, this DCHECK makes sure that the |result| is > properly > > replaced with net::OK in #124. Hence I think it's worth keeping it, as it's > only > > a DCHECK. > I still don't see any reason to check |result| which is never used after the > "result < 0" conditional above. Yeah, seems not really helping much here. Removed. > Anyways, it seems harmless as long as this is DCHECK. https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.h (right): https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.h:31: // Flush() must be called on completion to ensure that all bytes are written On 2014/09/22 12:10:31, hashimoto wrote: > On 2014/09/17 04:59:40, mtomasz wrote: > > On 2014/09/11 07:52:16, hashimoto wrote: > > > This description should be put in storage::FileStreamWriter's header. > > > > > > This class is used mainly with storage::FileStreamWriter*, not with > > > BufferingFileStreamWriter*. > > > So the must-be-flushed contract should be forced to all users of > > > FileStreamWriter. > > > > I tried a CHECK() and I think this is not the best idea. We would need always > to > > call Flush() after writing some data to a writer, including on shutdown. > > > > Currently we *do not* call Flush() on shutdown in CopyOrMoveOperationDelegate. > > As a result, we'd have a CHECK crash. To avoid, we could add Flush() calls in > > the destructor, however this is iffy, since Flush() may be asynchronous. > > Especially for this fsp::BufferingFileStreamWriter implementation, flushing is > > asynchronous, so it basically can't complete, and that data will be lost. > > > > Yet another issue is tests. If we fail on ASSERT, then the test case will > abort > > without calling Flush(). We'd crash. To avoid it we would have to create a > > wrapper over BufferingFileStreamWriter, which would call Flush() from the > > destructor. It is overcomplicated. > You can replace CHECK with something harmless like LOG(ERROR) with #if > defined(UNIT_TEST). That's possible, but I think flushing from the destructor is just simpler. We don't need to add a requirement to Flush() after copying which you were against. So there is no way we have a programming error by not calling Flush() you mentioned later. > > > > Instead, I went in the end with with the second solution we discussed before. > I > > call fsp::BufferingFileStreamWriter::FlushIntermediateBuffer() from the > > destructor of fsp::BufferingFileStreamWriter. As a result, we don't need to > add > > a special assumption to storage::FileStreamWriter about flushing. The data > will > > be always automatically flushed when a fsp::BufferingFileStreamWriter object > is > > destroyed. As for shutdown, we'd lose data anyway, since it's impossible to > > flush at the shutdown time. > Calling Flush() in the dtor is not helpful especially when the system is > shutdown just after the Flush() call. (it might result in a tricky race bug) What race do you have in mind? I fixed a possible race in the latest patchset. > Can't we do something to distinguish intended data loss (e.g. shutdown during > copy) from a bug where the developer forgets to call Flush()? (e.g. Calling > Flush() in the dtor of CopyOrMoveOperationDelegate()) There is no need to call Flush() anymore because adding the flushing logic to the destructor. So we will only lose data on shutdown, which as you said is intended.
https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.h (right): https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.h:31: // Flush() must be called on completion to ensure that all bytes are written On 2014/09/24 01:45:31, mtomasz wrote: > On 2014/09/22 12:10:31, hashimoto wrote: > > On 2014/09/17 04:59:40, mtomasz wrote: > > > On 2014/09/11 07:52:16, hashimoto wrote: > > > > This description should be put in storage::FileStreamWriter's header. > > > > > > > > This class is used mainly with storage::FileStreamWriter*, not with > > > > BufferingFileStreamWriter*. > > > > So the must-be-flushed contract should be forced to all users of > > > > FileStreamWriter. > > > > > > I tried a CHECK() and I think this is not the best idea. We would need > always > > to > > > call Flush() after writing some data to a writer, including on shutdown. > > > > > > Currently we *do not* call Flush() on shutdown in > CopyOrMoveOperationDelegate. > > > As a result, we'd have a CHECK crash. To avoid, we could add Flush() calls > in > > > the destructor, however this is iffy, since Flush() may be asynchronous. > > > Especially for this fsp::BufferingFileStreamWriter implementation, flushing > is > > > asynchronous, so it basically can't complete, and that data will be lost. > > > > > > Yet another issue is tests. If we fail on ASSERT, then the test case will > > abort > > > without calling Flush(). We'd crash. To avoid it we would have to create a > > > wrapper over BufferingFileStreamWriter, which would call Flush() from the > > > destructor. It is overcomplicated. > > You can replace CHECK with something harmless like LOG(ERROR) with #if > > defined(UNIT_TEST). > > That's possible, but I think flushing from the destructor is just simpler. We > don't need to add a requirement to Flush() after copying which you were against. > So there is no way we have a programming error by not calling Flush() you > mentioned later. > > > > > > > Instead, I went in the end with with the second solution we discussed > before. > > I > > > call fsp::BufferingFileStreamWriter::FlushIntermediateBuffer() from the > > > destructor of fsp::BufferingFileStreamWriter. As a result, we don't need to > > add > > > a special assumption to storage::FileStreamWriter about flushing. The data > > will > > > be always automatically flushed when a fsp::BufferingFileStreamWriter object > > is > > > destroyed. As for shutdown, we'd lose data anyway, since it's impossible to > > > flush at the shutdown time. > > Calling Flush() in the dtor is not helpful especially when the system is > > shutdown just after the Flush() call. (it might result in a tricky race bug) > > What race do you have in mind? I fixed a possible race in the latest patchset. Flush() in the dtor requires the provider process to stay alive for a sufficient amount of time after it gets called. I'm concerned about cases like the following: 1. A file operation finishes successfully, with Flush() being called in the dtor. 2. Wait for a moment. 3. User turns off the device. If Flush() completes during the step #2, there is no data loss. But if Flush() cannot finish during #2, the data will be lost. I guess this should result in a bug which is quite difficult to reproduce. > > > Can't we do something to distinguish intended data loss (e.g. shutdown during > > copy) from a bug where the developer forgets to call Flush()? (e.g. Calling > > Flush() in the dtor of CopyOrMoveOperationDelegate()) > > There is no need to call Flush() anymore because adding the flushing logic to > the destructor. So we will only lose data on shutdown, which as you said is > intended.
On 2014/09/26 07:05:55, hashimoto wrote: > https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... > File > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.h > (right): > > https://codereview.chromium.org/502973005/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.h:31: > // Flush() must be called on completion to ensure that all bytes are written > On 2014/09/24 01:45:31, mtomasz wrote: > > On 2014/09/22 12:10:31, hashimoto wrote: > > > On 2014/09/17 04:59:40, mtomasz wrote: > > > > On 2014/09/11 07:52:16, hashimoto wrote: > > > > > This description should be put in storage::FileStreamWriter's header. > > > > > > > > > > This class is used mainly with storage::FileStreamWriter*, not with > > > > > BufferingFileStreamWriter*. > > > > > So the must-be-flushed contract should be forced to all users of > > > > > FileStreamWriter. > > > > > > > > I tried a CHECK() and I think this is not the best idea. We would need > > always > > > to > > > > call Flush() after writing some data to a writer, including on shutdown. > > > > > > > > Currently we *do not* call Flush() on shutdown in > > CopyOrMoveOperationDelegate. > > > > As a result, we'd have a CHECK crash. To avoid, we could add Flush() calls > > in > > > > the destructor, however this is iffy, since Flush() may be asynchronous. > > > > Especially for this fsp::BufferingFileStreamWriter implementation, > flushing > > is > > > > asynchronous, so it basically can't complete, and that data will be lost. > > > > > > > > Yet another issue is tests. If we fail on ASSERT, then the test case will > > > abort > > > > without calling Flush(). We'd crash. To avoid it we would have to create a > > > > wrapper over BufferingFileStreamWriter, which would call Flush() from the > > > > destructor. It is overcomplicated. > > > You can replace CHECK with something harmless like LOG(ERROR) with #if > > > defined(UNIT_TEST). > > > > That's possible, but I think flushing from the destructor is just simpler. We > > don't need to add a requirement to Flush() after copying which you were > against. > > So there is no way we have a programming error by not calling Flush() you > > mentioned later. > > > > > > > > > > Instead, I went in the end with with the second solution we discussed > > before. > > > I > > > > call fsp::BufferingFileStreamWriter::FlushIntermediateBuffer() from the > > > > destructor of fsp::BufferingFileStreamWriter. As a result, we don't need > to > > > add > > > > a special assumption to storage::FileStreamWriter about flushing. The data > > > will > > > > be always automatically flushed when a fsp::BufferingFileStreamWriter > object > > > is > > > > destroyed. As for shutdown, we'd lose data anyway, since it's impossible > to > > > > flush at the shutdown time. > > > Calling Flush() in the dtor is not helpful especially when the system is > > > shutdown just after the Flush() call. (it might result in a tricky race bug) > > > > What race do you have in mind? I fixed a possible race in the latest patchset. > Flush() in the dtor requires the provider process to stay alive for a sufficient > amount of time after it gets called. > > I'm concerned about cases like the following: > 1. A file operation finishes successfully, with Flush() being called in the > dtor. > 2. Wait for a moment. > 3. User turns off the device. > If Flush() completes during the step #2, there is no data loss. > But if Flush() cannot finish during #2, the data will be lost. > I guess this should result in a bug which is quite difficult to reproduce. That's a good point. Let's go back to the CHECK version, but replacing it with LOG(ERROR).
mtomasz@chromium.org changed reviewers: + tzik@chromium.org
@hashimoto: I reverted to the previous solution. PTAL. @tzik: PTAL at storage/browser/fileapi/file_stream_writer.h. Thanks.
On 2014/10/07 03:12:14, mtomasz wrote: > @hashimoto: I reverted to the previous solution. PTAL. > @tzik: PTAL at storage/browser/fileapi/file_stream_writer.h. Thanks. Looks good overall. Is there any chance for the user to write a small chunk of data to the stream, and leave it without closing it? If so, can we write out the buffered data periodically?
On 2014/10/07 05:11:42, tzik wrote: > On 2014/10/07 03:12:14, mtomasz wrote: > > @hashimoto: I reverted to the previous solution. PTAL. > > @tzik: PTAL at storage/browser/fileapi/file_stream_writer.h. Thanks. > > Looks good overall. > > Is there any chance for the user to write a small chunk of data to the stream, > and leave it without closing it? > If so, can we write out the buffered data periodically? It can't happen that we write all bytes user desired and never closed the stream. But it may happen that copying operation is slow, and there are long breaks between calling FileStreamWriter::Write(). If a crash/hard-reboot/power-off happens during such long breaks, then we will lose some data. But even if we flush periodically in time, we may lose data in such cases. I'm not sure if it's worth adding this logic.
lgtm
On 2014/10/07 07:55:40, tzik wrote: > lgtm @hashimoto: Ping.
On 2014/10/10 06:18:41, mtomasz wrote: > On 2014/10/07 07:55:40, tzik wrote: > > lgtm > > @hashimoto: Ping. @hashimoto: Ping?
lgtm please align to the latest coding style. https://codereview.chromium.org/502973005/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.h (right): https://codereview.chromium.org/502973005/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.h:42: virtual int Flush(const net::CompletionCallback& callback) OVERRIDE; nit: override should be small-cased now. https://codereview.chromium.org/502973005/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc (right): https://codereview.chromium.org/502973005/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc:56: const net::CompletionCallback& callback) OVERRIDE { nit: override https://codereview.chromium.org/502973005/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc:118: scoped_ptr<storage::FileStreamWriter>(new FakeFileStreamWriter( nit: just make_scoped_ptr. https://codereview.chromium.org/502973005/diff/160001/storage/browser/fileapi... File storage/browser/fileapi/file_stream_writer.h (right): https://codereview.chromium.org/502973005/diff/160001/storage/browser/fileapi... storage/browser/fileapi/file_stream_writer.h:49: // After the last write, Flush() will be called unless the flushing on How about replacing "will be" with "must be"? IIUC this comment is to force all user code (including code written in future) to call Flush().
https://codereview.chromium.org/502973005/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.h (right): https://codereview.chromium.org/502973005/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.h:42: virtual int Flush(const net::CompletionCallback& callback) OVERRIDE; On 2014/10/16 21:39:03, hashimoto wrote: > nit: override should be small-cased now. Done. https://codereview.chromium.org/502973005/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc (right): https://codereview.chromium.org/502973005/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc:56: const net::CompletionCallback& callback) OVERRIDE { On 2014/10/16 21:39:03, hashimoto wrote: > nit: override Done. https://codereview.chromium.org/502973005/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc:118: scoped_ptr<storage::FileStreamWriter>(new FakeFileStreamWriter( On 2014/10/16 21:39:03, hashimoto wrote: > nit: just make_scoped_ptr. Done. https://codereview.chromium.org/502973005/diff/160001/storage/browser/fileapi... File storage/browser/fileapi/file_stream_writer.h (right): https://codereview.chromium.org/502973005/diff/160001/storage/browser/fileapi... storage/browser/fileapi/file_stream_writer.h:49: // After the last write, Flush() will be called unless the flushing on On 2014/10/16 21:39:03, hashimoto wrote: > How about replacing "will be" with "must be"? > IIUC this comment is to force all user code (including code written in future) > to call Flush(). Done.
The CQ bit was checked by mtomasz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/502973005/190001
Message was sent while issue was closed.
Committed patchset #10 (id:190001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/b75566966d7efcbb5f544f4488bae12ec7aed7c1 Cr-Commit-Position: refs/heads/master@{#300043} |
