[webkit-reviews] review denied: [Bug 49939] Implement FileWriterSync : [Attachment 74713] Merge out the .vcproj yet again.

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Eric U. <ericu at chromium.org>'s
request for review:
Bug 49939: Implement FileWriterSync
https://bugs.webkit.org/show_bug.cgi?id=49939

Attachment 74713: Merge out the .vcproj yet again.
https://bugs.webkit.org/attachment.cgi?id=74713&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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);