A moment ago (in https://bugs.webkit.org/show_bug.cgi?id=30920) I fixed a bug in LocalStorageThread, but I'm still not very happy with the way the class looks. It's pretty confusing how it holds onto a reference of itself, has unnecessary locks, detaches the thread when it could just join on it, etc. Let's try to clean things up.
Created attachment 42161 [details] Patch v1
Comment on attachment 42161 [details] Patch v1 > + // Even in weird, exceptional cases, don't wait on a non-existant thread to terminate. Spelling error here. It's "nonexistent" rather than "non-existant". > + void *returnValue; WebKit coding style is void* for this, not void *. > // FIXME: Rename this class to StorageThread > - class LocalStorageThread : public ThreadSafeShared<LocalStorageThread> { > + class LocalStorageThread { Should derive from Noncopyable for multiple reasons. One is that it will make this derive from FastAllocBase and save some work on the FastAllocBase. By the way, you could still have made this have a create function and a private constructor, just using PassOwnPtr instead of PassRefPtr. The benefit would be that we'd avoid having a "bare new" outside a create function. Might be good long term to have that style. Otherwise, looks good. I'm going to say review- because I think the Noncopyable is fairly important.
Created attachment 42162 [details] Patch v1
(In reply to comment #2) > (From update of attachment 42161 [details]) > > + // Even in weird, exceptional cases, don't wait on a non-existant thread to terminate. > > Spelling error here. It's "nonexistent" rather than "non-existant". > > > + void *returnValue; > > WebKit coding style is void* for this, not void *. > > > // FIXME: Rename this class to StorageThread > > - class LocalStorageThread : public ThreadSafeShared<LocalStorageThread> { > > + class LocalStorageThread { > > Should derive from Noncopyable for multiple reasons. One is that it will make > this derive from FastAllocBase and save some work on the FastAllocBase. > > By the way, you could still have made this have a create function and a private > constructor, just using PassOwnPtr instead of PassRefPtr. The benefit would be > that we'd avoid having a "bare new" outside a create function. Might be good > long term to have that style. > > Otherwise, looks good. I'm going to say review- because I think the Noncopyable > is fairly important. All done. Also, I noticed how silly it was to have methods for each task you might want to schedule. So I combined them into one generic function.
Ping.
Committed r50519: <http://trac.webkit.org/changeset/50519>
Created attachment 42517 [details] Patch
Had to roll out the previous change. New patch makes LocalStorageTask not ref-counted and only uses own ptrs.
Committed r50558: <http://trac.webkit.org/changeset/50558>