Bug 49939 – Implement FileWriterSync

• Component: DOM
• Status: RESOLVED FIXED

[Eric U.]

This bug is for the WebCore portion of the non-port-specific code [FileWriterSync.h, cpp] and such.  There will be separate bugs for the port-specific implementations.

[Eric U.]

Created attachment 74618 [details]
Webcore implementation, down to the level of AsyncFileWriter

This patch is a bit smaller than it looks; there's a lot of boilerplate, build file changes, copyright headers, and refactoring in there.  I've pulled out the chromium-specific changes into a separate bug, and kept the tests back until the test framework is checked in, but it's still big; sorry about that.  I don't see a clean cutting point that knocks this down much.

[Eric U.]

Created attachment 74690 [details]
Fixed 3 typo-level bugs that killed the debug build.

Here are the changes between this and the last patch:

[WebKit] diff /tmp/webcore_sync*.patch

Move return outside the #ifdef guard.
330d329
< +        return true;
331a331
> +        return true;

Replace "complete" with "true" [copy-paste error].
1139c1139
< +    m_complete = complete;
---
> +    m_complete = true;
1149c1149
< +    m_complete = complete;
---
> +    m_complete = true;

[Eric U.]

Created attachment 74708 [details]
Merged out to resolve conflicts.  No functional changes.

[Eric U.]

Created attachment 74713 [details]
Merge out the .vcproj yet again.

No functional changes; I just merged out the .vcproj yet again to see if that fixes the merge conflict.  I don't see the conflict in my client, anyway.

[David Levin]

Comment on attachment 74713 [details]
Merge out the .vcproj yet again.

View in context: https://bugs.webkit.org/attachment.cgi?id=74713&action=review

This seems fine. Just address the few issues I mentioned.

> WebCore/GNUmakefile.am:1471
> +	WebCore/fileapi/FileWriterBaseCallback.h \

Out of order. (Yes the next item is as well.)

> WebCore/WebCore.xcodeproj/project.pbxproj:-21454
> -			developmentRegion = English;

Please add this back and consider upgrading your local version of xcode. :)

> WebCore/fileapi/DOMFileSystemSync.cpp:100
> +#endif

Add blank line.

> WebCore/fileapi/DOMFileSystemSync.cpp:124
> +    }

Add blank line.

> WebCore/fileapi/DOMFileSystemSync.cpp:128
> +    }

Add blank line.

> WebCore/fileapi/DOMFileSystemSync.cpp:161
> +    ASSERT(static_cast<FileWriterSync*>(successCallback->fileWriterBase()) == fileWriter.get());

I don't think the static_cast is needed here.

> WebCore/fileapi/FileEntrySync.h:45
> +class ScriptExecutionContext;

I suppose this was always needed but it happened to be declared elsewhere until this change, right?

> WebCore/fileapi/FileSystemCallbacks.cpp:215
> +// FileWriterBaseCallbacks ----------------------------------------------------------

I just wanted to take an opportunity to point out that one class per file makes it so much easier for future people to find things.

