Bug 153612

Summary: Cleanup: Make DedicatedWorkerThread::create() an inline template method
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, andersca, darin
Priority: P2    
Version: WebKit Local Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=153617
Bug Depends on: 153157    
Bug Blocks:    
Attachments:
Description Flags
Patch aestes: review+

Description Daniel Bates 2016-01-28 13:07:56 PST
We should take advantage of C++11 variable template parameters and std::forward() to simplify DedicatedWorkerThread::create(), which turns around and passes it arguments to DedicatedWorkerThread::DedicatedWorkerThread().
Comment 1 Daniel Bates 2016-01-28 13:09:34 PST
Created attachment 270141 [details]
Patch
Comment 2 Daniel Bates 2016-01-28 13:14:11 PST
This patch does not apply because it depends on the patch for bug #153157, which changed the parameters taken by DedicatedWorkerThread::create(). I will rebase this patch before landing if this patch is reviewed before the patch for bug #153157.
Comment 3 Andy Estes 2016-01-28 13:18:02 PST
Comment on attachment 270141 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        to DedicatedWorkerThread::create() to DedicatedWorkerThread::DedicatedWorkerThread(). This

from ... to

> Source/WebCore/workers/DedicatedWorkerThread.h:42
> +        template<class... Args> static Ref<DedicatedWorkerThread> create(Args&&... args)

I'd use typename instead of class.
Comment 4 Andy Estes 2016-01-28 13:49:59 PST
It seems somewhat strange to have a one-off variadic template here, since the number of parameters will never actually vary in practice.

I wonder if we should consider creating something similar to std::make_unique for Ref and RefPtr? That would let us remove this type of duplication all over our codebase.
Comment 5 Anders Carlsson 2016-01-28 14:08:22 PST
(In reply to comment #4)
> It seems somewhat strange to have a one-off variadic template here, since
> the number of parameters will never actually vary in practice.
> 
> I wonder if we should consider creating something similar to
> std::make_unique for Ref and RefPtr? That would let us remove this type of
> duplication all over our codebase.

I've thought about that. We could even call it static Ref<T> create(...) and put it on RefCountedBase!
Comment 6 Daniel Bates 2016-01-28 14:28:08 PST
(In reply to comment #4)
> It seems somewhat strange to have a one-off variadic template here, since
> the number of parameters will never actually vary in practice.
> 

I know this is a one-off. I proposed this change because I've modified DedicatedWorkerThread::create() once in the patch for bug #153157 and plan to modify it again in a subsequent patch. Maybe we can come up with a better design for passing state to Web Worker threads. Currently, whenever we want to pass state to a Web Worker thread that cannot be derived from the existing state we passed then we need to modify DedicatedWorkerThread::create() among many other functions.

> I wonder if we should consider creating something similar to
> std::make_unique for Ref and RefPtr? That would let us remove this type of
> duplication all over our codebase.

Yes, we should do this.
Comment 7 Daniel Bates 2016-01-28 14:30:02 PST
(In reply to comment #5)
> (In reply to comment #4)
> > It seems somewhat strange to have a one-off variadic template here, since
> > the number of parameters will never actually vary in practice.
> > 
> > I wonder if we should consider creating something similar to
> > std::make_unique for Ref and RefPtr? That would let us remove this type of
> > duplication all over our codebase.
> 
> I've thought about that. We could even call it static Ref<T> create(...) and
> put it on RefCountedBase!

We should try this!
Comment 8 Andy Estes 2016-01-28 14:43:42 PST
(In reply to comment #5)
> (In reply to comment #4)
> > It seems somewhat strange to have a one-off variadic template here, since
> > the number of parameters will never actually vary in practice.
> > 
> > I wonder if we should consider creating something similar to
> > std::make_unique for Ref and RefPtr? That would let us remove this type of
> > duplication all over our codebase.
> 
> I've thought about that. We could even call it static Ref<T> create(...) and
> put it on RefCountedBase!

Yeah, that'd be nice, since it'd let us keep the constructors private.
Comment 9 Andy Estes 2016-01-28 14:45:53 PST
I filed https://bugs.webkit.org/show_bug.cgi?id=153617
Comment 10 Daniel Bates 2016-01-28 15:04:12 PST
Committed r195785: <http://trac.webkit.org/changeset/195785>
Comment 11 Andy Estes 2016-01-28 15:05:35 PST
(In reply to comment #10)
> Committed r195785: <http://trac.webkit.org/changeset/195785>

Looks like you didn't address my review feedback. Did you choose not to?
Comment 12 Daniel Bates 2016-01-28 15:17:06 PST
(In reply to comment #3)
> Comment on attachment 270141 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270141&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        to DedicatedWorkerThread::create() to DedicatedWorkerThread::DedicatedWorkerThread(). This
> 
> from ... to
> 
> > Source/WebCore/workers/DedicatedWorkerThread.h:42
> > +        template<class... Args> static Ref<DedicatedWorkerThread> create(Args&&... args)
> 
> I'd use typename instead of class.

Fixed in <http://trac.webkit.org/changeset/195786>.
Comment 13 Daniel Bates 2016-01-28 15:18:34 PST
(In reply to comment #11)
> (In reply to comment #10)
> > Committed r195785: <http://trac.webkit.org/changeset/195785>
> 
> Looks like you didn't address my review feedback. Did you choose not to?

No, I agree and appreciate your feedback. I inadvertently didn't commit it. See comment #12.