Bug 153612

Created attachment 270141 [details] Patch

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

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.

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

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

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

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

I filed https://bugs.webkit.org/show_bug.cgi?id=153617

Committed r195785: <http://trac.webkit.org/changeset/195785>

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

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

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