Bug 45808 – Add Worker support for FileSystem API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kinuko Yasuda]

Add Worker support for FileSystem API.

* requestFileSystem and Flags constructor must be exposed on the worker context.
* other FileSystem operations must be supported on workers too.

[Kinuko Yasuda]

Created attachment 67649 [details]
Patch

[Kinuko Yasuda]

Created attachment 67651 [details]
Patch

[Alexey Proskuryakov]

+    ~LocalFileSystem() { }

If the intention is for the destructor to never be called, then it shouldn't be defined.

+    // FIXME: See if access is allowed.

This is a scary FIXME to have in code!

+COMPILE_ASSERT(int(WorkerContext::TEMPORARY) == int(AsyncFileSystem::Temporary), enum_mismatch);

Coding style says that we should use static_cast, not C-style casts.

+        // They are placed here and in all capital letters to enforce compile-time enum checking.

How do capital letters help compile time enum checking?

I haven't reviewed the substance of the patch, largely because of naming issues that were making reading too hard. Specifically, "requestFileSystem()" has taken me aback, as a file system is not something I'd expect to be requested.

[Eric U.]

Comment on attachment 67651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67651&action=prettypatch

> WebCore/ChangeLog:8
> +        Exposed requestFileSystem and Flags constructor on worker contexts.
As soon as you expose DOMFileSystem types to workers, you need to add the NoStaticTables modifier in the IDL files for any type thus exposed.  About half of the files in fileapi/ have it already [from FileReader/Blob stuff], but they all should.  See https://trac.webkit.org/wiki/IdlAttributes for more info.

[Kinuko Yasuda]

Created attachment 67725 [details]
Patch

[Kinuko Yasuda]

Created attachment 67726 [details]
Patch

[Kinuko Yasuda]

Created attachment 67729 [details]
Patch

[Kinuko Yasuda]

