Bug 158111

Summary: WorkQueue::dispatch() / RunLoop::dispatch() should not copy captured lambda variables
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Web Template FrameworkAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, beidson, benjamin, buildbot, cgarcia, cmarcelo, commit-queue, darin, rniwa, sam, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 158010, 158124, 158166, 158176    
Attachments:
Description Flags
WIP patch
none
WIP patch
none
WIP patch
none
WIP patch
none
WIP patch
none
WIP patch
none
WIP patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews100 for mac-yosemite
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2016-05-25 22:15:20 PDT 
WorkQueue::dispatch() / RunLoop::dispatch() should not copy captured lambda variables. These are often used cross-thread and copying the captured lambda variables in dangerous.
Comment 1 Brady Eidson 2016-05-25 22:20:45 PDT 
👍👍👍
Comment 2 Chris Dumez 2016-05-25 22:38:28 PDT 
Created attachment 279867 [details]
WIP patch

WIP patch based on very initial brainstorming with Brady and Anders. Let me know what you think.
Comment 3 Chris Dumez 2016-05-25 22:43:51 PDT 
Created attachment 279868 [details]
WIP patch
Comment 4 WebKit Commit Bot 2016-05-25 22:45:59 PDT 
Attachment 279868 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:66:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:72:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/WorkQueue.h:77:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:556:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:574:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:586:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:605:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:626:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:655:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:674:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:424:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 16 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Chris Dumez 2016-05-26 10:38:00 PDT 
Created attachment 279895 [details]
WIP patch

Version without a virtual base. We could easily extend this to support other lambdas than void() by templatizing the class.
Right now it is non-copyable, but we could imagine a version that is copyable using ThreadSafeRefCounted internally.
Comment 6 WebKit Commit Bot 2016-05-26 10:39:24 PDT 
Attachment 279895 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:66:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:72:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/WorkQueue.h:77:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:556:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:574:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:586:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:605:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:626:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:655:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:674:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:424:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 16 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Chris Dumez 2016-05-26 11:02:47 PDT 
Created attachment 279897 [details]
WIP patch
Comment 8 WebKit Commit Bot 2016-05-26 11:04:27 PDT 
Attachment 279897 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:66:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:72:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/WorkQueue.h:77:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:556:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:574:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:586:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:605:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:626:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:655:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:674:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:424:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 16 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Chris Dumez 2016-05-26 11:56:01 PDT 
Created attachment 279899 [details]
WIP patch

Will play with EWS to get it building on all platforms.
Comment 10 WebKit Commit Bot 2016-05-26 11:57:07 PDT 
Attachment 279899 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:556:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:574:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:586:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:605:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:626:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:655:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:674:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:66:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:72:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:424:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 15 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Chris Dumez 2016-05-26 12:04:38 PDT 
Created attachment 279902 [details]
WIP patch
Comment 12 WebKit Commit Bot 2016-05-26 12:06:28 PDT 
Attachment 279902 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:556:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:574:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:586:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:605:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:626:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:655:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:674:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:66:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:72:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:424:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 15 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Chris Dumez 2016-05-26 12:27:03 PDT 
Created attachment 279904 [details]
WIP patch
Comment 14 WebKit Commit Bot 2016-05-26 12:27:55 PDT 
Attachment 279904 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:556:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:574:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:586:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:605:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:626:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:655:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:674:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/win/WorkItemWin.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:66:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:72:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:424:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 16 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Chris Dumez 2016-05-26 12:43:48 PDT 
Created attachment 279905 [details]
Patch
Comment 16 WebKit Commit Bot 2016-05-26 12:45:02 PDT 
Attachment 279905 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:66:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:72:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:424:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:556:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:574:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:586:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:605:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:626:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:655:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:674:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 15 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Chris Dumez 2016-05-26 12:52:47 PDT 
Created attachment 279906 [details]
Patch
Comment 18 WebKit Commit Bot 2016-05-26 12:54:05 PDT 
Attachment 279906 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:66:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:72:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:424:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:556:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:574:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:586:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:605:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:626:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:655:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:674:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 15 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Chris Dumez 2016-05-26 13:24:47 PDT 
Oh MSVC does not like this :/

