Bug 49939

Summary: Implement FileWriterSync
Product: WebKit Reporter: Eric U. <ericu@chromium.org>
Component: HTML DOMAssignee: Eric U. <ericu@chromium.org>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue@webkit.org
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 44358    
Attachments:
Description Flags
Webcore implementation, down to the level of AsyncFileWriter
none
Fixed 3 typo-level bugs that killed the debug build.
none
Merged out to resolve conflicts. No functional changes.
none
Merge out the .vcproj yet again.
levin: review-
Rolled in David's feedback.
levin: review+
Made the small tweaks David recommended. none

Description From 2010-11-22 14:26:41 PST
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.
------- Comment #1 From 2010-11-22 17:16:32 PST -------
Created an attachment (id=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.
------- Comment #2 From 2010-11-23 13:23:44 PST -------
Created an attachment (id=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;
------- Comment #3 From 2010-11-23 17:29:18 PST -------
Created an attachment (id=74708) [details]
Merged out to resolve conflicts.  No functional changes.
------- Comment #4 From 2010-11-23 18:10:28 PST -------
Created an attachment (id=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.
------- Comment #5 From 2010-11-23 20:42:47 PST -------
(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.)

> 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);
------- Comment #6 From 2010-11-24 13:34:47 PST -------
Created an attachment (id=74789) [details]
Rolled in David's feedback.
------- Comment #7 From 2010-11-24 13:47:22 PST -------
(In reply to comment #5)
> (From update of attachment 74713 [details] [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);
------- Comment #8 From 2010-11-24 14:10:16 PST -------
(From update of attachment 74789 [details])
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
------- Comment #9 From 2010-11-24 15:01:02 PST -------
Created an attachment (id=74799) [details]
Made the small tweaks David recommended.

Functionally the same; just needs a CQ+.
------- Comment #10 From 2010-11-24 15:12:11 PST -------
(From update of attachment 74799 [details])
rs=me, based on dave's r+.
------- Comment #11 From 2010-11-24 18:42:52 PST -------
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 koivisto@iki.fi.  The commit-queue is continuing to process your patch.
------- Comment #12 From 2010-11-24 18:44:23 PST -------
(From update of attachment 74799 [details])
Clearing flags on attachment: 74799

Committed r72715: <http://trac.webkit.org/changeset/72715>
------- Comment #13 From 2010-11-24 18:44:29 PST -------
All reviewed patches have been landed.  Closing bug.