Bug 158542

Summary: Greatly simplify CrossThreadTask.h
Product: WebKit Reporter: Brady Eidson <beidson>
Component: Web Template FrameworkAssignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, andersca, benjamin, cdumez, cmarcelo, commit-queue, sam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Brady Eidson 2016-06-08 14:20:23 PDT 
Greatly simplify CrossThreadTask.h

The current implementation predates variadic templates in C++11.

Now with C++14 we can get rid of most of this nonsense.

There is an additional move constructor call per argument object, which is disappointing.

Maybe there's a way to get rid of that and I can't see that way.

But compared to two extra copies (which is what we needed up through this morning before I landed http://trac.webkit.org/changeset/201809) we're still way better off.
Comment 1 Brady Eidson 2016-06-08 14:22:35 PDT 
Created attachment 280837 [details]
Patch
Comment 2 Sam Weinig 2016-06-08 14:31:49 PDT 
Can this use std::index_sequence rather than the homespun GeneratedIndices? For an example usage of it, see WebKit2’s HandleMessage.h.
Comment 3 Brady Eidson 2016-06-08 14:32:43 PDT 
(In reply to comment #2)
> Can this use std::index_sequence rather than the homespun GeneratedIndices?
> For an example usage of it, see WebKit2’s HandleMessage.h.

Holy crap, yes.
Comment 4 Brady Eidson 2016-06-08 14:51:13 PDT 
Created attachment 280843 [details]
Patch
Comment 5 Brady Eidson 2016-06-08 15:19:59 PDT 
Yikes on the build failures.

Currently exploring why I'm seemingly good locally...
Comment 6 Brady Eidson 2016-06-08 15:40:04 PDT 
Created attachment 280849 [details]
Patch
Comment 7 Brady Eidson 2016-06-08 15:57:17 PDT 
Created attachment 280852 [details]
Patch
Comment 8 Saam Barati 2016-06-09 00:50:58 PDT 
Comment on attachment 280852 [details]
Patch

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

> Source/WTF/wtf/CrossThreadTask.h:80
> +void callMemberFunctionForCrossThreadTaskImpl(C* object, MF function, ArgsTuple&& args, std::index_sequence<ArgsIndex...>)

std::index_sequence is quite clever.
After enough reading on the web, I actually understand what's happening here.
Comment 9 Darin Adler 2016-06-09 10:10:31 PDT 
Comment on attachment 280852 [details]
Patch

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

> Source/WTF/wtf/CrossThreadTask.h:65
> +template <typename F, typename ArgsTuple, typename ArgsIndicies = std::make_index_sequence<std::tuple_size<ArgsTuple>::value>>

The word indices has only two "i"s in it.
Comment 10 WebKit Commit Bot 2016-06-09 10:33:25 PDT 
Comment on attachment 280852 [details]
Patch

Clearing flags on attachment: 280852

Committed r201872: <http://trac.webkit.org/changeset/201872>
Comment 11 WebKit Commit Bot 2016-06-09 10:33:30 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 12 Brady Eidson 2016-06-09 10:41:28 PDT 
(In reply to comment #10)
> Comment on attachment 280852 [details]
> Patch
> 
> Clearing flags on attachment: 280852
> 
> Committed r201872: <http://trac.webkit.org/changeset/201872>

Fixed the typo in http://trac.webkit.org/changeset/201873