C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/FunctionDispatcher.h(71): error C2065: 'F': undeclared identifier (compiling source file C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\inspector\agents\InspectorHeapAgent.cpp) [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
  C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\memory(1630): note: see reference to function template instantiation 'WTF::NonCopyableFunction::LambdaWrapper::LambdaWrapper<_Ty,void>(F &&)' being compiled
          with
          [
              _Ty=Inspector::InspectorHeapAgent::didGarbageCollect::<lambda_8879b6e4ab22bfba2f51f56750bddbe6>,
              F=Inspector::InspectorHeapAgent::didGarbageCollect::<lambda_8879b6e4ab22bfba2f51f56750bddbe6>
          ] (compiling source file C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\inspector\agents\InspectorHeapAgent.cpp)
  C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/FunctionDispatcher.h(44): note: see reference to function template instantiation 'std::unique_ptr<WTF::NonCopyableFunction::LambdaWrapper,std::default_delete<_Ty>> std::make_unique<WTF::NonCopyableFunction::LambdaWrapper,Inspector::InspectorHeapAgent::didGarbageCollect::<lambda_8879b6e4ab22bfba2f51f56750bddbe6>>(Inspector::InspectorHeapAgent::didGarbageCollect::<lambda_8879b6e4ab22bfba2f51f56750bddbe6> &&)' being compiled
          with
          [
              _Ty=WTF::NonCopyableFunction::LambdaWrapper
          ] (compiling source file C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\inspector\agents\InspectorHeapAgent.cpp)
  C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\inspector\agents\InspectorHeapAgent.cpp(297): note: see reference to function template instantiation 'WTF::NonCopyableFunction::NonCopyableFunction<Inspector::InspectorHeapAgent::didGarbageCollect::<lambda_8879b6e4ab22bfba2f51f56750bddbe6>,void>(F &&)' being compiled
          with
          [
              F=Inspector::InspectorHeapAgent::didGarbageCollect::<lambda_8879b6e4ab22bfba2f51f56750bddbe6>
          ]

I don't get it.
Comment 20 Chris Dumez 2016-05-26 13:37:54 PDT 
Created attachment 279907 [details]
Patch
Comment 21 Chris Dumez 2016-05-26 13:38:36 PDT 
Trying to make MSVC happy as it seems confused by the template parameter used inside the lambda function.
Comment 22 WebKit Commit Bot 2016-05-26 13:39:32 PDT 
Attachment 279907 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:66:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:72:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:424:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:556:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:574:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:586:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:605:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:626:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:655:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:674:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 15 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Chris Dumez 2016-05-26 14:38:04 PDT 
Comment on attachment 279907 [details]
Patch

MSVC is finally happy.
Comment 24 Alexey Proskuryakov 2016-05-26 14:39:03 PDT 
Comment on attachment 279907 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        WorkQueue::dispatch() / RunLoop::dispatch() should not copy captured lambda variables.
> +        These are often used cross-thread and copying the captured lambda variables can be
> +        dangerous (e.g. we do not want to copy a String after calling isolatedCopy() upon
> +        capture).

I think that the ChangeLog would benefit from a description of what the correct approach is, in addition to this.
Comment 25 Chris Dumez 2016-05-26 14:49:27 PDT 
(In reply to comment #24)
> Comment on attachment 279907 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279907&action=review
> 
> > Source/JavaScriptCore/ChangeLog:11
> > +        WorkQueue::dispatch() / RunLoop::dispatch() should not copy captured lambda variables.
> > +        These are often used cross-thread and copying the captured lambda variables can be
> > +        dangerous (e.g. we do not want to copy a String after calling isolatedCopy() upon
> > +        capture).
> 
> I think that the ChangeLog would benefit from a description of what the
> correct approach is, in addition to this.

Sure, I'll improve the ChangeLog. Basically, the proposal is to use a new WTF::NonCopyableFunction type instead of an std::function for this case to guarantee that the passed-in lambda (and its captured variables) does not get copied.

NonCopyableFunction would also allow us to capture move-only types (such as std::unique_ptr) since NonCopyableFunction does not require the lambda to be copyable (unlike std::function).
Comment 26 Chris Dumez 2016-05-26 14:58:37 PDT 
Created attachment 279913 [details]
Patch
Comment 27 Chris Dumez 2016-05-26 14:59:06 PDT 
The new version improves the WTF ChangeLog.
Comment 28 WebKit Commit Bot 2016-05-26 14:59:46 PDT 
Attachment 279913 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:66:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:72:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:424:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:556:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:574:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:586:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:605:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:626:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:655:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:674:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 15 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 29 Build Bot 2016-05-26 15:43:02 PDT 
Comment on attachment 279913 [details]
Patch

Attachment 279913 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1387868

Number of test failures exceeded the failure limit.
Comment 30 Build Bot 2016-05-26 15:43:07 PDT 
Created attachment 279916 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 31 Chris Dumez 2016-05-26 15:49:38 PDT 
The EWS failure is unrelated I believe. It has been happening for the last week on several bugs. Failures look like:
Errors look like:
--- /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/imported/w3c/web-platform-tests/IndexedDB/idbobjectstore_delete-expected.txt
+++ /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/imported/w3c/web-platform-tests/IndexedDB/idbobjectstore_delete-actual.txt
@@ -1,3 +1,5 @@
-
-PASS IDBObjectStore.delete() - delete removes record (inline keys) 
-
+layer at (0,0) size 800x600
+  RenderView at (0,0) size 800x600
+layer at (0,0) size 800x600
+  RenderBlock {HTML} at (0,0) size 800x600
+    RenderBody {BODY} at (8,8) size 784x584
Comment 32 Darin Adler 2016-05-26 16:51:05 PDT 
Comment on attachment 279913 [details]
Patch

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

I’m sort of shocked that we have to make this NoncopyableFunction class. Nothing in standard C++ for this?

> Source/WTF/ChangeLog:13
> +        This patch introduces a new NonCopyableFunction type that behaves similarly to

The name should be NoncopyableFunction, since "noncopyable" is a single word, even though it’s not a real English word.

> Source/WTF/wtf/FunctionDispatcher.h:44
> +        : m_lambdaWrapper(std::make_unique<LambdaWrapper>(WTFMove(lambda)))

So much heap allocation. Sigh.

> Source/WTF/wtf/cocoa/WorkQueueCocoa.cpp:36
> +    auto* functionPtr = new NonCopyableFunction(WTFMove(function));

No way to do this without heap allocation? Isn’t there some technique that might work using a lambda instead of a block below?
Comment 33 Chris Dumez 2016-05-26 16:59:04 PDT 
(In reply to comment #32)
> Comment on attachment 279913 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279913&action=review
> 
> I’m sort of shocked that we have to make this NoncopyableFunction class.
> Nothing in standard C++ for this?

I couldn't find any. I only found this paper that talks about this issue:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4159.pdf

> 
> > Source/WTF/ChangeLog:13
> > +        This patch introduces a new NonCopyableFunction type that behaves similarly to
> 
> The name should be NoncopyableFunction, since "noncopyable" is a single
> word, even though it’s not a real English word.

Ok, will fix.

> 
> > Source/WTF/wtf/FunctionDispatcher.h:44
> > +        : m_lambdaWrapper(std::make_unique<LambdaWrapper>(WTFMove(lambda)))
> 
> So much heap allocation. Sigh.
> 
> > Source/WTF/wtf/cocoa/WorkQueueCocoa.cpp:36
> > +    auto* functionPtr = new NonCopyableFunction(WTFMove(function));
> 
> No way to do this without heap allocation? Isn’t there some technique that
> might work using a lambda instead of a block below?

I asked Anders if there was a better ways since I am not familiar with ObjC. He could not think of a better one but I'll investigate further.
Comment 34 Chris Dumez 2016-05-26 17:02:45 PDT 
(In reply to comment #32)
> Comment on attachment 279913 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279913&action=review
> 
> I’m sort of shocked that we have to make this NoncopyableFunction class.
> Nothing in standard C++ for this?
> 
> > Source/WTF/ChangeLog:13
> > +        This patch introduces a new NonCopyableFunction type that behaves similarly to
> 
> The name should be NoncopyableFunction, since "noncopyable" is a single
> word, even though it’s not a real English word.
> 
> > Source/WTF/wtf/FunctionDispatcher.h:44
> > +        : m_lambdaWrapper(std::make_unique<LambdaWrapper>(WTFMove(lambda)))
> 
> So much heap allocation. Sigh.

I used to have all the LambdaWrapper members directly on NoncopyableFunction in an earlier version of my patch (https://bugs.webkit.org/attachment.cgi?id=279895&action=review) but Anders suggested I used a std::unique_ptr internally to decrease the size of NoncopyableFunction (which had 3 pointer members).

> 
> > Source/WTF/wtf/cocoa/WorkQueueCocoa.cpp:36
> > +    auto* functionPtr = new NonCopyableFunction(WTFMove(function));
> 
> No way to do this without heap allocation? Isn’t there some technique that
> might work using a lambda instead of a block below?
Comment 35 Chris Dumez 2016-05-26 18:35:50 PDT 
(In reply to comment #32)
> Comment on attachment 279913 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279913&action=review
> 
> I’m sort of shocked that we have to make this NoncopyableFunction class.
> Nothing in standard C++ for this?
> 
> > Source/WTF/ChangeLog:13
> > +        This patch introduces a new NonCopyableFunction type that behaves similarly to
> 
> The name should be NoncopyableFunction, since "noncopyable" is a single
> word, even though it’s not a real English word.
> 
> > Source/WTF/wtf/FunctionDispatcher.h:44
> > +        : m_lambdaWrapper(std::make_unique<LambdaWrapper>(WTFMove(lambda)))
> 
> So much heap allocation. Sigh.
> 
> > Source/WTF/wtf/cocoa/WorkQueueCocoa.cpp:36
> > +    auto* functionPtr = new NonCopyableFunction(WTFMove(function));
> 
> No way to do this without heap allocation? Isn’t there some technique that
> might work using a lambda instead of a block below?

FYI, I have tried using a lambda instead of a block as so:
dispatch_async(m_dispatchQueue, [this, function = WTFMove(function)] {
        function();
        deref();
    });

However, this did not build:
/Volumes/Data/WebKit/OpenSource/Source/WTF/wtf/cocoa/WorkQueueCocoa.mm:36:37: error: call to implicitly-deleted copy constructor of 'const (lambda at /Volumes/Data/WebKit/OpenSource/Source/WTF/wtf/cocoa/WorkQueueCocoa.mm:36:37)'
/Volumes/Data/WebKit/OpenSource/Source/WTF/wtf/cocoa/WorkQueueCocoa.mm:36:44: note: copy constructor of '' is implicitly deleted because field '' has a deleted copy constructor
In file included from /Volumes/Data/WebKit/OpenSource/Source/WTF/wtf/cocoa/WorkQueueCocoa.mm:27:
In file included from /Volumes/Data/WebKit/OpenSource/Source/WTF/wtf/WorkQueue.h:33:
/Volumes/Data/WebKit/OpenSource/Source/WTF/wtf/FunctionDispatcher.h:36:26: note: 'NoncopyableFunction' has been explicitly marked deleted here
/Volumes/Data/WebKit/OpenSource/Source/WTF/wtf/cocoa/WorkQueueCocoa.mm:36:37: note: implicit capture of lambda object due to conversion to block pointer here
1 error generated.

It looks like it tries to copy my NoncopyableFunction object when converting the lambda into a block.
Comment 36 Chris Dumez 2016-05-26 19:15:46 PDT 
Created attachment 279935 [details]
Patch
Comment 37 WebKit Commit Bot 2016-05-26 19:19:31 PDT 
Attachment 279935 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:66:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:72:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:424:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:556:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:574:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:586:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:605:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:626:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:655:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:674:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 15 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 38 Chris Dumez 2016-05-26 19:50:19 PDT 
I renamed to NoncopyableFunction and stopped using a unique_ptr internally so we no longer need to heap allocate. Do you mind taking another look since the NoncopyableFunction class looks a bit different now?

Note that I could not find a way to avoid the heap allocations in WorkQueueCocoa.cpp.
Comment 39 Anders Carlsson 2016-05-27 10:17:39 PDT 
I don't think this is better than using a unique_ptr - we're still doing heap allocation of the lambda, so we might as well use a unique_ptr instead of untyped lambdas for deletion + invocation.
Comment 40 Chris Dumez 2016-05-27 10:23:45 PDT 
Comment on attachment 279935 [details]
Patch

Ok, I'll work on what Anders is suggesting.
Comment 41 Chris Dumez 2016-05-27 10:41:35 PDT 
Created attachment 279973 [details]
Patch
Comment 42 WebKit Commit Bot 2016-05-27 10:43:41 PDT 
Attachment 279973 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:66:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:72:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:424:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:556:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:574:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:586:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:605:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:626:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:655:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:674:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 15 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 43 Chris Dumez 2016-05-27 11:01:29 PDT 
Created attachment 279974 [details]
Patch
Comment 44 WebKit Commit Bot 2016-05-27 11:06:40 PDT 
Attachment 279974 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:66:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:72:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:424:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:556:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:574:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:586:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:605:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:626:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:655:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:674:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 15 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 45 Darin Adler 2016-05-27 11:38:16 PDT 
Comment on attachment 279973 [details]
Patch

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

> Source/JavaScriptCore/runtime/Watchdog.cpp:176
> +            this->m_timerDidFire = true;

I don’t think we need "this->" here.

> Source/JavaScriptCore/runtime/Watchdog.cpp:178
> +        this->deref();

I don’t think we need "this->" here.

> Source/WTF/wtf/FunctionDispatcher.h:34
> +class LambdaWrapperBase {

Should these be called LambdaWrapper or FunctionWrapper?

> Source/WTF/wtf/FunctionDispatcher.h:42
> +template<typename F, class = typename std::enable_if<std::is_rvalue_reference<F&&>::value>::type>

Is it better to use the "F" for this argument rather than a name like FunctionType?

> Source/WTF/wtf/FunctionDispatcher.h:44
> +    WTF_MAKE_NONCOPYABLE(LambdaWrapper);

I’m so sick of this macro. We should probably start writing out the "= delete" stuff instead in the future. We made this macro when it was a lot harder to delete the copy constructor and assignment operator.

> Source/WTF/wtf/FunctionDispatcher.h:59
> +    WTF_MAKE_NONCOPYABLE(NoncopyableFunction);

I don’t think this is needed. I think a class with a unique_ptr member ends up being non-copyable automatically.

> Source/WTF/wtf/FunctionDispatcher.h:63
> +    template<typename F, class = typename std::enable_if<std::is_rvalue_reference<F&&>::value>::type>

I think I would prefer FunctionType.

> Source/WTF/wtf/FunctionDispatcher.h:64
> +    NoncopyableFunction(F&& lambda)

Should this be marked explicit or not?

> Source/WTF/wtf/FunctionDispatcher.h:70
> +    NoncopyableFunction(NoncopyableFunction&&) = default;
> +    NoncopyableFunction& operator=(NoncopyableFunction&&) = default;

If we didn’t do the unnecessary WTF_MAKE_NONCOPYABLE, then I think this would just happen automatically without having to state it.
Comment 46 Chris Dumez 2016-05-27 11:44:12 PDT 
Comment on attachment 279974 [details]
Patch

Ok, this latest patch should do what Anders suggested.
Comment 47 Chris Dumez 2016-05-27 11:51:17 PDT 
Comment on attachment 279973 [details]
Patch

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

>> Source/WTF/wtf/FunctionDispatcher.h:34
>> +class LambdaWrapperBase {
> 
> Should these be called LambdaWrapper or FunctionWrapper?

Technically, the idea is to usually wrap a lambda, not an std::function. However, it will happily wrap anything that is callable (so a lambda or an std::function).
I could go with FunctionWrapper but I worry it may be confused as std::function wrapper. Maybe CallableWrapper?

>> Source/WTF/wtf/FunctionDispatcher.h:42
>> +template<typename F, class = typename std::enable_if<std::is_rvalue_reference<F&&>::value>::type>
> 
> Is it better to use the "F" for this argument rather than a name like FunctionType?

Maybe LambdaType or CallableType?
Comment 48 Darin Adler 2016-05-27 11:51:36 PDT 
Comment on attachment 279974 [details]
Patch

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

Most of my comments from the last patch still apply. Sorry, caught you at just the wrong time, I think. Please read those comments.

> Source/WTF/wtf/FunctionDispatcher.h:107
> +using WTF::NoncopyableFunction;
>  using WTF::FunctionDispatcher;

Normally we keep these in alphabetical order.

> Source/WTF/wtf/cocoa/WorkQueueCocoa.cpp:29
> +#include <wtf/RefPtr.h>

What creates the need for this include? Doesn’t make sense given the code I’m reading below.

> Source/WTF/wtf/efl/DispatchQueueWorkItemEfl.h:52
> +    static std::unique_ptr<TimerWorkItem> create(PassRefPtr<WorkQueue> workQueue, NoncopyableFunction&& function, std::chrono::nanoseconds delayNanoSeconds)

Amazing discipline, seeing this PassRefPtr and being able to leave it alone!
Comment 49 Chris Dumez 2016-05-27 11:58:09 PDT 
Comment on attachment 279973 [details]
Patch

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

>> Source/WTF/wtf/FunctionDispatcher.h:64
>> +    NoncopyableFunction(F&& lambda)
> 
> Should this be marked explicit or not?

The idea is to behave like a std::function (just not copyable), si I'd say no. The corresponding constructor is not explicit on std::function.

dispatch(NoncopyableFunction([]() {})) would look ugly.
Comment 50 Chris Dumez 2016-05-27 12:11:21 PDT 
Created attachment 279979 [details]
Patch
Comment 51 WebKit Commit Bot 2016-05-27 12:12:43 PDT 
Attachment 279979 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:66:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:72:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:424:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:556:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:574:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:586:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:605:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:626:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:655:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:674:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 15 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 52 Chris Dumez 2016-05-27 12:13:28 PDT 
Thus patch should take all Darin's feedback into consideration. However, I went with the naming:
* F -> CallableType
* LambdaWrapper -> CallableWrapper
* lambda -> callable

Let me know if you don't like this naming.
Comment 53 Chris Dumez 2016-05-27 12:14:44 PDT 
Created attachment 279981 [details]
Patch
Comment 54 WebKit Commit Bot 2016-05-27 12:16:03 PDT 
Attachment 279981 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:66:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:72:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:424:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:556:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:574:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:586:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:605:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:626:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:655:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:674:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 15 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 55 Chris Dumez 2016-05-27 12:24:20 PDT 
Created attachment 279983 [details]
Patch
Comment 56 Chris Dumez 2016-05-27 12:25:22 PDT 
Comment on attachment 279983 [details]
Patch

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

> Source/WTF/wtf/FunctionDispatcher.h:71
> +    class CallableWrapper final : public CallableWrapperBase {

I moved this class inside NoncopyableFunction as per Anders feedback offline.
Comment 57 WebKit Commit Bot 2016-05-27 12:25:38 PDT 
Attachment 279983 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:64:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:66:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.h:72:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:424:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:556:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:574:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:586:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:605:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:626:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:655:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:674:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 15 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 58 WebKit Commit Bot 2016-05-27 13:21:01 PDT 
Comment on attachment 279983 [details]
Patch

Clearing flags on attachment: 279983

Committed r201464: <http://trac.webkit.org/changeset/201464>
Comment 59 WebKit Commit Bot 2016-05-27 13:21:09 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 60 Yusuke Suzuki 2016-05-27 21:17:50 PDT 
Committed r201479: <http://trac.webkit.org/changeset/201479>