[webkit-reviews] review denied: [Bug 45808] Add Worker support for FileSystem API : [Attachment 67995] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Sep 20 16:15:19 PDT 2010
David Levin <levin at chromium.org> has denied 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 67995: Patch
https://bugs.webkit.org/attachment.cgi?id=67995&action=review
------- Additional Comments from David Levin <levin at chromium.org>
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?
More information about the webkit-reviews
mailing list