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




--- Comment #20 from David Levin <levin at chromium.org>  2010-09-20 16:15:20 PST ---
(From update of attachment 67995)
View in context: https://bugs.webkit.org/attachment.cgi?id=67995&action=review

Ok, I understand now.  Just a few comments to address and then this can get in.

Thanks!

> WebCore/fileapi/LocalFileSystem.cpp:53
> +static LocalFileSystem* staticLocalFileSystem = 0;

Why are you doing a singleton in this manner (as opposed to using a member variable with s_ prefix).

> WebCore/page/DOMWindow.idl:-196
> -

The only change to this file is a whitespace change?

I would just omit this change from this patch.

> WebCore/platform/AsyncFileSystem.cpp:44
> +    // Each platform should implement this.

Put a notImplemented() here (and include NotImplemented.h). (Doing this may make the comment redundant. Your choice.)

> WebCore/platform/AsyncFileSystem.cpp:50
> +    // Each platform should implement this.

Ditto.

> WebCore/workers/WorkerContext.h:128
> +        // The code generator generates the code that performs compile-time enum checking with constant attributes defined in the idl and enum values in the header, and the enum names must match with the attribute names.

I see that you put this in the bug and I didn't notice. I don't think either of these comments is necessary now (especially now that I understand it will general a compile time error if this is changed). Sorry to go back and forth on this.

> WebCore/workers/WorkerContext.idl:112
> +        attribute [EnabledAtRuntime=FileSystem] FlagsConstructor Flags;

Typically attribute is indented to leave "space" for readonly. See the attributes at this top of this file. Sorry that this isn't done consistently in this idl.

> WebKit/chromium/src/WebWorkerBase.cpp:121
> +// This class is used to post a openFileSystem request to the main thread and get called back for the request.  This must be destructed on the worker thread.

Single space after . in WK code.

> WebKit/chromium/src/WebWorkerBase.cpp:122
> +class OpenFileSystemMainThreadBridge {

How does "OpenFileSystemMainThreadBridge" get cleaned up if the Worker is terminated before the callback happens?

It would be nice to add a comment about this.

In fact, (although I've figure it out now), I think it would be nice to add a comment to explain how this works overall.

> WebKit/chromium/src/WebWorkerBase.cpp:124
> +    static void start(WebWorkerBase* worker, WebCommonWorkerClient* commonClient, const WTF::String& mode, WebFileSystem::Type type, long long size, WebFileSystemCallbacks* callbacks)

Why is this getting put in WebWorkerBase.cpp (as opposed to its own file)? (I know one other class did that but I wouldn't follow its example.)

Also as you do this, I suspect that you will move out methods out of this class definition that shouldn't be inlined.

> WebKit/chromium/src/WebWorkerBase.cpp:219
> +        OwnPtr<OpenFileSystemMainThreadBridge> bridge(bridgePtr);

Why create this local variable?  PassOwnPtr will do the destruction (and you can avoid the odd bridgePtr naming then).

> WebKit/chromium/src/WebWorkerBase.cpp:227
> +        OwnPtr<OpenFileSystemMainThreadBridge> bridge(bridgePtr);

Ditto.

> WebKit/chromium/src/WebWorkerBase.cpp:233
> +    WTF::String m_mode;

It would be better imo if m_mode were owned by MainThreadFileSystemCallbacks.

Otherwise you are accessing it on the main thread in didFailOnMainThread/didOpenFileSystemOnMainThread but destroying it on the Worker thread.

By making it part of MainThreadFileSystemCallbacks, it would get destroyed on the main thread as well.

> WebKit/chromium/src/WebWorkerBase.cpp:234
> +    WebFileSystemCallbacks* m_workerContextCallbacks;

This name "m_workerContextCallbacks" doesn't seem good. When I read it, I think the variable is about callbacks from the worker context, but it isn't. It is about the file system callbacks (on the worker context?).

Maybe m_callbacksOnWorkerContext?

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