[webkit-reviews] review denied: [Bug 45808] Add Worker support for FileSystem API : [Attachment 68211] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Kinuko Yasuda
<kinuko at chromium.org>'s request for review:
Bug 45808: Add Worker support for FileSystem API
https://bugs.webkit.org/show_bug.cgi?id=45808

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

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

> WebCore/platform/AsyncFileSystem.cpp:37
> +#include "NotImplemented.h"

Out of order. (I wonder why the style bot didn't catch this. Maybe the #if
ENABLE messed it up.)

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:65
> +	   MutexLocker locker(m_mutex);

This doesn't work because didFailOnMainThread may have just been called and
posted a task to the Worker thread.

Here's a solution.  Do the following:

s/WorkerFileSystemCallbacksBridge/WorkerFileSystemCallbacks/
s/ThreadableCallbacksBridgeWrapper/WorkerFileSystemCallbacksBridge/
s/m_bridge/m_fileSystemCallbacks/

Move the post OnMainThread/OnWorkerThread methods into this class (now called
WorkerFileSystemCallbacksBridge).

Only use m_fileSystemCallbacks (formerly m_bridge) on the worker thread.

You'll still need the m_mutex to guard m_worker which may be cleared from the
worker thread but used on the main thread to post tasks. 

This model is similar to what "AllowDatabaseMainThreadBridge" is doing.

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:142
> +    PassRefPtr<ThreadableCallbacksBridgeWrapper> m_bridgeWrapper;

PassRefPtr should only be used for function parameters. This should be RefPtr.

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:45
> +class ThreadableCallbacksBridgeWrapper;

Out of order.

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:48
> +class WebWorkerBase;

Out of order.

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:63
> +class WorkerFileSystemCallbacksBridge : public WebCore::ActiveDOMObject,
public RefCounted<WorkerFileSystemCallbacksBridge> {

Interesting idea about the ActiveDOMObject. I've discussed it with dimich and
he indicated that you get a lot of extra stuff that you may not want when you
have an ActiveDOMObject.

There are two ideas:
1. If this could be owned by a JS class.
2. If that isn't possible, then perhaps adding a callback to ~WorkerContext.
You can find several similar things in ~ScriptExecutionContext
(WebKit/WebCore/dom/ScriptExecutionContext.cpp)