Bug 30935 – Simplify LocalStorageThread

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Orlow]

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.

[Jeremy Orlow]

Created attachment 42161 [details]
Patch v1

[Darin Adler]

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.

[Jeremy Orlow]

Created attachment 42162 [details]
Patch v1

[Jeremy Orlow]

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

[Jeremy Orlow]

Ping.

[Jeremy Orlow]

Committed r50519: <http://trac.webkit.org/changeset/50519>

[Jeremy Orlow]

Created attachment 42517 [details]
Patch

[Jeremy Orlow]

Had to roll out the previous change.  New patch makes LocalStorageTask not ref-counted and only uses own ptrs.

[Jeremy Orlow]

Committed r50558: <http://trac.webkit.org/changeset/50558>