[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 #68467|review?                     |review+
               Flag|                            |




--- Comment #35 from David Levin <levin at chromium.org>  2010-09-22 17:40:31 PST ---
(From update of attachment 68467)
View in context: https://bugs.webkit.org/attachment.cgi?id=68467&action=review

I have a few comments which you can address on landing.

Also, *please* get an ok from Darin Fisher on the change to WebKit/chromium/public/WebCommonWorkerClient.h before landing.

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:103
> +WorkerFileSystemCallbacksBridge::WorkerFileSystemCallbacksBridge(WebWorkerBase* worker, ScriptExecutionContext* scriptExecutionContext, WebFileSystemCallbacks* callbacks)

Consider moving this to be sorted in the same order as the header.

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:107
> +{

Please add  ASSERT(scriptExecutionContext->isContextThread());

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:110
> +WorkerFileSystemCallbacksBridge::~WorkerFileSystemCallbacksBridge()

Please move this if you move it in the header.

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:147
> +{

please add ASSERT(isMainThread());

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:152
> +{

please add ASSERT(isMainThread());

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:180
> +    MutexLocker locker(m_mutex);
> +    if (m_worker)
> +        m_worker->postTaskForModeToWorkerContext(task, mode);
> +    m_selfRef.clear();

Please fix this code like this:

 { // Let go of the mutex before possibly deleting this due to m_selfRef.clear().
     MutexLocker locker(m_mutex);
     if (m_worker)
         m_worker->postTaskForModeToWorkerContext(task, mode);
}
m_selfRef.clear();

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:50
> +struct WebFileInfo;

I would put struct after class (sorting the whole string).

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:73
> +    ~WorkerFileSystemCallbacksBridge();

fwiw, I would consider making this private and making ThreadSafeShared<WorkerFileSystemCallbacksBridge> a friend (like what is done in chromium for ref counted classes).

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:83
> +    // Method that posts an initial request task to the main thread. It is supposed to be called immediately after the bridge is constructed (it doesn't check if the context has been stopped or not).

Consider "constructed. (It doesn't ... not.)"

Also I'd remove "Method that" here.

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:103
> +    // Helper method.

Does this comment add anything? (If you want to add something information, fine, but this doesn't seem to be.)

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:106
> +    // m_selfRef keeps a reference to itself until callbacks are fired on the worker thread.

until tasks are created for the worker thread (at which point the tasks hold the reference).

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