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

[bugzilla-daemon at webkit.org]

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





--- Comment #23 from Kinuko Yasuda <kinuko at chromium.org>  2010-09-20 21:04:25 PST ---
(In reply to comment #20)
> (From update of attachment 67995 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67995&action=review
> 
> > 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).

Changed to use a static member variable s_instance.  (I had borrowed some code from DatabaseTracker, which does something similar to my previous patch.)

> > WebCore/page/DOMWindow.idl:-196
> > -
> The only change to this file is a whitespace change?
> I would just omit this change from this patch.

Fixed.

> > 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.

Fixed.

> > 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.

Ok, I removed the comment now.

> > 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.

Fixed.

> > 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.

Fixed.

> > WebKit/chromium/src/WebWorkerBase.cpp:122
> > +class OpenFileSystemMainThreadBridge {
> 
> How does "OpenFileSystemMainThreadBridge" get cleaned up if the Worker is terminated before the callback happens?

I made the Bridge subclass of ActiveDOMObject to get notified when the context is going to be stopped. (Does it make sense?)
Also introduced a wrapper class that is shared by both treads and to be used as a weak reference to the bridge.

> 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.

I moved the class into a separate class, WorkerFileSystemCallbacksBridge.

> > 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.

Removed the local variables.
Also changed the bridge class to RefCounted -- mainly for the future synchronous implementation.

> > 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.

Right... fixed.

> > 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?

Fixed.

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