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

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted 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 68467: Patch
https://bugs.webkit.org/attachment.cgi?id=68467&action=review

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