Bug 44360 – Add Chromium API for FileWriter

• Component: DOM
• Status: RESOLVED FIXED

[Eric U.]

Add the Chromium API in WebKit/chromium/public for FileWriter.

[Eric U.]

Created attachment 65014 [details]
Patch

[Kinuko Yasuda]

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

[Eric U.]

(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.

[Darin Fisher (:fishd, Google)]

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.)

[Darin Fisher (:fishd, Google)]

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

[Eric U.]

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.

[Darin Fisher (:fishd, Google)]

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.

[Eric U.]

Created attachment 65516 [details]
Rolled in all comments.

[WebKit Review Bot]

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.

[Eric U.]

Created attachment 65519 [details]
Fixed style error

[Darin Fisher (:fishd, Google)]

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?

[Eric U.]

(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.

[Darin Fisher (:fishd, Google)]

(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.

[Eric U.]

Created attachment 65582 [details]
Added requested comment

I've added the comment; no other changes.

[WebKit Review Bot]

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.

[Eric U.]

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.

[Eric U.]

Comment on attachment 65630 [details]
Adjusted as discussed offline

Fixing attachment type.

[Kinuko Yasuda]

(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;

[Eric U.]

Created attachment 65638 [details]
Fixed issues Kinuko found

All fixed.  Thanks!

[Darin Fisher (:fishd, Google)]

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

[Eric U.]

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.

[Michael Nordman]

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

[Eric U.]

Created attachment 65744 [details]
Added virtual hint

Added virtual hints, as Michael suggested.

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

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.

[Eric U.]

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

No functional changes.

[Eric Seidel (no email)]

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.

[Eric U.]

(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.

[WebKit Commit Bot]

Comment on attachment 66980 [details]
Merged out the latest code.

Clearing flags on attachment: 66980

Committed r67446: <http://trac.webkit.org/changeset/67446>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.