[Chromium] implementation of async FileWriter for workers

• Component: DOM
• Status: RESOLVED FIXED

[Eric U.]

This bug is just for the async interface, not FileWriterSync.

[Eric U.]

Created attachment 70788 [details]
Patch

[Eric U.]

I'm sorry this is so large; I hadn't realized it was going to be.

If you'd like I can pull out the addition of FileInfo and post that first.  However, that bit's pretty clear and separate already, so I'm not sure if that really makes the reviewing job much easier.

[Kinuko Yasuda]

Comment on attachment 70788 [details]
Patch

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

Made some early comments.  To me it's a bit unclear how bridge's reference is held in shutdown sequence.  Usually it seems to be held by WorkerAsyncFileWriter, but who holds it after the writer is destroyed?

> WebCore/fileapi/FileInfo.h:38
> +struct FileInfo {

Hmm what will the plan be about Metadata?  Are we going to keep both FileInfo and Metadata around?

> WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:139
> +class WorkerFileWriterHelperCallbacks : public AsyncFileSystemCallbacks {

It looks like FileWriterHelperCallbacks in AsyncFileSystemChromium can be also derived from AsyncFileSystemCallbacks and we could merge the two classes into one in a separate file?

Also if it derives from AsyncFileSystemCallbacks (but not from WebFileSystemCallbacks) it's going to be owned; it'd be better to have create() method that returns PassOwnPtr.

> WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:153
> +    }

nit: please insert empty line here

> WebKit/chromium/src/WorkerAsyncFileWriterChromium.cpp:63
> +    mode.append(String::number(workerContext->thread()->runLoop().createUniqueId()));

Later when we implement sync mode it'd be better to have different unique mode per operation (i.e. per granularity with which we want to wait for in sync case).

> WebKit/chromium/src/WorkerAsyncFileWriterChromium.h:57
> +    WorkerAsyncFileWriterChromium(WebKit::WebFileSystem*, const String& path, WorkerContext*, AsyncFileWriterClient*, bool synchronous);

ditto; would be better to have create() method that returns PassOwnPtr.

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:66
> +        Type newRef(bridge.get());

Isn't it enough to just pass bridge.get() to createCallbackTask?

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:102
> +    dispatchTaskToMainThread(createCallbackTask(&shutdownOnMainThread, bridge));

need to release the bridge here? (will it be the last reference to bridge?)

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:128
> +void WorkerFileWriterCallbacksBridge::didWrite(long long bytes, bool complete)

Who holds the bridge's reference after shutdown is called until all the didXxx fire?

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:69
> +class WorkerFileWriterCallbacksBridge : public ThreadSafeShared<WorkerFileWriterCallbacksBridge>, public WebCore::WorkerContext::Observer, public WebFileWriterClient {

nit: You may not need to call this 'Callbacks' bridge :)

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:115
> +    bool derefIfWorkerIsStopped();

Not needed?

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:136
> +    String m_mode;

m_path and m_mode are not thread safe, they're created on the worker thread and may later be released on the main thread?

[Eric U.]

(In reply to comment #3)
> (From update of attachment 70788 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70788&action=review
> 
> Made some early comments.  To me it's a bit unclear how bridge's reference is held in shutdown sequence.  Usually it seems to be held by WorkerAsyncFileWriter, but who holds it after the writer is destroyed?

The write passes off its last reference to shutdownOnMainThread, since the bridge needs to be destroyed on the main thread.  Any number of calls may have in-flight Tasks being posted between threads.  These also hold references.  I see that I have a bug here: a Task in flight to the worker context thread might actually hold the last reference; I'll have to work on that.

> > WebCore/fileapi/FileInfo.h:38
> > +struct FileInfo {
> 
> Hmm what will the plan be about Metadata?  Are we going to keep both FileInfo and Metadata around?

I see FileInfo as the internal data structure used to move info around, that is somewhat orthogonal to the DOM object that Metadata represents.  I think they should stay separate.  I'd be open to a different name if you think that would help.

> > WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:139
> > +class WorkerFileWriterHelperCallbacks : public AsyncFileSystemCallbacks {
> 
> It looks like FileWriterHelperCallbacks in AsyncFileSystemChromium can be also derived from AsyncFileSystemCallbacks and we could merge the two classes into one in a separate file?

That seems likely.  It would also make this changelist even larger, so I'd like to do that in a followup CL if that's OK.

> Also if it derives from AsyncFileSystemCallbacks (but not from WebFileSystemCallbacks) it's going to be owned; it'd be better to have create() method that returns PassOwnPtr.
> 
> > WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:153
> > +    }
> 
> nit: please insert empty line here

Done.

> > WebKit/chromium/src/WorkerAsyncFileWriterChromium.cpp:63
> > +    mode.append(String::number(workerContext->thread()->runLoop().createUniqueId()));
> 
> Later when we implement sync mode it'd be better to have different unique mode per operation (i.e. per granularity with which we want to wait for in sync case).

In sync mode, there can be only one operation going on at a time.  Unlike the FileSystem, which is shared, a FileWriter is tied to a single DOM object.

> > WebKit/chromium/src/WorkerAsyncFileWriterChromium.h:57
> > +    WorkerAsyncFileWriterChromium(WebKit::WebFileSystem*, const String& path, WorkerContext*, AsyncFileWriterClient*, bool synchronous);
> 
> ditto; would be better to have create() method that returns PassOwnPtr.

Done.

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:66
> > +        Type newRef(bridge.get());
> 
> Isn't it enough to just pass bridge.get() to createCallbackTask?

Yes--cleaned up.  Thanks!

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:102
> > +    dispatchTaskToMainThread(createCallbackTask(&shutdownOnMainThread, bridge));
> 
> need to release the bridge here? (will it be the last reference to bridge?)

It's a PassRefPtr, so it should be zeroed out by getting passed to the parameter.  The caller released its reference when calling postShutdownToMainThread.

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:128
> > +void WorkerFileWriterCallbacksBridge::didWrite(long long bytes, bool complete)
> 
> Who holds the bridge's reference after shutdown is called until all the didXxx fire?

Each task holds a reference.  As above, I need to make sure that the last one's on the main thread.

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:69
> > +class WorkerFileWriterCallbacksBridge : public ThreadSafeShared<WorkerFileWriterCallbacksBridge>, public WebCore::WorkerContext::Observer, public WebFileWriterClient {
> 
> nit: You may not need to call this 'Callbacks' bridge :)

Well, it does bridge the callbacks across threads, and it's parallel to the WorkerFileSystemCallbacksBridge.  I can change it if you think it's wrong, but I have a mild preference for it as it is.

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:115
> > +    bool derefIfWorkerIsStopped();
> 
> Not needed?

Yup; obsolete.  Deleted.

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:136
> > +    String m_mode;
> 
> m_path and m_mode are not thread safe, they're created on the worker thread and may later be released on the main thread?

Switched to threadsafeCopy() of inputs.

I'll post a new patch when I've fixed the shutdown issue.

[Eric U.]

(In reply to comment #3)
> (From update of attachment 70788 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70788&action=review
> 
> Made some early comments.  To me it's a bit unclear how bridge's reference is held in shutdown sequence.  Usually it seems to be held by WorkerAsyncFileWriter, but who holds it after the writer is destroyed?
> 
> > WebCore/fileapi/FileInfo.h:38
> > +struct FileInfo {
> 
> Hmm what will the plan be about Metadata?  Are we going to keep both FileInfo and Metadata around?
> 
> > WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:139
> > +class WorkerFileWriterHelperCallbacks : public AsyncFileSystemCallbacks {
> 
> It looks like FileWriterHelperCallbacks in AsyncFileSystemChromium can be also derived from AsyncFileSystemCallbacks and we could merge the two classes into one in a separate file?
> 
> Also if it derives from AsyncFileSystemCallbacks (but not from WebFileSystemCallbacks) it's going to be owned; it'd be better to have create() method that returns PassOwnPtr.
> 
> > WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:153
> > +    }
> 
> nit: please insert empty line here
> 
> > WebKit/chromium/src/WorkerAsyncFileWriterChromium.cpp:63
> > +    mode.append(String::number(workerContext->thread()->runLoop().createUniqueId()));
> 
> Later when we implement sync mode it'd be better to have different unique mode per operation (i.e. per granularity with which we want to wait for in sync case).
> 
> > WebKit/chromium/src/WorkerAsyncFileWriterChromium.h:57
> > +    WorkerAsyncFileWriterChromium(WebKit::WebFileSystem*, const String& path, WorkerContext*, AsyncFileWriterClient*, bool synchronous);
> 
> ditto; would be better to have create() method that returns PassOwnPtr.
> 
> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:66
> > +        Type newRef(bridge.get());
> 
> Isn't it enough to just pass bridge.get() to createCallbackTask?

Actually, that turns out not to work.  If you do that, the CrossThreadCopier just copies a bare pointer across, so when the PassRefPtr bridge gets deleted, it may give bridge its last deref, and it can be deleted before the PassRefPtr parameter to shutdownOnMainThread gets to ref it.

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:102
> > +    dispatchTaskToMainThread(createCallbackTask(&shutdownOnMainThread, bridge));
> 
> need to release the bridge here? (will it be the last reference to bridge?)
> 
> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:128
> > +void WorkerFileWriterCallbacksBridge::didWrite(long long bytes, bool complete)
> 
> Who holds the bridge's reference after shutdown is called until all the didXxx fire?
> 
> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:69
> > +class WorkerFileWriterCallbacksBridge : public ThreadSafeShared<WorkerFileWriterCallbacksBridge>, public WebCore::WorkerContext::Observer, public WebFileWriterClient {
> 
> nit: You may not need to call this 'Callbacks' bridge :)
> 
> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:115
> > +    bool derefIfWorkerIsStopped();
> 
> Not needed?
> 
> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:136
> > +    String m_mode;
> 
> m_path and m_mode are not thread safe, they're created on the worker thread and may later be released on the main thread?

[Eric U.]

Created attachment 70899 [details]
Patch

[Eric U.]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 70788 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=70788&action=review
> > 
> > Made some early comments.  To me it's a bit unclear how bridge's reference is held in shutdown sequence.  Usually it seems to be held by WorkerAsyncFileWriter, but who holds it after the writer is destroyed?
> 
> The write passes off its last reference to shutdownOnMainThread, since the bridge needs to be destroyed on the main thread.  Any number of calls may have in-flight Tasks being posted between threads.  These also hold references.  I see that I have a bug here: a Task in flight to the worker context thread might actually hold the last reference; I'll have to work on that.

I take that back; it's not a problem.  As long as m_writer is cleared on the Main thread, it doesn't matter which thread does the final deletion of the bridge object.  I've added comments to that effect.

> > > WebCore/fileapi/FileInfo.h:38
> > > +struct FileInfo {
> > 
> > Hmm what will the plan be about Metadata?  Are we going to keep both FileInfo and Metadata around?
> 
> I see FileInfo as the internal data structure used to move info around, that is somewhat orthogonal to the DOM object that Metadata represents.  I think they should stay separate.  I'd be open to a different name if you think that would help.
> 
> > > WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:139
> > > +class WorkerFileWriterHelperCallbacks : public AsyncFileSystemCallbacks {
> > 
> > It looks like FileWriterHelperCallbacks in AsyncFileSystemChromium can be also derived from AsyncFileSystemCallbacks and we could merge the two classes into one in a separate file?
> 
> That seems likely.  It would also make this changelist even larger, so I'd like to do that in a followup CL if that's OK.
> 
> > Also if it derives from AsyncFileSystemCallbacks (but not from WebFileSystemCallbacks) it's going to be owned; it'd be better to have create() method that returns PassOwnPtr.
> > 
> > > WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:153
> > > +    }
> > 
> > nit: please insert empty line here
> 
> Done.
> 
> > > WebKit/chromium/src/WorkerAsyncFileWriterChromium.cpp:63
> > > +    mode.append(String::number(workerContext->thread()->runLoop().createUniqueId()));
> > 
> > Later when we implement sync mode it'd be better to have different unique mode per operation (i.e. per granularity with which we want to wait for in sync case).
> 
> In sync mode, there can be only one operation going on at a time.  Unlike the FileSystem, which is shared, a FileWriter is tied to a single DOM object.
> 
> > > WebKit/chromium/src/WorkerAsyncFileWriterChromium.h:57
> > > +    WorkerAsyncFileWriterChromium(WebKit::WebFileSystem*, const String& path, WorkerContext*, AsyncFileWriterClient*, bool synchronous);
> > 
> > ditto; would be better to have create() method that returns PassOwnPtr.
> 
> Done.
> 
> > > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:66
> > > +        Type newRef(bridge.get());
> > 
> > Isn't it enough to just pass bridge.get() to createCallbackTask?
> 
> Yes--cleaned up.  Thanks!
> 
> > > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:102
> > > +    dispatchTaskToMainThread(createCallbackTask(&shutdownOnMainThread, bridge));
> > 
> > need to release the bridge here? (will it be the last reference to bridge?)
> 
> It's a PassRefPtr, so it should be zeroed out by getting passed to the parameter.  The caller released its reference when calling postShutdownToMainThread.
> 
> > > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:128
> > > +void WorkerFileWriterCallbacksBridge::didWrite(long long bytes, bool complete)
> > 
> > Who holds the bridge's reference after shutdown is called until all the didXxx fire?
> 
> Each task holds a reference.  As above, I need to make sure that the last one's on the main thread.
> 
> > > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:69
> > > +class WorkerFileWriterCallbacksBridge : public ThreadSafeShared<WorkerFileWriterCallbacksBridge>, public WebCore::WorkerContext::Observer, public WebFileWriterClient {
> > 
> > nit: You may not need to call this 'Callbacks' bridge :)
> 
> Well, it does bridge the callbacks across threads, and it's parallel to the WorkerFileSystemCallbacksBridge.  I can change it if you think it's wrong, but I have a mild preference for it as it is.
> 
> > > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:115
> > > +    bool derefIfWorkerIsStopped();
> > 
> > Not needed?
> 
> Yup; obsolete.  Deleted.
> 
> > > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:136
> > > +    String m_mode;
> > 
> > m_path and m_mode are not thread safe, they're created on the worker thread and may later be released on the main thread?
> 
> Switched to threadsafeCopy() of inputs.
> 
> I'll post a new patch when I've fixed the shutdown issue.

[Michael Nordman]

Comment on attachment 70899 [details]
Patch

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

> WebKit/chromium/src/AsyncFileWriterChromium.h:53
> +    AsyncFileWriterChromium(AsyncFileWriterClient*, WebKit::WebFileSystem*, const String& path);

Given the raw ptr, i was wondering about lifetimes. Instead of passing the raw ptr around, have you considered accessing as needed via...
webkitClient()->fileSystem()

[Eric U.]

(In reply to comment #8)
> (From update of attachment 70899 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70899&action=review
> 
> > WebKit/chromium/src/AsyncFileWriterChromium.h:53
> > +    AsyncFileWriterChromium(AsyncFileWriterClient*, WebKit::WebFileSystem*, const String& path);
> 
> Given the raw ptr, i was wondering about lifetimes. Instead of passing the raw ptr around, have you considered accessing as needed via...
> webkitClient()->fileSystem()

I could certainly do that, both in the worker and non-worker AsyncFileWriterClient.  So could {Worker}AsyncFileWriterChromium.  My reflex is not to use the globals, but it would save space and a little bit of code, and I'd guess it'd be about the same speed.  Is there a preferred webkit way of doing this?

[Michael Nordman]

Comment on attachment 70899 [details]
Patch

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

> WebKit/chromium/src/WorkerAsyncFileWriterChromium.cpp:60
> +    WebWorkerBase* worker = static_cast<WebWorkerBase*>(workerLoaderProxy);

Do you actually need the WebWorkerBase* or will the WorkerLoaderProxy interface satisfy the bridges needs, specifically a way to post tasks back to the worker thread.

> WebKit/chromium/src/WorkerAsyncFileWriterChromium.cpp:64
> +    m_bridge = WorkerFileWriterCallbacksBridge::create(webFileSystem, path, worker, workerContext, mode, client);

I think it may be worth trying to simplify this patch. The shutdown sequence in particular feels delicate. I'm familiar with another 'bridge' to the main thread with similar characterics (starts an async operation than may be canceled) that seems simpler, see WorkerThreadableLoader. It may be that this case is more complicated, if so why?

Ok, that's kind of vague feedback, but here's a concrete way to simplify things a little bit. Post a task to the main thread to 'init' the WebFileWriter in this ctor so you don't need to test for it prior to handling other tasks (the init task will have already run).

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:207
> +    m_worker->dispatchTaskToMainThread(createCallbackTask(&runTaskOnMainThread, this, task));

I think this is a static method call, maybe use WebWorkerBase::dispatchTaskToMainThread to clarify.

[Kinuko Yasuda]

Comment on attachment 70899 [details]
Patch

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

>> WebKit/chromium/src/WorkerAsyncFileWriterChromium.cpp:64
>> +    m_bridge = WorkerFileWriterCallbacksBridge::create(webFileSystem, path, worker, workerContext, mode, client);
> 
> I think it may be worth trying to simplify this patch. The shutdown sequence in particular feels delicate. I'm familiar with another 'bridge' to the main thread with similar characterics (starts an async operation than may be canceled) that seems simpler, see WorkerThreadableLoader. It may be that this case is more complicated, if so why?
> 
> Ok, that's kind of vague feedback, but here's a concrete way to simplify things a little bit. Post a task to the main thread to 'init' the WebFileWriter in this ctor so you don't need to test for it prior to handling other tasks (the init task will have already run).

(A bit tangent...)
I'd generally prefer having a separate class (maybe a subclass of WebFileWriterClient) for the things that are to be created and held solely on the main thread.  Then we could initialize it in bridge's ctor (or in its initialization method) and let it own WebFileWriter and a reference to the bridge.   Then we'd have two distinct WebFileWriterClient, one in the worker thread, the other one in the main thread. Both hold a ref to the bridge.  When one goes away it just tells the peer to go away.  No refptr passing except for the initialization phase.

>> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:207
>> +    m_worker->dispatchTaskToMainThread(createCallbackTask(&runTaskOnMainThread, this, task));
> 
> I think this is a static method call, maybe use WebWorkerBase::dispatchTaskToMainThread to clarify.

If you change this would you mind changing the similar line in WorkerFileSystemCallbacksBridge (and possibly AllowDatabaseMainThreadBridge in WebWorkerBase.cpp)?

[Eric U.]

(In reply to comment #10)
> (From update of attachment 70899 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70899&action=review
> 
> > WebKit/chromium/src/WorkerAsyncFileWriterChromium.cpp:60
> > +    WebWorkerBase* worker = static_cast<WebWorkerBase*>(workerLoaderProxy);
> 
> Do you actually need the WebWorkerBase* or will the WorkerLoaderProxy interface satisfy the bridges needs, specifically a way to post tasks back to the worker thread.

Actually, it looks like I don't; I'll take that out.

> > WebKit/chromium/src/WorkerAsyncFileWriterChromium.cpp:64
> > +    m_bridge = WorkerFileWriterCallbacksBridge::create(webFileSystem, path, worker, workerContext, mode, client);
> 
> I think it may be worth trying to simplify this patch. The shutdown sequence in particular feels delicate. I'm familiar with another 'bridge' to the main thread with similar characterics (starts an async operation than may be canceled) that seems simpler, see WorkerThreadableLoader. It may be that this case is more complicated, if so why?

The reason that WorkerThreadableLoader looks simpler is that it adds in another helper class [ThreadableLoaderClientWrapper] which acts as a refcounted weak pointer back to the main class.  So it makes the main class look a bit simpler by itself, but adds an extra class, and I feel like that makes it overall more complicated.  You have to read multiple files to understand it.

> Ok, that's kind of vague feedback, but here's a concrete way to simplify things a little bit. Post a task to the main thread to 'init' the WebFileWriter in this ctor so you don't need to test for it prior to handling other tasks (the init task will have already run).

This makes a lot of sense.  Let me do that and see how much clearer it gets.

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:207
> > +    m_worker->dispatchTaskToMainThread(createCallbackTask(&runTaskOnMainThread, this, task));
> 
> I think this is a static method call, maybe use WebWorkerBase::dispatchTaskToMainThread to clarify.

Done, thanks.

[David Levin]

Comment on attachment 70899 [details]
Patch

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

Meta comment:
Pulling things into smaller patches is better.

You get things in faster because the reviewer can often approve simple things quickly and the complicated things get the attention they need without the added distraction of other stuff.

Also when you go through multiple rounds of reviews, it means that the reviewer needs to look over a bunch of cruft everytime that while simple -- still needs to be looked at everytime. (We don't have nice intra-patch diffing in WebKit land).

So please consider making patches as small as possible and breaking up independent pieces.

Initial brief comments on the patch.

> WebCore/fileapi/FileInfo.h:38
> +struct FileInfo {

WebKit tries to avoid abbreviations "Info".

FileMetadata ?

> WebCore/fileapi/FileInfo.h:62
> +#endif // DOMFileSystem_h

This comment is incorrect.

> WebCore/fileapi/FileSystemCallbacks.h:37
> +#include "FileInfo.h"

Don't include. Use a fwd decl.

> WebCore/platform/AsyncFileSystemCallbacks.h:36
> +#include "FileInfo.h"

Avoid include. Use fwd decl.

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:69
> +class WorkerFileWriterCallbacksBridge : public ThreadSafeShared<WorkerFileWriterCallbacksBridge>, public WebCore::WorkerContext::Observer, public WebFileWriterClient {

Any ThreadSafeShared class that owns strings bothers me.

ThreadSafeShared basically means that it can be destructed on multiple threads.

String is not designed to be used on multiple threads. It takes very very careful coding to get this right.

Then even if you get it right, any maintainance patches need to get it right (and they won't because it is too subtle).

In short, I think this is fundamentally flawed.

[Eric U.]

(In reply to comment #11)
> (From update of attachment 70899 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70899&action=review
> 
> >> WebKit/chromium/src/WorkerAsyncFileWriterChromium.cpp:64
> >> +    m_bridge = WorkerFileWriterCallbacksBridge::create(webFileSystem, path, worker, workerContext, mode, client);
> > 
> > I think it may be worth trying to simplify this patch. The shutdown sequence in particular feels delicate. I'm familiar with another 'bridge' to the main thread with similar characterics (starts an async operation than may be canceled) that seems simpler, see WorkerThreadableLoader. It may be that this case is more complicated, if so why?
> > 
> > Ok, that's kind of vague feedback, but here's a concrete way to simplify things a little bit. Post a task to the main thread to 'init' the WebFileWriter in this ctor so you don't need to test for it prior to handling other tasks (the init task will have already run).
> 
> (A bit tangent...)
> I'd generally prefer having a separate class (maybe a subclass of WebFileWriterClient) for the things that are to be created and held solely on the main thread.  Then we could initialize it in bridge's ctor (or in its initialization method) and let it own WebFileWriter and a reference to the bridge.   Then we'd have two distinct WebFileWriterClient, one in the worker thread, the other one in the main thread. Both hold a ref to the bridge.  When one goes away it just tells the peer to go away.  No refptr passing except for the initialization phase.

I looked into this a bit, but I'm not sure that anything along these lines will actually make things significantly simpler.  The bridge would look pretty much the same except for shutdown.

The Tasks passed in each direction through the bridge still need to have RefPtrs to it, so that it won't vanish out from under them.  The main thread still needs to use the WorkerLoaderProxy to post tasks to the context thread, but that should be created on the context thread [I'm not sure if it needs to be destroyed there--I'll look into that].

I think I've managed a good bit of simplification without the split you're describing.  I'll post it in a bit, and please let me know if you still think I should try adding another wrapper class.

> >> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:207
> >> +    m_worker->dispatchTaskToMainThread(createCallbackTask(&runTaskOnMainThread, this, task));
> > 
> > I think this is a static method call, maybe use WebWorkerBase::dispatchTaskToMainThread to clarify.
> 
> If you change this would you mind changing the similar line in WorkerFileSystemCallbacksBridge (and possibly AllowDatabaseMainThreadBridge in WebWorkerBase.cpp)?

Can do; I'll put that in a separate cleanup patch, though, along with the AsyncFileWriterChromium stuff I'm going to drop out of this one for size.

[Eric U.]

(In reply to comment #13)
> (From update of attachment 70899 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70899&action=review
> 
> Meta comment:
> Pulling things into smaller patches is better.
> 
> You get things in faster because the reviewer can often approve simple things quickly and the complicated things get the attention they need without the added distraction of other stuff.
> 
> Also when you go through multiple rounds of reviews, it means that the reviewer needs to look over a bunch of cruft everytime that while simple -- still needs to be looked at everytime. (We don't have nice intra-patch diffing in WebKit land).
> 
> So please consider making patches as small as possible and breaking up independent pieces.

I've split off the AsyncFileWriterChromium cleanup stuff into a separate patch; I'll log another bug for it.

> Initial brief comments on the patch.
> 
> > WebCore/fileapi/FileInfo.h:38
> > +struct FileInfo {
> 
> WebKit tries to avoid abbreviations "Info".
> 
> FileMetadata ?
> 
> > WebCore/fileapi/FileInfo.h:62
> > +#endif // DOMFileSystem_h
> 
> This comment is incorrect.
> 
> > WebCore/fileapi/FileSystemCallbacks.h:37
> > +#include "FileInfo.h"
> 
> Don't include. Use a fwd decl.
> 
> > WebCore/platform/AsyncFileSystemCallbacks.h:36
> > +#include "FileInfo.h"
> 
> Avoid include. Use fwd decl.
> 
> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:69
> > +class WorkerFileWriterCallbacksBridge : public ThreadSafeShared<WorkerFileWriterCallbacksBridge>, public WebCore::WorkerContext::Observer, public WebFileWriterClient {
> 
> Any ThreadSafeShared class that owns strings bothers me.
> 
> ThreadSafeShared basically means that it can be destructed on multiple threads.
> 
> String is not designed to be used on multiple threads. It takes very very careful coding to get this right.
> 
> Then even if you get it right, any maintainance patches need to get it right (and they won't because it is too subtle).
> 
> In short, I think this is fundamentally flawed.

As a matter of fact, I'm about to post a cleanup that does away with all stored strings.  I'll roll your other comments in first, though.  Thanks!

[Eric U.]

Created attachment 71339 [details]
Rolled in feedback, removed AsyncFileWriter changes.

I'm afraid this is still quite large, even after I dropped out the AsyncFileWriter cleanup changes.  But a fair bit of it is mechanical: the switchover to FileMetadata and associated build files.  And the bridge code should be much simpler now.

[WebKit Review Bot]

Attachment 71339 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric U.]

Created attachment 71362 [details]
Fixed include order.

[Michael Nordman]

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

> WebKit/chromium/src/WebFileSystemCallbacksImpl.cpp:74
> +        fileMetadata.type = FileMetadata::TypeDirectory;

In many cases we arrange to have the webcore enum values equate to the webkit enum values so all we need are static_casts<> in this layer. There are COMPILE_TIME_ASSERTS to ensure the values are actually equal, see AssertMatchingEnums.cpp. Can we do that here too?

> WebKit/chromium/src/WorkerAsyncFileWriterChromium.cpp:67
> +    bridge->postShutdownToMainThread(m_bridge.release());

Is the handoff of the 'last reference' needed, would a typical method call (like the others) followed by the RefPtr going out of scope suffice?

> WebKit/chromium/src/WorkerAsyncFileWriterChromium.h:60
> +    WebKit::WebFileWriterClient* getMainThreadClient();

where is this method used?

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:52
> +template<> struct CrossThreadCopierBase<false, false, WebKit::WebURL> {

Would it make sense to pass a KURL instead of a WebURL in the postWriteToMainThread? I'm guessing there is a KURL cross thread copier defined already.

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:104
> +    ASSERT(!m_clientOnWorkerThread);

I don't understand this assertion. The data member is reset only when notifyStop() is called, which according to the comment is a WorkerContext::Observer override. Do file writers only go away during worker context shutdown?

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:158
> +    postInitToMainThread(path);

I'm not sure this is safe to call from within the ctor? What is the value of the refcount at this point? It the task wrappers involve ref'ing and deref'ing the bridge. Depending on when threads run when, is it possible that the object will be deleted prior to returning from the ctor and being placed into the callers RefPtr?

This pattern was a source of some bugs in chrome. Consider having the factory method schedule the init on the main thread after having safely taken a reference.

[Eric U.]

(In reply to comment #19)
> View in context: https://bugs.webkit.org/attachment.cgi?id=71339&action=review
> 
> > WebKit/chromium/src/WebFileSystemCallbacksImpl.cpp:74
> > +        fileMetadata.type = FileMetadata::TypeDirectory;
> 
> In many cases we arrange to have the webcore enum values equate to the webkit enum values so all we need are static_casts<> in this layer. There are COMPILE_TIME_ASSERTS to ensure the values are actually equal, see AssertMatchingEnums.cpp. Can we do that here too?

Very neat.  Done.

> > WebKit/chromium/src/WorkerAsyncFileWriterChromium.cpp:67
> > +    bridge->postShutdownToMainThread(m_bridge.release());
> 
> Is the handoff of the 'last reference' needed, would a typical method call (like the others) followed by the RefPtr going out of scope suffice?

You're right.  With the current organization, it's safe for the actual deletion to happen on any thread, as long as this gets called first.  I've fixed up the comments and made the caller's code simpler.

> > WebKit/chromium/src/WorkerAsyncFileWriterChromium.h:60
> > +    WebKit::WebFileWriterClient* getMainThreadClient();
> 
> where is this method used?

Nowhere.  Removed.

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:52
> > +template<> struct CrossThreadCopierBase<false, false, WebKit::WebURL> {
> 
> Would it make sense to pass a KURL instead of a WebURL in the postWriteToMainThread? I'm guessing there is a KURL cross thread copier defined already.

I'll talk to you offline about this.

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:104
> > +    ASSERT(!m_clientOnWorkerThread);
> 
> I don't understand this assertion. The data member is reset only when notifyStop() is called, which according to the comment is a WorkerContext::Observer override. Do file writers only go away during worker context shutdown?

No, that's a bug.  Fixed.

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:158
> > +    postInitToMainThread(path);
> 
> I'm not sure this is safe to call from within the ctor? What is the value of the refcount at this point? It the task wrappers involve ref'ing and deref'ing the bridge. Depending on when threads run when, is it possible that the object will be deleted prior to returning from the ctor and being placed into the callers RefPtr?

The parent class ThreadSafeShared starts with a refcount of 1, so this should be perfectly safe.  The adoptRef called in the factory method is adopting that initial reference.

> This pattern was a source of some bugs in chrome. Consider having the factory method schedule the init on the main thread after having safely taken a reference.

Since this is both safe and part of constructing a viable instance, I think it's better in the constructor.

[Eric U.]

Created attachment 71378 [details]
Rolled in Michael's comments, including the KURL change.

[David Levin]

Comment on attachment 71378 [details]
Rolled in Michael's comments, including the KURL change.

I'm not sure why this is up for review. The strings are still in the ThreadSafeShared class.

I could point out nits but that code may change.

I could try to understand the structure and point out issues there but it is hard for me to know what is going to change (due to those strings).

btw, I'd recommend make the fileInfo change as a separate patch. It looks independent and something that could get r+'ed quickly.

r- due to the strings in the ThreadSafeShared class (and not wanting to spend time diving into code that may change a fair bit due to this).

[Eric U.]

(In reply to comment #22)
> (From update of attachment 71378 [details])
> I'm not sure why this is up for review. The strings are still in the ThreadSafeShared class.
> 
> I could point out nits but that code may change.
> 
> I could try to understand the structure and point out issues there but it is hard for me to know what is going to change (due to those strings).
> 
> btw, I'd recommend make the fileInfo change as a separate patch. It looks independent and something that could get r+'ed quickly.
> 
> r- due to the strings in the ThreadSafeShared class (and not wanting to spend time diving into code that may change a fair bit due to this).

David:  Many apologies for your wasted time.  I apparently managed to grab an old patch by accident when uploading this time [from back before your first set of comments].  I'll try again tomorrow.

[Eric U.]

Created attachment 72273 [details]
Patch

[David Levin]

Comment on attachment 72273 [details]
Patch

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

Some quick 1st pass comments. (I would save these as a draft until done but that feature doesn't exist.)

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:52
> +template<> struct CrossThreadCopierBase<false, false, PassRefPtr<WebKit::WorkerFileWriterCallbacksBridge> > {

I think the real problem is that the CrossThreadCopier doesn't recognize ThreadSafeShared class which are passed in a PassRefPtr.

Please add the following to CrossThreadCopier.h and remove this specialization.

    template<typename T> struct CrossThreadCopier : public CrossThreadCopierBase<WTF::IsConvertibleToInteger<T>::value,
                                                                                 WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, PassRefPtr>::Type, ThreadSafeShared>::value,
                                                                                 T> {
    };

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:63
> +using namespace WebCore;

Since you have this, s/WebCore::// throughout the rest of this file.

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:89
> +        PassRefPtr<WorkerFileWriterCallbacksBridge> bridge)

Why is this wrapped? (And if you must then perhaps only 4 spaces for this indent).

[Eric U.]

(In reply to comment #25)
> (From update of attachment 72273 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72273&action=review
> 
> Some quick 1st pass comments. (I would save these as a draft until done but that feature doesn't exist.)
> 
> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:52
> > +template<> struct CrossThreadCopierBase<false, false, PassRefPtr<WebKit::WorkerFileWriterCallbacksBridge> > {
> 
> I think the real problem is that the CrossThreadCopier doesn't recognize ThreadSafeShared class which are passed in a PassRefPtr.
> 
> Please add the following to CrossThreadCopier.h and remove this specialization.
> 
>     template<typename T> struct CrossThreadCopier : public CrossThreadCopierBase<WTF::IsConvertibleToInteger<T>::value,
>                                                                                  WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, PassRefPtr>::Type, ThreadSafeShared>::value,
>                                                                                  T> {
>     };

That gives me: "error: redefinition of 'struct WebCore::CrossThreadCopier<T>'".
These templates are a bit beyond me...I'm not sure what to do here.
You're talking about adding this next text, not replacing the version that uses RefPtr instead of your PassRefPtr?

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:63
> > +using namespace WebCore;
> 
> Since you have this, s/WebCore::// throughout the rest of this file.

Done.

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:89
> > +        PassRefPtr<WorkerFileWriterCallbacksBridge> bridge)
> 
> Why is this wrapped? (And if you must then perhaps only 4 spaces for this indent).

Typo; I've unwrapped it.  I'll hold off on re-uploading until we resolve the template issue.

[Eric U.]

(In reply to comment #26)
> (In reply to comment #25)
> > (From update of attachment 72273 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=72273&action=review
> > 
> > Some quick 1st pass comments. (I would save these as a draft until done but that feature doesn't exist.)
> > 
> > > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:52
> > > +template<> struct CrossThreadCopierBase<false, false, PassRefPtr<WebKit::WorkerFileWriterCallbacksBridge> > {
> > 
> > I think the real problem is that the CrossThreadCopier doesn't recognize ThreadSafeShared class which are passed in a PassRefPtr.
> > 
> > Please add the following to CrossThreadCopier.h and remove this specialization.
> > 
> >     template<typename T> struct CrossThreadCopier : public CrossThreadCopierBase<WTF::IsConvertibleToInteger<T>::value,
> >                                                                                  WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, PassRefPtr>::Type, ThreadSafeShared>::value,
> >                                                                                  T> {
> >     };
> 
> That gives me: "error: redefinition of 'struct WebCore::CrossThreadCopier<T>'".
> These templates are a bit beyond me...I'm not sure what to do here.
> You're talking about adding this next text, not replacing the version that uses RefPtr instead of your PassRefPtr?

s/next/new/

> 
> > > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:63
> > > +using namespace WebCore;
> > 
> > Since you have this, s/WebCore::// throughout the rest of this file.
> 
> Done.
> 
> > > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:89
> > > +        PassRefPtr<WorkerFileWriterCallbacksBridge> bridge)
> > 
> > Why is this wrapped? (And if you must then perhaps only 4 spaces for this indent).
> 
> Typo; I've unwrapped it.  I'll hold off on re-uploading until we resolve the template issue.

[Eric U.]

Created attachment 73436 [details]
Patch

[Eric U.]

(In reply to comment #28)
> Created an attachment (id=73436) [details]
> Patch

Uploaded anyway, just to keep line numbers current for your next pass.

[David Levin]

Comment on attachment 73436 [details]
Patch

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

This looks close. It may need a few changes or at least a few more comments.

And it would b good to include the generic version of the template instead of this specific instance. I'll tell you what to change in another note and explain it as well.

> WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:158
> +            OwnPtr<WorkerAsyncFileWriterChromium> asyncFileWriterChromium = adoptPtr(new WorkerAsyncFileWriterChromium(m_webFileSystem, m_path, m_workerContext, m_client, false));

Typically you'd hide the constructor for WorkerAsyncFileWriterChromium and only expose a create method which returned a PassOwnPtr. (The adoptPtr would only be used in the create method.) Everytime I see it outside of a "create" method it looks wrong (and mildly is).

> WebKit/chromium/src/WorkerAsyncFileWriterChromium.cpp:53
> +static const char fileWriterOperationsMode[] = "fileWriterOperationsMode";

This is unused. Please remove it for now (so when you add it, I'll be more likely to notice the sync handling code as well).

> WebKit/chromium/src/WorkerAsyncFileWriterChromium.cpp:58
> +    ASSERT(!synchronous); // Not implemented yet.

#include <wtf/Assertions.h> (above).

> WebKit/chromium/src/WorkerAsyncFileWriterChromium.cpp:84
> +} // namespace

If you include // namespace, add the namespace name, but I think the preference is just to not do this at all.

> WebKit/chromium/src/WorkerAsyncFileWriterChromium.h:43
> +class WebFileSystem;

Please indent (4 spaces).

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:159
> +void WorkerFileWriterCallbacksBridge::didWriteOnWorkerThread(ScriptExecutionContext*, WorkerFileWriterCallbacksBridge* bridge, long long length, bool complete)

How do you know that bridge is still alive/ok to use at this point?

(Add a comment somewhere. Likely at the class level to explain how the lifetime works.)

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:161
> +    bridge->m_clientOnWorkerThread->didWrite(length, complete);

How do you know that m_clientOnWorkerThread is still alive/ok to use?

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:69
> +//      This calls the original client (m_clientOnWorkerThread).

A very important part about this description would be something to indicate lifetimes of objects/when things die or are known to be alive and how the lifetimes relate.

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:110
> +    // They release a selfRef of the WorkerFileWriterCallbacksBridge.

I don't see the release of the selfRef.

[David Levin]

Here's the code change needed:

diff --git a/WebCore/platform/CrossThreadCopier.h b/WebCore/platform/CrossThreadCopier.h
index 6f7bb25..5091b3e 100644
--- a/WebCore/platform/CrossThreadCopier.h
+++ b/WebCore/platform/CrossThreadCopier.h
@@ -71,7 +71,7 @@ namespace WebCore {
 
     // Custom copy methods.
     template<typename T> struct CrossThreadCopierBase<false, true, T> {
-        typedef typename WTF::RemoveTemplate<T, RefPtr>::Type RefCountedType;
+        typedef typename WTF::RemoveTemplate<typename WTF::RemoveTemplate<T, RefPtr>::Type, PassRefPtr>::Type RefCountedType;
         typedef PassRefPtr<RefCountedType> Type;
         static Type copy(const T& refPtr)
         {
@@ -113,7 +113,8 @@ namespace WebCore {
     };
 
     template<typename T> struct CrossThreadCopier : public CrossThreadCopierBase<WTF::IsConvertibleToInteger<T>::value,
-                                                                                 WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, RefPtr>::Type, ThreadSafeShared>::value,
+                                                                                 WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, RefPtr>::Type, ThreadSafeShared>::value
+                                                                                 || WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, PassRefPtr>::Type, ThreadSafeShared>::value,
                                                                                  T> {
     };




Explanation:

The change in CrossThreadCopier simply adds this line
   WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, PassRefPtr>::Type, ThreadSafeShared>::value

This is similar to the previous line with PassRefPtr substituted for RefPtr. Here's the clause broken down:

   "typename WTF::RemoveTemplate<T, PassRefPtr>::Type" says take the given type and remove "PassRefPtr" from around it (if it is there). Then give me that type.

 so if T is PassRefPtr<Foo>, we get

   WTF::IsSubclassOfTemplate<Foo, ThreadSafeShared>::value

Now it says does Foo derive from the template ThreadSafeShared? (Think "class Foo : public ThreadSafeShared<Foo>", but this would also be true for "class Foo : public ThreadSafeShared<Bar>".)

This gets or'ed with the previous expression so the second parameter to CrossThreadCopierBase is the OR of these two clauses.

Alternately, I could have consolidated them like this:
    WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<typename WTF::RemoveTemplate<T, RefPtr>::Type, PassRefPtr>::Type, ThreadSafeShared>::value
but that is completely unreadable (imo) as opposed pretty hard to read.



For this change:
-        typedef typename WTF::RemoveTemplate<T, RefPtr>::Type RefCountedType;
+        typedef typename WTF::RemoveTemplate<typename WTF::RemoveTemplate<T, RefPtr>::Type, PassRefPtr>::Type RefCountedType;
Thinking about this more, it is approaching being unreadable. I would break it up into two lines like this (which I just tried and it worked):

  typedef typename WTF::RemoveTemplate<T, RefPtr>::Type TypeWithoutRefPtr;
  typedef typename WTF::RemoveTemplate<TypeWithoutRefPtr, PassRefPtr>::Type RefCountedType;

The first line is like it was before and it converts RefPtr<Foo> to Foo (and Foo to Foo).
The second line does the same for PassRefPtr.

The definitions for all of these things are in JavaScriptCore/wtf/TypeTraits.h They will bend your brain a little bit but hopefully in a fun way and if work to understand them, you'll have a very good understanding of templates (imo).

btw, although I made this sound easy, it did take me a little bit to modify this code appropriately. (I missed the change to CrossThreadCopierBase and I got one of those famous template errors that is very hard to read.)

[Eric U.]

(In reply to comment #30)
> (From update of attachment 73436 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73436&action=review
> 
> This looks close. It may need a few changes or at least a few more comments.
> 
> And it would b good to include the generic version of the template instead of this specific instance. I'll tell you what to change in another note and explain it as well.

Done.  Thanks for the explanation.  I need to go read it about 3 more times.

> > WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:158
> > +            OwnPtr<WorkerAsyncFileWriterChromium> asyncFileWriterChromium = adoptPtr(new WorkerAsyncFileWriterChromium(m_webFileSystem, m_path, m_workerContext, m_client, false));
> 
> Typically you'd hide the constructor for WorkerAsyncFileWriterChromium and only expose a create method which returned a PassOwnPtr. (The adoptPtr would only be used in the create method.) Everytime I see it outside of a "create" method it looks wrong (and mildly is).

Done.

> > WebKit/chromium/src/WorkerAsyncFileWriterChromium.cpp:53
> > +static const char fileWriterOperationsMode[] = "fileWriterOperationsMode";
> 
> This is unused. Please remove it for now (so when you add it, I'll be more likely to notice the sync handling code as well).

Removed.

> > WebKit/chromium/src/WorkerAsyncFileWriterChromium.cpp:58
> > +    ASSERT(!synchronous); // Not implemented yet.
> 
> #include <wtf/Assertions.h> (above).

Added.

> > WebKit/chromium/src/WorkerAsyncFileWriterChromium.cpp:84
> > +} // namespace
> 
> If you include // namespace, add the namespace name, but I think the preference is just to not do this at all.

Removed.

> > WebKit/chromium/src/WorkerAsyncFileWriterChromium.h:43
> > +class WebFileSystem;
> 
> Please indent (4 spaces).

Done.

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:159
> > +void WorkerFileWriterCallbacksBridge::didWriteOnWorkerThread(ScriptExecutionContext*, WorkerFileWriterCallbacksBridge* bridge, long long length, bool complete)
> 
> How do you know that bridge is still alive/ok to use at this point?
> 
> (Add a comment somewhere. Likely at the class level to explain how the lifetime works.)

Switched to a PassRefPtr instead.

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:161
> > +    bridge->m_clientOnWorkerThread->didWrite(length, complete);
> 
> How do you know that m_clientOnWorkerThread is still alive/ok to use?

Commented.

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:69
> > +//      This calls the original client (m_clientOnWorkerThread).
> 
> A very important part about this description would be something to indicate lifetimes of objects/when things die or are known to be alive and how the lifetimes relate.

Added a paragraph.

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:110
> > +    // They release a selfRef of the WorkerFileWriterCallbacksBridge.
> 
> I don't see the release of the selfRef.

Deleted the obsolete comment.

[Eric U.]

Created attachment 73680 [details]
Patch

[Eric U.]

Comment on attachment 73680 [details]
Patch

Kinuko just pointed out an improvement; I'll upload a patch in a few minutes.

[David Levin]

(I tend to do my code reviews in multiple passes.)

These are comments from my "nit" pass. I won't r- for any of these, but I hope you will fix them before landing the patch.

Now, I'm beginning my "overview" pass where I will work to understand the patch, the flow of the data and object lifetimes.

> WebKit/chromium/src/WorkerAsyncFileWriterChromium.h:37
> +#include "WebFileError.h"

Do you need this include? (Please use fwd declarations whenever possible.)

> WebKit/chromium/src/WorkerAsyncFileWriterChromium.h:38
> +#include "WebFileWriterClient.h"

Do you need this include?

> WebKit/chromium/src/WorkerAsyncFileWriterChromium.h:39
> +#include <wtf/OwnPtr.h>

Do you need this include?

> WebKit/chromium/src/WorkerAsyncFileWriterChromium.h:57
> +    static PassOwnPtr<WorkerAsyncFileWriterChromium> create(WebKit::WebFileSystem* webFileSystem, const String& path, WorkerContext* workerContext, AsyncFileWriterClient* client, bool synchronous)

Since the callers pass in a true/false value for synchronous (as opposed to a variable name), it is pretty opaque at the calling site what the parameter is about.

In these cases, WebKit style is to use an enum instead of a bool (which you may convert to an bool inside the constructor).

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:36
> +#include "PlatformString.h"

Not needed. Used fwd decl.

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:37
> +#include "WebFileError.h"

Do you need this include?

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:42
> +#include <wtf/Threading.h>

Use <wtf/ThreadSafeShared.h> instead.

[David Levin]

Comment on attachment 73680 [details]
Patch

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

OK this looks good. The lifetimes look good and the functionality seems fine.

Look forward to seeing the final patch you put up.

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:100
> +    bridge->m_writer = adoptPtr(webKitClient()->fileSystem()->createFileWriter(path, bridge.get()));

Why are we doing adoptPtr here? (Always a bad sign outside of a ::create method and outside of the class itself).

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:146
> +// We know m_clientOnWorkerThread is still valid because it is only cleared on the context thread, and because we check in runTaskOnMainThread before calling any of these methods.

s/runTaskOnMainThread/runTaskOnWorkerThread/

Ah I suggested this type of thing in the last bridge that I reviewed, so I should have seen it but I'm glad you added this comment because it helped me to look in the correct place much faster and now I get it.

Basically runTaskOnMainThread and runTaskOnWorkerThread guard against running after shutdown has been started by checking the variables set in shutdownOnMainThread and postShutdownToMainThread respectively (which are run on the same thread).

These bridges are starting to follow a pattern. If you think of a way of pulling out this common pattern, that would be cool (but doesn't block this change at all). Just something that we can think about.

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:59
> +// the context thread.  The responses through the WebFileWriterClient interface

Single spaces after . in comments is typical WebKit style (though not documented currently).

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:71
> +// The bridge object is refcounted, so that it doesn't get deleted while there

Now it does :) and that makes it a lot easier to check that it is correct.

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:123
> +    void initWebFileWriter();

This doesn't seem to exist.

[David Levin]

Comment on attachment 73680 [details]
Patch

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

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:188
> +    m_proxy->postTaskForModeToWorkerContext(createCallbackTask(&runTaskOnWorkerThread, this, task), "");

s/""/WorkerRunLoop::defaultMode()/

[Eric U.]

(In reply to comment #35)
> (I tend to do my code reviews in multiple passes.)
> 
> These are comments from my "nit" pass. I won't r- for any of these, but I hope you will fix them before landing the patch.
> 
> Now, I'm beginning my "overview" pass where I will work to understand the patch, the flow of the data and object lifetimes.
> 
> > WebKit/chromium/src/WorkerAsyncFileWriterChromium.h:37
> > +#include "WebFileError.h"
> 
> Do you need this include? (Please use fwd declarations whenever possible.)
> 
> > WebKit/chromium/src/WorkerAsyncFileWriterChromium.h:38
> > +#include "WebFileWriterClient.h"
> 
> Do you need this include?
> 
> > WebKit/chromium/src/WorkerAsyncFileWriterChromium.h:39
> > +#include <wtf/OwnPtr.h>
> 
> Do you need this include?

Removed a bunch of includes.

> > WebKit/chromium/src/WorkerAsyncFileWriterChromium.h:57
> > +    static PassOwnPtr<WorkerAsyncFileWriterChromium> create(WebKit::WebFileSystem* webFileSystem, const String& path, WorkerContext* workerContext, AsyncFileWriterClient* client, bool synchronous)
> 
> Since the callers pass in a true/false value for synchronous (as opposed to a variable name), it is pretty opaque at the calling site what the parameter is about.
> 
> In these cases, WebKit style is to use an enum instead of a bool (which you may convert to an bool inside the constructor).

Changed to enum.

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:36
> > +#include "PlatformString.h"
> 
> Not needed. Used fwd decl.

Done.

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:37
> > +#include "WebFileError.h"
> 
> Do you need this include?

Yes; it's an enum, not a class.

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:42
> > +#include <wtf/Threading.h>
> 
> Use <wtf/ThreadSafeShared.h> instead.

Done.

[Eric U.]

(In reply to comment #36)
> (From update of attachment 73680 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73680&action=review
> 
> OK this looks good. The lifetimes look good and the functionality seems fine.
> 
> Look forward to seeing the final patch you put up.
> 
> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:100
> > +    bridge->m_writer = adoptPtr(webKitClient()->fileSystem()->createFileWriter(path, bridge.get()));
> 
> Why are we doing adoptPtr here? (Always a bad sign outside of a ::create method and outside of the class itself).

It was a bug; removed.

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:146
> > +// We know m_clientOnWorkerThread is still valid because it is only cleared on the context thread, and because we check in runTaskOnMainThread before calling any of these methods.
> 
> s/runTaskOnMainThread/runTaskOnWorkerThread/

Fixed.

> Ah I suggested this type of thing in the last bridge that I reviewed, so I should have seen it but I'm glad you added this comment because it helped me to look in the correct place much faster and now I get it.
> 
> Basically runTaskOnMainThread and runTaskOnWorkerThread guard against running after shutdown has been started by checking the variables set in shutdownOnMainThread and postShutdownToMainThread respectively (which are run on the same thread).

Yup.

> These bridges are starting to follow a pattern. If you think of a way of pulling out this common pattern, that would be cool (but doesn't block this change at all). Just something that we can think about.

Hmm...I'll ponder it, but nothing immediately comes to mind.

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:59
> > +// the context thread.  The responses through the WebFileWriterClient interface
> 
> Single spaces after . in comments is typical WebKit style (though not documented currently).

Done.

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:71
> > +// The bridge object is refcounted, so that it doesn't get deleted while there
> 
> Now it does :) and that makes it a lot easier to check that it is correct.

Uh, you mean "now it doesn't", right?

> > WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:123
> > +    void initWebFileWriter();
> 
> This doesn't seem to exist.

Removed.

[Eric U.]

Created attachment 74061 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 74061 [details]
Patch

Clearing flags on attachment: 74061

Committed r72200: <http://trac.webkit.org/changeset/72200>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.