153612 2016-01-28 13:07:56 -0800 Cleanup: Make DedicatedWorkerThread::create() an inline template method 2016-01-28 15:18:34 -0800 1 1 1 Unclassified WebKit WebCore Misc. WebKit Local Build All All RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=153617 P2 Normal --- 153157 1 dbates webkit-unassigned aestes andersca darin oldest_to_newest 1159849 0 dbates 2016-01-28 13:07:56 -0800 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(). 1159850 1 270141 dbates 2016-01-28 13:09:34 -0800 Created attachment 270141 Patch 1159858 2 dbates 2016-01-28 13:14:11 -0800 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. 1159860 3 270141 aestes 2016-01-28 13:18:02 -0800 Comment on attachment 270141 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. 1159865 4 aestes 2016-01-28 13:49:59 -0800 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. 1159871 5 andersca 2016-01-28 14:08:22 -0800 (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! 1159878 6 dbates 2016-01-28 14:28:08 -0800 (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. 1159880 7 dbates 2016-01-28 14:30:02 -0800 (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! 1159885 8 aestes 2016-01-28 14:43:42 -0800 (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. 1159889 9 aestes 2016-01-28 14:45:53 -0800 I filed https://bugs.webkit.org/show_bug.cgi?id=153617 1159902 10 dbates 2016-01-28 15:04:12 -0800 Committed r195785: <http://trac.webkit.org/changeset/195785> 1159904 11 aestes 2016-01-28 15:05:35 -0800 (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? 1159909 12 dbates 2016-01-28 15:17:06 -0800 (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>. 1159910 13 dbates 2016-01-28 15:18:34 -0800 (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. 270141 2016-01-28 13:09:34 -0800 2016-01-28 13:18:02 -0800 Patch Bug153612.patch text/plain 5477 dbates RnJvbSA5NDBiNGFlYWI4ZmExNTFjOTgyOWE5YzM0MjY4NjE3ZGFlYzc1NWM5IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBEYW5pZWwgQmF0ZXMgPGRhYmF0ZXNAYXBwbGUuY29tPgpEYXRl OiBUaHUsIDI4IEphbiAyMDE2IDEzOjA4OjA5IC0wODAwClN1YmplY3Q6IFtQQVRDSF0gQ2xlYW51 cDogTWFrZSBEZWRpY2F0ZWRXb3JrZXJUaHJlYWQ6OmNyZWF0ZSgpIGFuIGlubGluZQogdGVtcGxh dGUgbWV0aG9kIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNTM2MTIK ClJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgoKTWFrZSB1c2Ugb2YgdmFyaWFkaWMgdGVtcGxh dGUgYXJndW1lbnRzIGFuZCBzdGQ6OmZvcndhcmQoKSB0byBmb3J3YXJkIHRoZSBhcmd1bWVudHMg cGFzc2VkCnRvIERlZGljYXRlZFdvcmtlclRocmVhZDo6Y3JlYXRlKCkgdG8gRGVkaWNhdGVkV29y a2VyVGhyZWFkOjpEZWRpY2F0ZWRXb3JrZXJUaHJlYWQoKS4gVGhpcwpyZW1vdmVzIHRoZSBuZWVk IHRvIGR1cGxpY2F0ZSBjb2RlIHdoZW5ldmVyIHdlIG1vZGlmeSB0aGUgcGFyYW1ldGVyIHR5cGVz IG9yIG51bWJlciBvZgpwYXJhbWV0ZXJzIHRha2VuIGJ5IERlZGljYXRlZFdvcmtlclRocmVhZDo6 RGVkaWNhdGVkV29ya2VyVGhyZWFkKCkuCgoqIHdvcmtlcnMvRGVkaWNhdGVkV29ya2VyVGhyZWFk LmNwcDoKKFdlYkNvcmU6OkRlZGljYXRlZFdvcmtlclRocmVhZDo6Y3JlYXRlKTogRGVsZXRlZC4K KiB3b3JrZXJzL0RlZGljYXRlZFdvcmtlclRocmVhZC5oOiBSZW9yZ2FuaXplZCBsaXN0aW5nIG9m IG1lbWJlciBmdW5jdGlvbnMgc3VjaCB0aGF0IHdlCmdyb3VwIHRoZSBjcmVhdGlvbi9jb25zdHJ1 Y3RvciBhbmQgZGVzdHJ1Y3RvciBmdW5jdGlvbnMuCihXZWJDb3JlOjpEZWRpY2F0ZWRXb3JrZXJU aHJlYWQ6OmNyZWF0ZSk6IE1vZGlmaWVkIHRvIGJlIGFuIGlubGluZSB0ZW1wbGF0ZSB3aXRoIHZh cmlhZGljCnBhcmFtZXRlcnMgdGhhdCBzdGQ6OmZvcndhcmQoKXMgaXRzIGFyZ3VtZW50cyB0byBE ZWRpY2F0ZWRXb3JrZXJUaHJlYWQ6OkRlZGljYXRlZFdvcmtlclRocmVhZCgpLgotLS0KIFNvdXJj ZS9XZWJDb3JlL0NoYW5nZUxvZyAgICAgICAgICAgICAgICAgICAgICAgICB8IDE5ICsrKysrKysr KysrKysrKysrKysKIFNvdXJjZS9XZWJDb3JlL3dvcmtlcnMvRGVkaWNhdGVkV29ya2VyVGhyZWFk LmNwcCB8ICA1IC0tLS0tCiBTb3VyY2UvV2ViQ29yZS93b3JrZXJzL0RlZGljYXRlZFdvcmtlclRo cmVhZC5oICAgfCAgOCArKysrKystLQogMyBmaWxlcyBjaGFuZ2VkLCAyNSBpbnNlcnRpb25zKCsp LCA3IGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBi L1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCA2YzRkY2M5Li5kYWM4MTA5IDEwMDY0NAot LS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCkBAIC0xLDUgKzEsMjQgQEAKIDIwMTYtMDEtMjggIERhbmllbCBCYXRlcyAgPGRhYmF0ZXNA YXBwbGUuY29tPgogCisgICAgICAgIENsZWFudXA6IE1ha2UgRGVkaWNhdGVkV29ya2VyVGhyZWFk OjpjcmVhdGUoKSBhbiBpbmxpbmUgdGVtcGxhdGUgbWV0aG9kCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNTM2MTIKKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBNYWtlIHVzZSBvZiB2YXJpYWRpYyB0ZW1wbGF0 ZSBhcmd1bWVudHMgYW5kIHN0ZDo6Zm9yd2FyZCgpIHRvIGZvcndhcmQgdGhlIGFyZ3VtZW50cyBw YXNzZWQKKyAgICAgICAgdG8gRGVkaWNhdGVkV29ya2VyVGhyZWFkOjpjcmVhdGUoKSB0byBEZWRp Y2F0ZWRXb3JrZXJUaHJlYWQ6OkRlZGljYXRlZFdvcmtlclRocmVhZCgpLiBUaGlzCisgICAgICAg IHJlbW92ZXMgdGhlIG5lZWQgdG8gZHVwbGljYXRlIGNvZGUgd2hlbmV2ZXIgd2UgbW9kaWZ5IHRo ZSBwYXJhbWV0ZXIgdHlwZXMgb3IgbnVtYmVyIG9mCisgICAgICAgIHBhcmFtZXRlcnMgdGFrZW4g YnkgRGVkaWNhdGVkV29ya2VyVGhyZWFkOjpEZWRpY2F0ZWRXb3JrZXJUaHJlYWQoKS4KKworICAg ICAgICAqIHdvcmtlcnMvRGVkaWNhdGVkV29ya2VyVGhyZWFkLmNwcDoKKyAgICAgICAgKFdlYkNv cmU6OkRlZGljYXRlZFdvcmtlclRocmVhZDo6Y3JlYXRlKTogRGVsZXRlZC4KKyAgICAgICAgKiB3 b3JrZXJzL0RlZGljYXRlZFdvcmtlclRocmVhZC5oOiBSZW9yZ2FuaXplZCBsaXN0aW5nIG9mIG1l bWJlciBmdW5jdGlvbnMgc3VjaCB0aGF0IHdlCisgICAgICAgIGdyb3VwIHRoZSBjcmVhdGlvbi9j b25zdHJ1Y3RvciBhbmQgZGVzdHJ1Y3RvciBmdW5jdGlvbnMuCisgICAgICAgIChXZWJDb3JlOjpE ZWRpY2F0ZWRXb3JrZXJUaHJlYWQ6OmNyZWF0ZSk6IE1vZGlmaWVkIHRvIGJlIGFuIGlubGluZSB0 ZW1wbGF0ZSB3aXRoIHZhcmlhZGljCisgICAgICAgIHBhcmFtZXRlcnMgdGhhdCBzdGQ6OmZvcndh cmQoKXMgaXRzIGFyZ3VtZW50cyB0byBEZWRpY2F0ZWRXb3JrZXJUaHJlYWQ6OkRlZGljYXRlZFdv cmtlclRocmVhZCgpLgorCisyMDE2LTAxLTI4ICBEYW5pZWwgQmF0ZXMgIDxkYWJhdGVzQGFwcGxl LmNvbT4KKwogICAgICAgICBDU1A6IFVzZSB0aGUgc2VydmVkIENTUCBoZWFkZXIgZm9yIGRlZGlj YXRlZCB3b3JrZXJzCiAgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0xNTMxNTcKICAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzI0MzgzMjU0PgpkaWZmIC0tZ2l0 IGEvU291cmNlL1dlYkNvcmUvd29ya2Vycy9EZWRpY2F0ZWRXb3JrZXJUaHJlYWQuY3BwIGIvU291 cmNlL1dlYkNvcmUvd29ya2Vycy9EZWRpY2F0ZWRXb3JrZXJUaHJlYWQuY3BwCmluZGV4IDNiZDFi YTcuLjc4NjJjZDUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3dvcmtlcnMvRGVkaWNhdGVk V29ya2VyVGhyZWFkLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS93b3JrZXJzL0RlZGljYXRlZFdv cmtlclRocmVhZC5jcHAKQEAgLTM5LDExICszOSw2IEBACiAKIG5hbWVzcGFjZSBXZWJDb3JlIHsK IAotUmVmPERlZGljYXRlZFdvcmtlclRocmVhZD4gRGVkaWNhdGVkV29ya2VyVGhyZWFkOjpjcmVh dGUoY29uc3QgVVJMJiBzY3JpcHRVUkwsIGNvbnN0IFN0cmluZyYgdXNlckFnZW50LCBjb25zdCBT dHJpbmcmIHNvdXJjZUNvZGUsIFdvcmtlckxvYWRlclByb3h5JiB3b3JrZXJMb2FkZXJQcm94eSwg V29ya2VyT2JqZWN0UHJveHkmIHdvcmtlck9iamVjdFByb3h5LCBXb3JrZXJUaHJlYWRTdGFydE1v ZGUgc3RhcnRNb2RlLCBjb25zdCBDb250ZW50U2VjdXJpdHlQb2xpY3lSZXNwb25zZUhlYWRlcnMm IGNvbnRlbnRTZWN1cml0eVBvbGljeVJlc3BvbnNlSGVhZGVycywgY29uc3QgU2VjdXJpdHlPcmln aW4qIHRvcE9yaWdpbikKLXsKLSAgICByZXR1cm4gYWRvcHRSZWYoKm5ldyBEZWRpY2F0ZWRXb3Jr ZXJUaHJlYWQoc2NyaXB0VVJMLCB1c2VyQWdlbnQsIHNvdXJjZUNvZGUsIHdvcmtlckxvYWRlclBy b3h5LCB3b3JrZXJPYmplY3RQcm94eSwgc3RhcnRNb2RlLCBjb250ZW50U2VjdXJpdHlQb2xpY3lS ZXNwb25zZUhlYWRlcnMsIHRvcE9yaWdpbikpOwotfQotCiBEZWRpY2F0ZWRXb3JrZXJUaHJlYWQ6 OkRlZGljYXRlZFdvcmtlclRocmVhZChjb25zdCBVUkwmIHVybCwgY29uc3QgU3RyaW5nJiB1c2Vy QWdlbnQsIGNvbnN0IFN0cmluZyYgc291cmNlQ29kZSwgV29ya2VyTG9hZGVyUHJveHkmIHdvcmtl ckxvYWRlclByb3h5LCBXb3JrZXJPYmplY3RQcm94eSYgd29ya2VyT2JqZWN0UHJveHksIFdvcmtl clRocmVhZFN0YXJ0TW9kZSBzdGFydE1vZGUsIGNvbnN0IENvbnRlbnRTZWN1cml0eVBvbGljeVJl c3BvbnNlSGVhZGVycyYgY29udGVudFNlY3VyaXR5UG9saWN5UmVzcG9uc2VIZWFkZXJzLCBjb25z dCBTZWN1cml0eU9yaWdpbiogdG9wT3JpZ2luKQogICAgIDogV29ya2VyVGhyZWFkKHVybCwgdXNl ckFnZW50LCBzb3VyY2VDb2RlLCB3b3JrZXJMb2FkZXJQcm94eSwgd29ya2VyT2JqZWN0UHJveHks IHN0YXJ0TW9kZSwgY29udGVudFNlY3VyaXR5UG9saWN5UmVzcG9uc2VIZWFkZXJzLCB0b3BPcmln aW4pCiAgICAgLCBtX3dvcmtlck9iamVjdFByb3h5KHdvcmtlck9iamVjdFByb3h5KQpkaWZmIC0t Z2l0IGEvU291cmNlL1dlYkNvcmUvd29ya2Vycy9EZWRpY2F0ZWRXb3JrZXJUaHJlYWQuaCBiL1Nv dXJjZS9XZWJDb3JlL3dvcmtlcnMvRGVkaWNhdGVkV29ya2VyVGhyZWFkLmgKaW5kZXggODU5NGVm Ny4uY2IzYTM0NiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvd29ya2Vycy9EZWRpY2F0ZWRX b3JrZXJUaHJlYWQuaAorKysgYi9Tb3VyY2UvV2ViQ29yZS93b3JrZXJzL0RlZGljYXRlZFdvcmtl clRocmVhZC5oCkBAIC0zOSwxMCArMzksMTQgQEAgbmFtZXNwYWNlIFdlYkNvcmUgewogCiAgICAg Y2xhc3MgRGVkaWNhdGVkV29ya2VyVGhyZWFkIDogcHVibGljIFdvcmtlclRocmVhZCB7CiAgICAg cHVibGljOgotICAgICAgICBzdGF0aWMgUmVmPERlZGljYXRlZFdvcmtlclRocmVhZD4gY3JlYXRl KGNvbnN0IFVSTCYgc2NyaXB0VVJMLCBjb25zdCBTdHJpbmcmIHVzZXJBZ2VudCwgY29uc3QgU3Ry aW5nJiBzb3VyY2VDb2RlLCBXb3JrZXJMb2FkZXJQcm94eSYsIFdvcmtlck9iamVjdFByb3h5Jiwg V29ya2VyVGhyZWFkU3RhcnRNb2RlLCBjb25zdCBDb250ZW50U2VjdXJpdHlQb2xpY3lSZXNwb25z ZUhlYWRlcnMmLCBjb25zdCBTZWN1cml0eU9yaWdpbiogdG9wT3JpZ2luKTsKLSAgICAgICAgV29y a2VyT2JqZWN0UHJveHkmIHdvcmtlck9iamVjdFByb3h5KCkgY29uc3QgeyByZXR1cm4gbV93b3Jr ZXJPYmplY3RQcm94eTsgfQorICAgICAgICB0ZW1wbGF0ZTxjbGFzcy4uLiBBcmdzPiBzdGF0aWMg UmVmPERlZGljYXRlZFdvcmtlclRocmVhZD4gY3JlYXRlKEFyZ3MmJi4uLiBhcmdzKQorICAgICAg ICB7CisgICAgICAgICAgICByZXR1cm4gYWRvcHRSZWYoKm5ldyBEZWRpY2F0ZWRXb3JrZXJUaHJl YWQoc3RkOjpmb3J3YXJkPEFyZ3M+KGFyZ3MpLi4uKSk7CisgICAgICAgIH0KICAgICAgICAgdmly dHVhbCB+RGVkaWNhdGVkV29ya2VyVGhyZWFkKCk7CiAKKyAgICAgICAgV29ya2VyT2JqZWN0UHJv eHkmIHdvcmtlck9iamVjdFByb3h5KCkgY29uc3QgeyByZXR1cm4gbV93b3JrZXJPYmplY3RQcm94 eTsgfQorCiAgICAgcHJvdGVjdGVkOgogICAgICAgICB2aXJ0dWFsIFJlZjxXb3JrZXJHbG9iYWxT Y29wZT4gY3JlYXRlV29ya2VyR2xvYmFsU2NvcGUoY29uc3QgVVJMJiwgY29uc3QgU3RyaW5nJiB1 c2VyQWdlbnQsIGNvbnN0IENvbnRlbnRTZWN1cml0eVBvbGljeVJlc3BvbnNlSGVhZGVycyYsIFBh c3NSZWZQdHI8U2VjdXJpdHlPcmlnaW4+IHRvcE9yaWdpbikgb3ZlcnJpZGU7CiAgICAgICAgIHZp cnR1YWwgdm9pZCBydW5FdmVudExvb3AoKSBvdmVycmlkZTsKLS0gCjIuMy44IChBcHBsZSBHaXQt NTgpCgo=