No need to do a change here (and perhaps not ever but especially not in this patch and I guess these are small classes so it feels like overkill to have their own file but I'm starting to feel like even those cases may benefit from their own file).

Ok minor soliloquy is over...

> WebCore/fileapi/FileWriter.cpp:47
> +    : FileWriterBase()

I don't think you need to like a call to the default constructor.

> WebCore/fileapi/FileWriterBase.h:48
> +class FileWriterBase : public RefCounted<FileWriterBase>, public AsyncFileWriterClient {

I don't understand what AsyncFileWriterClient has to do with base functionality (especially because none of the callbacks are implemented here).

I understand that both derived class would need to derive from AsyncFileWriterClient but it feels like that is due to each of their own reasons (s it shouldn't be here -- I have a hard time explaining this well, so I won't defend it too hard but it feels wrong).

> WebCore/fileapi/FileWriterBase.h:51
> +    void initialize(PassOwnPtr<AsyncFileWriter> writer, long long length);

Remove param name "writer" and "ec"" below.

> WebCore/fileapi/FileWriterBase.h:55
> +    void truncate(long long length, ExceptionCode& ec);

Where are these implemented?

FileWriterBase::write, etc. (I see overrides in FileWriter but that is confusing to me because these aren't virtual.)

> WebCore/fileapi/FileWriterSync.cpp:45
> +    ASSERT(writer());

ASSERT(m_complete);

> WebCore/fileapi/FileWriterSync.cpp:57
> +    if (!ec) {

fail fast
if (ec)
  return;

> WebCore/fileapi/FileWriterSync.cpp:64
> +void FileWriterSync::seek(long long position, ExceptionCode& ec)

This implementation seems like it should move into FileWriterBase.

Actually I think there should be a function that both FileWriterSync::seek and FileWriter::seek can call but it should likely not be seek as it is shouldn't be called on its own. (It is like "baseSeek" but that is a pretty terrible great name...)

> WebCore/fileapi/FileWriterSync.cpp:66
> +    ASSERT(writer());

ASSERT(m_complete);

> WebCore/fileapi/FileWriterSync.cpp:79
> +    ASSERT(writer());

ASSERT(m_complete);

> WebCore/fileapi/FileWriterSync.cpp:90
> +    if (m_error == FileError::OK) {

fail fast

> WebCore/fileapi/FileWriterSync.cpp:128
> +    : FileWriterBase()

Not needed.

> WebCore/fileapi/FileWriterSync.cpp:137
> +{

ASSERT(m_complete);

[Eric U.]

Created attachment 74789 [details]
Rolled in David's feedback.

[Eric U.]

(In reply to comment #5)
> (From update of attachment 74713 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74713&action=review
> 
> This seems fine. Just address the few issues I mentioned.
> 
> > WebCore/GNUmakefile.am:1471
> > +	WebCore/fileapi/FileWriterBaseCallback.h \
> 
> Out of order. (Yes the next item is as well.)

Fixed.

> > WebCore/WebCore.xcodeproj/project.pbxproj:-21454
> > -			developmentRegion = English;
> 
> Please add this back and consider upgrading your local version of xcode. :)

Arg.  Yeah, I've been waiting to do that until after M9, so that I wouldn't risk delays.  I'll upgrade next week.

> > WebCore/fileapi/DOMFileSystemSync.cpp:100
> > +#endif
> 
> Add blank line.

Done.

> > WebCore/fileapi/DOMFileSystemSync.cpp:124
> > +    }
> 
> Add blank line.

Done.

> > WebCore/fileapi/DOMFileSystemSync.cpp:128
> > +    }
> 
> Add blank line.

Done.

> > WebCore/fileapi/DOMFileSystemSync.cpp:161
> > +    ASSERT(static_cast<FileWriterSync*>(successCallback->fileWriterBase()) == fileWriter.get());
> 
> I don't think the static_cast is needed here.

It is now ;'>.
I've taken your advice to move the inheritance of AsyncFileWriterClient down to its subclasses, so I think this cast can now change the pointer value.

> > WebCore/fileapi/FileEntrySync.h:45
> > +class ScriptExecutionContext;
> 
> I suppose this was always needed but it happened to be declared elsewhere until this change, right?

Nope; it would have been needed if FileEntrySync had stayed an ActiveDOMObject, but when I took that out, this wasn't needed any more.
Removed.

> > WebCore/fileapi/FileSystemCallbacks.cpp:215
> > +// FileWriterBaseCallbacks ----------------------------------------------------------
> 
> I just wanted to take an opportunity to point out that one class per file makes it so much easier for future people to find things.
> 
> No need to do a change here (and perhaps not ever but especially not in this patch and I guess these are small classes so it feels like overkill to have their own file but I'm starting to feel like even those cases may benefit from their own file).
> 
> Ok minor soliloquy is over...
> 
> > WebCore/fileapi/FileWriter.cpp:47
> > +    : FileWriterBase()
> 
> I don't think you need to like a call to the default constructor.

Removed.

> > WebCore/fileapi/FileWriterBase.h:48
> > +class FileWriterBase : public RefCounted<FileWriterBase>, public AsyncFileWriterClient {
> 
> I don't understand what AsyncFileWriterClient has to do with base functionality (especially because none of the callbacks are implemented here).
> 
> I understand that both derived class would need to derive from AsyncFileWriterClient but it feels like that is due to each of their own reasons (s it shouldn't be here -- I have a hard time explaining this well, so I won't defend it too hard but it feels wrong).

No, you're absolutely right.  I've pushed it down to the subclasses.

> > WebCore/fileapi/FileWriterBase.h:51
> > +    void initialize(PassOwnPtr<AsyncFileWriter> writer, long long length);
> 
> Remove param name "writer" and "ec"" below.

Done.

> > WebCore/fileapi/FileWriterBase.h:55
> > +    void truncate(long long length, ExceptionCode& ec);
> 
> Where are these implemented?
> 
> FileWriterBase::write, etc. (I see overrides in FileWriter but that is confusing to me because these aren't virtual.)

Copy/paste error; removed.

> > WebCore/fileapi/FileWriterSync.cpp:45
> > +    ASSERT(writer());
> 
> ASSERT(m_complete);

Added these and several other asserts.

> > WebCore/fileapi/FileWriterSync.cpp:57
> > +    if (!ec) {
> 
> fail fast
> if (ec)
>   return;

Done.

Fun C++ fact of the day: if you forget the semicolon after your return statement, in a function returning void, and the next line is a function call returning void, you compile with no warning or error.  You don't get what you expected, though.

> > WebCore/fileapi/FileWriterSync.cpp:64
> > +void FileWriterSync::seek(long long position, ExceptionCode& ec)
> 
> This implementation seems like it should move into FileWriterBase.
> 
> Actually I think there should be a function that both FileWriterSync::seek and FileWriter::seek can call but it should likely not be seek as it is shouldn't be called on its own. (It is like "baseSeek" but that is a pretty terrible great name...)

Done; I called it seekInternal.

> > WebCore/fileapi/FileWriterSync.cpp:66
> > +    ASSERT(writer());
> 
> ASSERT(m_complete);
> 
> > WebCore/fileapi/FileWriterSync.cpp:79
> > +    ASSERT(writer());
> 
> ASSERT(m_complete);
> 
> > WebCore/fileapi/FileWriterSync.cpp:90
> > +    if (m_error == FileError::OK) {
> 
> fail fast

Done.

> > WebCore/fileapi/FileWriterSync.cpp:128
> > +    : FileWriterBase()
> 
> Not needed.

Fixed.

> > WebCore/fileapi/FileWriterSync.cpp:137
> > +{
> 
> ASSERT(m_complete);

[David Levin]

Comment on attachment 74789 [details]
Rolled in David's feedback.

View in context: https://bugs.webkit.org/attachment.cgi?id=74789&action=review

Please consider a follow up clean up patch. Or just clean this one up and re-upload (with the reviewed by filled in as me and then get any committer to cq+ it).

Not cq+ so you can make this decision.

> WebCore/fileapi/DOMFileSystemSync.cpp:94
> +    }

Adding blank lines between methods would be nice.

> WebCore/fileapi/FileWriter.h:59
> +    // FileWriterBase

Nope.

> WebCore/fileapi/FileWriter.h:60
>      void write(Blob* data, ExceptionCode& ec);

remove "ec" in all places.

> WebCore/fileapi/FileWriterBase.h:76
> +    }

blank

> WebCore/fileapi/FileWriterBase.h:77
> +    void seekInternal(long long position);

blank

[Eric U.]

Created attachment 74799 [details]
Made the small tweaks David recommended.

Functionally the same; just needs a CQ+.

[Dumitru Daniliuc]

Comment on attachment 74799 [details]
Made the small tweaks David recommended.

rs=me, based on dave's r+.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74799 [details]:

http/tests/misc/uncacheable-script-repeated.html

Please file bugs against the tests.  These tests were authored by [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 74799 [details]
Made the small tweaks David recommended.

Clearing flags on attachment: 74799

Committed r72715: <http://trac.webkit.org/changeset/72715>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.