[Webkit-unassigned] [Bug 45808] Add Worker support for FileSystem API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=45808


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #68211|review?                     |review-
               Flag|                            |




--- Comment #25 from David Levin <levin at chromium.org>  2010-09-21 14:39:34 PST ---
(From update of attachment 68211)
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)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.