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

[bugzilla-daemon at webkit.org]

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 67862: Patch
https://bugs.webkit.org/attachment.cgi?id=67862&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=67862&action=prettypatch

> WebCore/ChangeLog:10
> +	   Also changed how to get the base path for Web file systems (in
non-chromium ports) so that it works for workers too.  This patch assumes each
port calls LocalFileSystem::initializeLocalFileSystem() in its initialization
phase.

In general lines are wrapped some in the ChangeLog (unlike everywhere else in
WebKit.

> WebCore/ChangeLog:35
> +	   * workers/WorkerContext.idl:

Ideally you would write something (brief) that describes why you did a change
in this file (or function).

See Darin Adler's ChangeLogs for a good example of what I mean.

> WebCore/ChangeLog:1454
> +	   Added NoStaticTables and changed modulename from storage to fileapi:


This change seems very misplaced.

> WebCore/fileapi/LocalFileSystem.cpp:62
> +    staticLocalFileSystem = new LocalFileSystem(basePath);

Is this function called from multiple threads (because it isn't threadsafe)?

You can either assert that this is on the main thread or use the macro which
takes a mutex to do this in a threadsafe manner. 

Also here is another plain new. Use OwnPtr... as I mention in another place
(below).

> WebCore/fileapi/LocalFileSystem.cpp:67
> +    if (!staticLocalFileSystem)

same comment as the other method which does something like this.

> WebCore/workers/WorkerContext.h:127
> +	   // They are placed here and in all capital letters to enforce
compile-time enum checking.

Alexey had a question about this comment that seems unanswered (and I have the
same question).

It may be best to answer the question by fixing the comment since at least two
people looking at this code have had the same reaction.

> WebKit/chromium/src/WebWorkerBase.cpp:128
> +	   ASSERT(new OpenFileSystemMainThreadBridge(worker, commonClient,
mode, type, size, callbacks));

This isn't going to work in !debug.

Also, why ASSERT this at all?

Lastly, use OwnPtr().leakPtr() here. WebKit is moving away from having an
"new"s that don't use OwnPtr.

> WebKit/chromium/src/WebWorkerBase.cpp:150
> +	       commonClient->openFileSystem(type, size, new
MainThreadFileSystemCallbacks(bridge));

Another instance of new.
Consider making the constructor for MainThreadFileSystemCallbacks private and
adding a create method which returns a PassOwnPtr.

> WebKit/chromium/src/WebWorkerBase.cpp:164
> +		   didFail(WebFileErrorAbort);

This call to "didFail" is incorrect.
This is the destructor and "didFail" will "delete this" (again).

> WebKit/chromium/src/WebWorkerBase.cpp:170
> +	       m_bridge.leakPtr()->didOpenFileSystemOnMainThread(name, path);

Why are these calls doing leak ptr? (A comment may be in order.)

I didn't parse/grok the whole patch yet but I felt there were enough things
here that it may be good to give this feedback now.