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
Daniel Bates
2016-01-28 13:07:56 PST
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. 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. |