(In reply to comment #3)
> +    ~LocalFileSystem() { }
> If the intention is for the destructor to never be called, then it shouldn't be defined.

Removed.

> +    // FIXME: See if access is allowed.
> This is a scary FIXME to have in code!

Added access and availability checks.

> +COMPILE_ASSERT(int(WorkerContext::TEMPORARY) == int(AsyncFileSystem::Temporary), enum_mismatch);
> Coding style says that we should use static_cast, not C-style casts.

Fixed.

> +        // They are placed here and in all capital letters to enforce compile-time enum checking.
> How do capital letters help compile time enum checking?

If there're constant attributes in idl the code generator automatically generates enum checking code with the enum values (unless we specify DontCheckEnums).

> I haven't reviewed the substance of the patch, largely because of naming issues that were making reading too hard. Specifically, "requestFileSystem()" has taken me aback, as a file system is not something I'd expect to be requested.

The names LocalFileSystem and requestFileSystem are taken from its spec:
http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#using-localfilesystem

In terms of class/file naming there're too many *FileSystem classes and I admit it might be hard to read -- DOMFileSystem corresponds to JavaScript's FileSystem interface in the FileSystem API and AsyncFileSystem is a platform-dependent lower layer implementation for the filesystems.

If there're any feedbacks I could do to improve the design / readability I'm glad to change the code.
Thanks,

[Kinuko Yasuda]

(In reply to comment #4)
> (From update of attachment 67651 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67651&action=prettypatch
> 
> > WebCore/ChangeLog:8
> > +        Exposed requestFileSystem and Flags constructor on worker contexts.
> As soon as you expose DOMFileSystem types to workers, you need to add the NoStaticTables modifier in the IDL files for any type thus exposed.  About half of the files in fileapi/ have it already [from FileReader/Blob stuff], but they all should.  See https://trac.webkit.org/wiki/IdlAttributes for more info.

Right right... thanks for pointing it out.  Fixed (for all non-callback interfaces).

[Michael Nordman]

Comment on attachment 67729 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67729&action=prettypatch

> WebKit/chromium/public/WebCommonWorkerClient.h:90
> +    virtual void openFileSystem(WebFrame*, WebFileSystem::Type, long long size, WebFileSystemCallbacks*)

As mentioned in the chromium review, I don't think the WebFrame* param is needed or desired here.

[Kinuko Yasuda]

Created attachment 67862 [details]
Patch

Removed Frame parameters from WebCommonWorkerClient::openFileSystem().

[Michael Nordman]

Comment on attachment 67862 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67862&action=prettypatch

> WebKit/chromium/src/LocalFileSystemChromium.cpp:70
> +        webWorker->openFileSystem(0, static_cast<WebFileSystem::Type>(type), size, new WebFileSystemCallbacksImpl(new FileSystemCallbacks(successCallback, errorCallback, context)));

is the first '0' arg value stale now that we've removed the 'frame' parameter?

> WebKit/chromium/src/WebWorkerBase.h:95
> +    virtual void openFileSystem(WebFrame*, WebFileSystem::Type, long long size, WebFileSystemCallbacks*);

ah... can we remove the WebFrame* here too?

> WebKit/chromium/src/WebWorkerBase.cpp:338
> +void WebWorkerBase::openFileSystem(WebFrame*, WebFileSystem::Type type, long long size, WebFileSystemCallbacks* callbacks)

and here

> WebKit/chromium/src/WebWorkerClientImpl.h:102
> +    virtual void openFileSystem(WebFrame*, WebFileSystem::Type, long long size, WebFileSystemCallbacks*)

and here

[David Levin]

Comment on attachment 67862 [details]
Patch

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.

[Kinuko Yasuda]

Created attachment 67974 [details]
Patch

[Kinuko Yasuda]

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

[Kinuko Yasuda]

Created attachment 67994 [details]
Patch

[Kinuko Yasuda]

Created attachment 67995 [details]
Patch

Made a few more fixes.

[David Levin]

(In reply to comment #15)
> > > 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.

Actually I didn't mean that you needed a new patch. I mean the description in the ChangeLog was very misplaced. (It was ~100 lines below your change log entry in some random spot.) But I'm glad that you created a new patch which was really small.

As much as you can break down your big patches into smaller ones, they will be easier to review, more thoroughly reviewed and each one will get through more quickly (though on the whole it may take a little longer).

[Kinuko Yasuda]

(In reply to comment #18)
> (In reply to comment #15)
> > > > 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.
> 
> Actually I didn't mean that you needed a new patch. I mean the description in the ChangeLog was very misplaced. (It was ~100 lines below your change log entry in some random spot.) But I'm glad that you created a new patch which was really small.

Oh I see... resolve-ChangeLog somehow placed them at a random place.

> As much as you can break down your big patches into smaller ones, they will be easier to review, more thoroughly reviewed and each one will get through more quickly (though on the whole it may take a little longer).

Thanks, I'll try doing so.  (This one might be still a bit larger)

[David Levin]

Comment on attachment 67995 [details]
Patch

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?

[Kinuko Yasuda]

Created attachment 68182 [details]
Patch

[Kinuko Yasuda]

Created attachment 68183 [details]
Patch

[Kinuko Yasuda]

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

[Kinuko Yasuda]

Created attachment 68211 [details]
Patch

[David Levin]

Comment on attachment 68211 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68211&action=review

> WebCore/platform/AsyncFileSystem.cpp:37
> +#include "NotImplemented.h"

Out of order. (I wonder why the style bot didn't catch this. Maybe the #if ENABLE messed it up.)

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:65
> +        MutexLocker locker(m_mutex);

This doesn't work because didFailOnMainThread may have just been called and posted a task to the Worker thread.

Here's a solution.  Do the following:

s/WorkerFileSystemCallbacksBridge/WorkerFileSystemCallbacks/
s/ThreadableCallbacksBridgeWrapper/WorkerFileSystemCallbacksBridge/
s/m_bridge/m_fileSystemCallbacks/

Move the post OnMainThread/OnWorkerThread methods into this class (now called WorkerFileSystemCallbacksBridge).

Only use m_fileSystemCallbacks (formerly m_bridge) on the worker thread.

You'll still need the m_mutex to guard m_worker which may be cleared from the worker thread but used on the main thread to post tasks. 

This model is similar to what "AllowDatabaseMainThreadBridge" is doing.

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:142
> +    PassRefPtr<ThreadableCallbacksBridgeWrapper> m_bridgeWrapper;

PassRefPtr should only be used for function parameters. This should be RefPtr.

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:45
> +class ThreadableCallbacksBridgeWrapper;

Out of order.

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:48
> +class WebWorkerBase;

Out of order.

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:63
> +class WorkerFileSystemCallbacksBridge : public WebCore::ActiveDOMObject, public RefCounted<WorkerFileSystemCallbacksBridge> {

Interesting idea about the ActiveDOMObject. I've discussed it with dimich and he indicated that you get a lot of extra stuff that you may not want when you have an ActiveDOMObject.

There are two ideas:
1. If this could be owned by a JS class.
2. If that isn't possible, then perhaps adding a callback to ~WorkerContext. You can find several similar things in ~ScriptExecutionContext (WebKit/WebCore/dom/ScriptExecutionContext.cpp)

[Kinuko Yasuda]

Created attachment 68328 [details]
Patch

[Kinuko Yasuda]

Created attachment 68329 [details]
Patch

[Kinuko Yasuda]

Created attachment 68330 [details]
Patch

Removed the code for other FS operations, that weren't meant to be added yet.

[Kinuko Yasuda]

Thanks for the iteration.

(In reply to comment #25)
> (From update of attachment 68211 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68211&action=review
> 
> > WebCore/platform/AsyncFileSystem.cpp:37
> > +#include "NotImplemented.h"
> Out of order. (I wonder why the style bot didn't catch this. Maybe the #if ENABLE messed it up.)

Fixed.

> > WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:65
> > +        MutexLocker locker(m_mutex);
> This doesn't work because didFailOnMainThread may have just been called and posted a task to the Worker thread.
> Here's a solution.  Do the following:

Fixed (not exactly as mentioned, but tried to follow the way).

Turned WorkerFileSystemCallbacksBridge into ThreadSafeShared and changed its post methods to use mutex protected m_worker.
No wrapper class for worker's callback, and is touched only on the worker thread.

> > WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:142
> > +    PassRefPtr<ThreadableCallbacksBridgeWrapper> m_bridgeWrapper;
> PassRefPtr should only be used for function parameters. This should be RefPtr.

Gush, fixed.

> > WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:45
> > +class ThreadableCallbacksBridgeWrapper;
> Out of order.
> > WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:48
> > +class WebWorkerBase;
> Out of order.

Fixed.

> > WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:63
> > +class WorkerFileSystemCallbacksBridge : public WebCore::ActiveDOMObject, public RefCounted<WorkerFileSystemCallbacksBridge> {
> 
> Interesting idea about the ActiveDOMObject. I've discussed it with dimich and he indicated that you get a lot of extra stuff that you may not want when you have an ActiveDOMObject.
> 
> There are two ideas:
> 1. If this could be owned by a JS class.
> 2. If that isn't possible, then perhaps adding a callback to ~WorkerContext. You can find several similar things in ~ScriptExecutionContext (WebKit/WebCore/dom/ScriptExecutionContext.cpp)

Added a WorkerContext::Observer class whose instances are notified when the WorkerThread is stopping.  (The class might be a bit overkilling)

[Kinuko Yasuda]

Created attachment 68338 [details]
Patch

Nits fixes

[David Levin]

Comment on attachment 68338 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68338&action=review

I almost r+'ed but it feels like there are enough little things to address that it would be good to check it over one last time after the adjustment.

Thanks for your patience. It is almost all done!

> WebCore/fileapi/LocalFileSystem.h:74
>      String m_basePath;

What about making a (private) nested class that has this string as a private member?  Then access would be forced to got through its accessor.

Something like this:

class SystemBasePath {
public:
    explicit SystemBasePath(string path) : m_value(path) { }

    string value() const
    {
        return m_value.threadsafeCopy();
    }  

private:
    String m_value;
};
SystemBasePath m_basePath;

Then no need for a comment that people probably won't see when adding code or doing reviews :)

> WebCore/workers/WorkerContext.cpp:116
> +    HashSet<Observer*>::iterator observersEnd = m_workerObservers.end();
> +    for (HashSet<Observer*>::iterator iter = m_workerObservers.begin(); iter != observersEnd; ++iter)
> +        (*iter)->stopObserving();

Just call notifyObserversOfStop.

> WebCore/workers/WorkerContext.cpp:385
> +    if (m_context) {

Consider "fail fast" here.

if (!m_context)
    return;

> WebCore/workers/WorkerContext.cpp:386
> +        ASSERT(m_context->isContextThread());

Why doesn't this do
  m_context->unregisterObserver(this);
?

> WebCore/workers/WorkerContext.cpp:397
> +    HashSet<Observer*>::iterator observersEnd = m_workerObservers.end();
> +    for (HashSet<Observer*>::iterator iter = m_workerObservers.begin(); iter != observersEnd; ++iter) {
> +        (*iter)->notifyStop();
> +        (*iter)->stopObserving();
> +    }

I would make this robust to unregisterObserver being called during notifyStop. For example, an object may delete itself :)

Here's a simple change:
HashSet<Observer*>::iterator iter = m_workerObservers.begin();
while (iter != m_workerObservers.end()) {
    (*iter)->notifyStop();
    (*iter)->stopObserving();
    iter = m_workerObservers.begin();
}

Of course, there could be a pathological case in which an object adds something else when it gets the notifyStop but that seems highly unlikely.

> WebCore/workers/WorkerContext.cpp:398
> +    m_workerObservers.clear();

If the above is done, this should be already done.

> WebCore/workers/WorkerContext.h:59
> +    class WorkerWeakReference;

This doesn't appear to be used (or defined).

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

Is this comment useful? (If anyone tries to change this, they'll get a compile type error.)

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:120
> +    if (m_callbacksOnWorkerThread) {

How do you know that m_callbacksOnWorkerThread is still valid at this point?

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:126
> +void WorkerFileSystemCallbacksBridge::postOpenFileSystemOnMainThread(WebCommonWorkerClient* commonClient, WebFileSystem::Type type, long long size, const String& mode)

Consider making the name postOpenFileSystemToMainThread.

OnXThread is being used for methods that run on Thread X but this method is to push something to the main thread.

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:134
> +{

Please consider adding asserts to verify that things are called on the correct thread:
 ASSERT(isMainThread()); and ASSERT(m_worker->isContextThread());

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:145
> +    mayPostTaskToWorker(createCallbackTask(&didFailOnWorkerThread, this, error), mode);

I believe you can change s/this/this->m_selfRef.releaseRef()/ and the remove all of the clear calls.
or you may only be able to do s/this/this->m_selfRef/ (and just have one clear call in mayPostTaskToWorker).  (I should make the former possible in the future if it isn't currently.)

(here and in didOpenFileSystemOnMainThread)

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:153
> +void WorkerFileSystemCallbacksBridge::didFailOnWorkerThread(ScriptExecutionContext*, WorkerFileSystemCallbacksBridge* bridge, WebFileError error)

s/WorkerFileSystemCallbacksBridge*/PassRefPtr<WorkerFileSystemCallbacksBridge>/
ditto for didOpenFileSystemOnWorkerThread

[Kinuko Yasuda]

Created attachment 68463 [details]
Patch

[Kinuko Yasuda]

Created attachment 68467 [details]
Patch

[Kinuko Yasuda]

Thanks, updated.

(In reply to comment #31)
> (From update of attachment 68338 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68338&action=review
> > WebCore/fileapi/LocalFileSystem.h:74
> >      String m_basePath;
> 
> What about making a (private) nested class that has this string as a private member?  Then access would be forced to got through its accessor.
> Something like this:
> class SystemBasePath { ...

Sounds a good idea.  Fixed.

> > WebCore/workers/WorkerContext.cpp:116
> > +    HashSet<Observer*>::iterator observersEnd = m_workerObservers.end();
> > +    for (HashSet<Observer*>::iterator iter = m_workerObservers.begin(); iter != observersEnd; ++iter)
> > +        (*iter)->stopObserving();
> Just call notifyObserversOfStop.

Fixed.

> > WebCore/workers/WorkerContext.cpp:385
> > +    if (m_context) {
> Consider "fail fast" here.

Fixed.

> > WebCore/workers/WorkerContext.cpp:386
> > +        ASSERT(m_context->isContextThread());
> 
> Why doesn't this do
>   m_context->unregisterObserver(this);
> ?

Fixed.  (I wondered the same thing when I was self-reviewing... :))

> > WebCore/workers/WorkerContext.cpp:397
> > +    HashSet<Observer*>::iterator observersEnd = m_workerObservers.end();
> > +    for (HashSet<Observer*>::iterator iter = m_workerObservers.begin(); iter != observersEnd;
>  ...
 > I would make this robust to unregisterObserver being called during notifyStop. For example, an object may delete itself :)

Right.  Fixed.

> > WebCore/workers/WorkerContext.h:59
> > +    class WorkerWeakReference;
> This doesn't appear to be used (or defined).

Removed.

> > WebCore/workers/WorkerContext.h:130
> > +        // They are placed here and in all capital letters to enforce compile-time enum checking.
> Is this comment useful? (If anyone tries to change this, they'll get a compile type error.)

Removed.

> > WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:120
> > +    if (m_callbacksOnWorkerThread) {
> How do you know that m_callbacksOnWorkerThread is still valid at this point?

'Not owned' comment was wrong... it is self-destructed too and the bridge has the responsibility to call any of the callbacks (and the pointer itself should be valid until the bridge fires it).

> > WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:126
> > +void WorkerFileSystemCallbacksBridge::postOpenFileSystemOnMainThread(WebCommonWorkerClient* commonClient, WebFileSystem::Type type, long long size, const String& mode)
> Consider making the name postOpenFileSystemToMainThread.
> OnXThread is being used for methods that run on Thread X but this method is to push something to the main thread.

Fixed.

> > WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:134
> > +{
> Please consider adding asserts to verify that things are called on the correct thread:
>  ASSERT(isMainThread()); and ASSERT(m_worker->isContextThread());

Added.

> > WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:145
> > +    mayPostTaskToWorker(createCallbackTask(&didFailOnWorkerThread, this, error), mode);
> 
> I believe you can change s/this/this->m_selfRef.releaseRef()/ and the remove all of the clear calls.
> or you may only be able to do s/this/this->m_selfRef/ (and just have one clear call in mayPostTaskToWorker).  (I should make the former possible in the future if it isn't currently.)

Ah that's sweet.
Seems like the former doesn't work for now, so I changed the code to pass this->m_selfRef and left only one m_selfRef.clear in mayPostTaskToWorker.

[David Levin]

Comment on attachment 68467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68467&action=review

I have a few comments which you can address on landing.

Also, *please* get an ok from Darin Fisher on the change to WebKit/chromium/public/WebCommonWorkerClient.h before landing.

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:103
> +WorkerFileSystemCallbacksBridge::WorkerFileSystemCallbacksBridge(WebWorkerBase* worker, ScriptExecutionContext* scriptExecutionContext, WebFileSystemCallbacks* callbacks)

Consider moving this to be sorted in the same order as the header.

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:107
> +{

Please add  ASSERT(scriptExecutionContext->isContextThread());

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:110
> +WorkerFileSystemCallbacksBridge::~WorkerFileSystemCallbacksBridge()

Please move this if you move it in the header.

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:147
> +{

please add ASSERT(isMainThread());

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:152
> +{

please add ASSERT(isMainThread());

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:180
> +    MutexLocker locker(m_mutex);
> +    if (m_worker)
> +        m_worker->postTaskForModeToWorkerContext(task, mode);
> +    m_selfRef.clear();

Please fix this code like this:

 { // Let go of the mutex before possibly deleting this due to m_selfRef.clear().
     MutexLocker locker(m_mutex);
     if (m_worker)
         m_worker->postTaskForModeToWorkerContext(task, mode);
}
m_selfRef.clear();

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:50
> +struct WebFileInfo;

I would put struct after class (sorting the whole string).

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:73
> +    ~WorkerFileSystemCallbacksBridge();

fwiw, I would consider making this private and making ThreadSafeShared<WorkerFileSystemCallbacksBridge> a friend (like what is done in chromium for ref counted classes).

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:83
> +    // Method that posts an initial request task to the main thread. It is supposed to be called immediately after the bridge is constructed (it doesn't check if the context has been stopped or not).

Consider "constructed. (It doesn't ... not.)"

Also I'd remove "Method that" here.

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:103
> +    // Helper method.

Does this comment add anything? (If you want to add something information, fine, but this doesn't seem to be.)

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:106
> +    // m_selfRef keeps a reference to itself until callbacks are fired on the worker thread.

until tasks are created for the worker thread (at which point the tasks hold the reference).

[David Levin]

Comment on attachment 68467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68467&action=review


One more important change:

> WebCore/workers/WorkerContext.cpp:407
> +        (*iter)->notifyStop();
> +        (*iter)->stopObserving();

(I know I wrote this code but) It isn't quite correct. notifyStop may result in the object being deleted in which case calling stopObserving is really bad.

Instead I would suggest
  WorkerContext::Observer* observer = *iter;
  m_workerObservers.remove(iter); // It is ok to remove the observer directly because unregisterObserver is ok with the observer not being in m_workerObservers.
  observer->notifyStop();

[Darin Fisher (:fishd, Google)]

Comment on attachment 68467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68467&action=review

> WebKit/chromium/public/WebCommonWorkerClient.h:90
> +    virtual void openFileSystem(WebFileSystem::Type, long long size, WebFileSystemCallbacks*)

it is actually convention in the webkit API that methods, to be implemented by the embedder, have default implementations.

[Darin Fisher (:fishd, Google)]

(In reply to comment #37)
> (From update of attachment 68467 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68467&action=review
> 
> > WebKit/chromium/public/WebCommonWorkerClient.h:90
> > +    virtual void openFileSystem(WebFileSystem::Type, long long size, WebFileSystemCallbacks*)
> 
> it is actually convention in the webkit API that methods, to be implemented by the embedder, have default implementations.

This means that you can remove the FIXME comment, and i agree with this addition to WebCommonWorkerClient.  Thanks!

[Kinuko Yasuda]

Created attachment 68608 [details]
Patch

[Kinuko Yasuda]

Created attachment 68610 [details]
Patch

[David Levin]

Comment on attachment 68610 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68610&action=review

Two trivial suggestions.

> WebCore/workers/WorkerContext.h:157
> +        void registerObserver(Observer* observer);
> +        void unregisterObserver(Observer* observer);

Remove param name "observer".

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h:107
> +    // m_selfRef keeps a reference to itself until tasks are created for the worker thread (at which point the tasks hold the reference).

My typo s/tasks/task/

[Kinuko Yasuda]

Committed r68222: <http://trac.webkit.org/changeset/68222>