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

[bugzilla-daemon at webkit.org]

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


Kinuko Yasuda <kinuko at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |levin at chromium.org




--- Comment #15 from Kinuko Yasuda <kinuko at chromium.org>  2010-09-17 17:14:29 PST ---
(In reply to comment #13)
> (From update of attachment 67862 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67862&action=prettypatch
> 
> > WebCore/ChangeLog:10
> In general lines are wrapped some in the ChangeLog (unlike everywhere else in WebKit.
> Ideally you would write something (brief) that describes why you did a change in this file (or function).

Fixed. (Or I tried to fix.)

> > WebCore/ChangeLog:1454
> > +        Added NoStaticTables and changed modulename from storage to fileapi:
> 
> This change seems very misplaced.

Hmm that's true I removed them from this patch.  I'm going to file another issue.

> > WebCore/fileapi/LocalFileSystem.cpp:62
> > +    staticLocalFileSystem = new LocalFileSystem(basePath);
> Is this function called from multiple threads (because it isn't threadsafe)?

initializeLocalFileSystem is assumed to be called only once before any workers start (as in the header comment).
localFileSystem() should have had an assert -- fixed.

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

Fixed.

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

I tried to answer in comment #8 but seems like it didn't work well :(
"If there're constant attributes in idl the code generator automatically generates enum checking code with the enum values (unless we specify DontCheckEnums)."

I added some more comment in the header file, hope it makes sense.

>>        // They are placed here and in all capital letters to enforce compile-time enum checking.
>>        // 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.

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

Oops, right it was wrong.  Fixed.

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

I fixed my code to explicitly use that.

(I had wondered if I should do so even I would never use the return value... but maybe the coding rules should be respected)

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

Fixed.

> > WebKit/chromium/src/WebWorkerBase.cpp:164
> > +                didFail(WebFileErrorAbort);
> 
> This call to "didFail" is incorrect.
> This is the destructor and "didFail" will "delete this" (again).

Fixed.

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

The bridge destructs itself on the worker thread, and it shouldn't get deleted on the main thread.  I added a comment there.

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

Thanks.  Could I expect more feedbacks then... :)

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