[webkit-reviews] review denied: [Bug 47681] [Chromium] implementation of async FileWriter for workers : [Attachment 73436] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Eric U. <ericu at chromium.org>'s
request for review:
Bug 47681: [Chromium] implementation of async FileWriter for workers
https://bugs.webkit.org/show_bug.cgi?id=47681

Attachment 73436: Patch
https://bugs.webkit.org/attachment.cgi?id=73436&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=73436&action=review

This looks close. It may need a few changes or at least a few more comments.

And it would b good to include the generic version of the template instead of
this specific instance. I'll tell you what to change in another note and
explain it as well.

> WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:158
> +	       OwnPtr<WorkerAsyncFileWriterChromium> asyncFileWriterChromium =
adoptPtr(new WorkerAsyncFileWriterChromium(m_webFileSystem, m_path,
m_workerContext, m_client, false));

Typically you'd hide the constructor for WorkerAsyncFileWriterChromium and only
expose a create method which returned a PassOwnPtr. (The adoptPtr would only be
used in the create method.) Everytime I see it outside of a "create" method it
looks wrong (and mildly is).

> WebKit/chromium/src/WorkerAsyncFileWriterChromium.cpp:53
> +static const char fileWriterOperationsMode[] = "fileWriterOperationsMode";

This is unused. Please remove it for now (so when you add it, I'll be more
likely to notice the sync handling code as well).

> WebKit/chromium/src/WorkerAsyncFileWriterChromium.cpp:58
> +    ASSERT(!synchronous); // Not implemented yet.

#include <wtf/Assertions.h> (above).

> WebKit/chromium/src/WorkerAsyncFileWriterChromium.cpp:84
> +} // namespace

If you include // namespace, add the namespace name, but I think the preference
is just to not do this at all.

> WebKit/chromium/src/WorkerAsyncFileWriterChromium.h:43
> +class WebFileSystem;

Please indent (4 spaces).

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:159
> +void
WorkerFileWriterCallbacksBridge::didWriteOnWorkerThread(ScriptExecutionContext*
, WorkerFileWriterCallbacksBridge* bridge, long long length, bool complete)

How do you know that bridge is still alive/ok to use at this point?

(Add a comment somewhere. Likely at the class level to explain how the lifetime
works.)

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:161
> +    bridge->m_clientOnWorkerThread->didWrite(length, complete);

How do you know that m_clientOnWorkerThread is still alive/ok to use?

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:69
> +//	   This calls the original client (m_clientOnWorkerThread).

A very important part about this description would be something to indicate
lifetimes of objects/when things die or are known to be alive and how the
lifetimes relate.

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:110
> +    // They release a selfRef of the WorkerFileWriterCallbacksBridge.

I don't see the release of the selfRef.