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




--- Comment #13 from David Levin <levin at chromium.org>  2010-09-17 15:20:25 PST ---
(From update of attachment 67862)
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.

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