Bug 44360

Created attachment 65014 [details] Patch

WebKit/chromium/public/WebFileWriter.h:47 + // at destruction time. Who is going to create and hold the WebFileWriter?

(In reply to comment #2) > WebKit/chromium/public/WebFileWriter.h:47 > + // at destruction time. > Who is going to create and hold the WebFileWriter? It'll be created by WebFileSystem::createFileWriter, and held by the implementation of AsyncFileWriter in WebKit/chromium/src, which will be held by the WebCore/html/FileWriter. I have that implementation [it's pretty thin] ready to post if this API is acceptable. Or I can post it in the other bug if it'd help make this clearer.

Comment on attachment 65014 [details] Patch WebKit/chromium/public/WebFileWriter.h:48 + virtual void setFileWriterClient(WebFileWriterClient *client) = 0; normally client interfaces are set at construction time. so the method used to create a WebFileWriter should be parameterized by a WebFileWriterClient. that way clients are immutable for the lifetime of an object. that helps keep code simpler. any reason not to conform this code to that pattern? WebKit/chromium/public/WebFileWriter.h:50 + virtual void write(const WebString& path, long long position, const WebURL& blobURL) = 0; so a WebFileWriter instance can be used to modify multiple files? is it intended to be used more like a global service then? why not instantiate a WebFileWriter from a file path? WebKit/chromium/public/WebFileWriter.h:52 + virtual void abort() = 0; cancel is the more common verb in the API (see WebURLLoader) WebKit/chromium/public/WebFileWriterClient.h:43 + virtual void didWrite(long long) = 0; please give this 'long long' parameter a name since it isn't obvious what this parameter is used for. WebKit/chromium/public/WebFileWriterClient.h:41 + virtual ~WebFileWriterClient() { } if you do not expect people to delete a WebFileWriterClient through this interface then make this destructor protected. (normally, we don't delete *Client pointers.)

I think it would be helpful to include in this patch the API method used to construct a WebFileWriter.

Created attachment 65363 [details] NOT ready for checkin, but better than the last one Darin, this isn't ready to go yet, and includes pieces that might be better split off, but perhaps you can take a look at it and see if it's closer. I'm not sure where I should delete the WebFileWriterClient if not through the API, so I haven't handled that yet.

Comment on attachment 65363 [details] NOT ready for checkin, but better than the last one WebKit/chromium/public/WebFileSystem.h:108 + virtual void createFileWriter(const WebString& path, WebFileWriterClient*, WebFileSystemCallbacks*) { WEBKIT_ASSERT_NOT_REACHED(); } I would expect this to look like: virtual WebFileWriter* createFileWriter(const WebString& path, WebFileWriterClient*) { ... } WebKit/chromium/public/WebFileWriter.h:44 + WebFileWriter(WebFileWriterClient* client); we should move the client to the implementation class. take a look at how WebURLLoader works for example. WebKit/chromium/public/WebFileWriter.h:48 + virtual void write(long long position, const WebURL& blobURL) = 0; add some documentation about how there can only be one outstanding write or truncate at a time and that cancel is intended to interrupt that.

Created attachment 65516 [details] Rolled in all comments.

Attachment 65516 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit/chromium/public/WebFileSystem.h:107: Use 0 instead of NULL. [readability/null] [5] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 65519 [details] Fixed style error

Comment on attachment 65519 [details] Fixed style error WebKit/chromium/public/WebFileWriter.h:50 + // Cancel will attempt to abort a running write or truncate. if cancel is called, will the client be guaranteed to receive a didFail even if the asynchronous operation actually ran to completion before cancel was called?

