Bug 30935 - Simplify LocalStorageThread
Summary: Simplify LocalStorageThread
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Jeremy Orlow
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-29 17:04 PDT by Jeremy Orlow
Modified: 2009-11-05 01:33 PST (History)
3 users (show)

See Also:


Attachments
Patch v1 (7.81 KB, patch)
2009-10-29 17:16 PDT, Jeremy Orlow
no flags Details | Formatted Diff | Diff
Patch v1 (9.25 KB, patch)
2009-10-29 17:41 PDT, Jeremy Orlow
no flags Details | Formatted Diff | Diff
Patch (9.89 KB, patch)
2009-11-04 13:54 PST, Jeremy Orlow
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Orlow 2009-10-29 17:04:03 PDT
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.
Comment 1 Jeremy Orlow 2009-10-29 17:16:51 PDT
Created attachment 42161 [details]
Patch v1
Comment 2 Darin Adler 2009-10-29 17:27:52 PDT
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.
Comment 3 Jeremy Orlow 2009-10-29 17:41:32 PDT
Created attachment 42162 [details]
Patch v1
Comment 4 Jeremy Orlow 2009-10-29 17:42:56 PDT
(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.
Comment 5 Jeremy Orlow 2009-11-02 16:39:01 PST
Ping.
Comment 6 Jeremy Orlow 2009-11-04 11:11:59 PST
Committed r50519: <http://trac.webkit.org/changeset/50519>
Comment 7 Jeremy Orlow 2009-11-04 13:54:52 PST
Created attachment 42517 [details]
Patch
Comment 8 Jeremy Orlow 2009-11-04 13:55:29 PST
Had to roll out the previous change.  New patch makes LocalStorageTask not ref-counted and only uses own ptrs.
Comment 9 Jeremy Orlow 2009-11-05 01:33:00 PST
Committed r50558: <http://trac.webkit.org/changeset/50558>