(In reply to comment #11) > (From update of attachment 65519 [details]) > WebKit/chromium/public/WebFileWriter.h:50 > + // Cancel will attempt to abort a running write or truncate. > if cancel is called, will the client be guaranteed to receive a didFail even if the asynchronous operation actually ran to completion before cancel was called? We should probably guarantee that--it simplifies things on the client side. However, if the operation has already completed, there's nothing to cancel, so the didFail will be reporting the failure to cancel. We'll have to find the appropriate error code for "I don't know what you're trying to do, so I can't do it". I'll add a comment to that effect.

(In reply to comment #12) > (In reply to comment #11) > > (From update of attachment 65519 [details] [details]) > > WebKit/chromium/public/WebFileWriter.h:50 > > + // Cancel will attempt to abort a running write or truncate. > > if cancel is called, will the client be guaranteed to receive a didFail even if the asynchronous operation actually ran to completion before cancel was called? > > We should probably guarantee that--it simplifies things on the client side. > However, if the operation has already completed, there's nothing to cancel, so the didFail will be reporting the failure to cancel. We'll have to find the appropriate error code for "I don't know what you're trying to do, so I can't do it". > > I'll add a comment to that effect. Sorry, I meant... suppose the async operation has "completed" but the corresponding client method has not been called yet. From the point of view of the WebKit thread, if a consumer calls cancel before it has received a client callback, then the client callback should be didFail(WebFileErrorAbort) even if the operation has finished its work on whatever background thread it is running on.

Created attachment 65582 [details] Added requested comment I've added the comment; no other changes.

Attachment 65582 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit/chromium/public/WebFileSystem.h:107: Use 0 instead of NULL. [readability/null] [5] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 65630 [details] Adjusted as discussed offline I added a "complete" parameter to write(); it seemed cumbersome to add a "didSucceed" method, especially for truncate, which needs only a single call. I added a bunch of explanatory comments as well.

Comment on attachment 65630 [details] Adjusted as discussed offline Fixing attachment type.

(In reply to comment #17) > (From update of attachment 65630 [details]) some minor comments; WebKit/chromium/src/AsyncFileWriterChromium.cpp:56 + m_writer->write(position, WebURL(data->url())); nits: may be good to have ASSERT(m_writer)? WebKit/chromium/src/AsyncFileWriterChromium.h:49 + class AsyncFileWriterChromium : public WebCore::AsyncFileWriter, WebFileWriterClient { WebFileWriterClient is privately inherited - is it intentional? WebKit/chromium/src/AsyncFileWriterChromium.h:67 + OwnPtr<WebFileWriter*> m_writer; OwnPtr<WebFileWriter> m_writer;

Created attachment 65638 [details] Fixed issues Kinuko found All fixed. Thanks!

Comment on attachment 65638 [details] Fixed issues Kinuko found WebKit/chromium/public/WebFileWriterClient.h:48 + // Called if the write or truncate fails, or if it is cancelled before didWrite or didTruncate is called. what does the client call sequence look like if writing completes but cancel is called before didWrite(bytes, true) is called? the comment above for didFail says that didFail will be called if cancel is called before didWrite or didTruncate is called. but what if the system did complete writing the file before that time? i'm concerned that it is complicated for the client to know that if he receives didWrite(bytes, true) that didFail may or may not follow. he would need to keep track of whether or not he called cancel. WebKit/chromium/src/AsyncFileWriterChromium.cpp:43 + AsyncFileWriterChromium::AsyncFileWriterChromium(WebCore::FileWriterClient* client) nit: i recommend putting a 'using namespace WebCore' at the top so you can drop the WebCore:: prefix in this .cpp file

Created attachment 65730 [details] Adjusted comments for behavior and clarity, added namespace decl. I think this adjustment addresses all your concerns. I'll let the comments speak for themselves; if they don't answer all your questions, I should add more.

Comment on attachment 65730 [details] Adjusted comments for behavior and clarity, added namespace decl. drive by comment WebKit/chromium/src/AsyncFileWriterChromium.h:57 + void write(long long position, WebCore::Blob* data); these methods are overrides, maybe use the virtual keyword to help make that more clear

Created attachment 65744 [details] Added virtual hint Added virtual hints, as Michael suggested.

Comment on attachment 65744 [details] Added virtual hint Rejecting patch 65744 from commit-queue. Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Fisher', u'--force']" exit_code: 1 Last 500 characters of output: file WebKit/chromium/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file WebKit/chromium/WebKit.gyp Hunk #2 FAILED at 294. 1 out of 2 hunks FAILED -- saving rejects to file WebKit/chromium/WebKit.gyp.rej patching file WebKit/chromium/public/WebFileSystem.h patching file WebKit/chromium/public/WebFileWriter.h patching file WebKit/chromium/public/WebFileWriterClient.h patching file WebKit/chromium/src/AsyncFileWriterChromium.cpp patching file WebKit/chromium/src/AsyncFileWriterChromium.h Full output: http://queues.webkit.org/results/3837044

Comment on attachment 65519 [details] Fixed style error Cleared Darin Fisher's review+ from obsolete attachment 65519 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Created attachment 66980 [details] Merged out the latest code. No functional changes.

Comment on attachment 65744 [details] Added virtual hint Cleared Darin Fisher's review+ from obsolete attachment 65744 [details] so that this bug does not appear in http://webkit.org/pending-commit.

(In reply to comment #26) > Created an attachment (id=66980) [details] > Merged out the latest code. > > No functional changes. Ping? This is the same CL that was R+ a while back, but failed on CQ due to a merge problem.

Comment on attachment 66980 [details] Merged out the latest code. Clearing flags on attachment: 66980 Committed r67446: <http://trac.webkit.org/changeset/67446>

All reviewed patches have been landed. Closing bug.