Bug 123804 – Manage ScriptExecutionContext::Task derivatives through std::unique_ptr

• Component: New Bugs
• Status: RESOLVED INVALID

[Zan Dobersek]

Manage ScriptExecutionContext::Task derivatives through std::unique_ptr

[Zan Dobersek]

Created attachment 216048 [details]
Patch

[WebKit Commit Bot]

Attachment 216048 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/StdLibExtras.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/quota/StorageErrorCallback.h', u'Source/WebCore/Modules/quota/StorageInfo.cpp', u'Source/WebCore/Modules/webdatabase/Database.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseManager.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseSync.cpp', u'Source/WebCore/Modules/webdatabase/SQLCallbackWrapper.h', u'Source/WebCore/Modules/webdatabase/SQLStatement.cpp', u'Source/WebCore/Modules/webdatabase/SQLTransaction.cpp', u'Source/WebCore/Modules/websockets/ThreadableWebSocketChannelClientWrapper.cpp', u'Source/WebCore/Modules/websockets/ThreadableWebSocketChannelClientWrapper.h', u'Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp', u'Source/WebCore/bindings/js/JSCallbackData.h', u'Source/WebCore/bindings/js/JSDOMGlobalObjectTask.h', u'Source/WebCore/bindings/js/JSDOMWindowBase.cpp', u'Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp', u'Source/WebCore/bindings/scripts/CodeGeneratorJS.pm', u'Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp', u'Source/WebCore/dom/CrossThreadTask.h', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/dom/Document.h', u'Source/WebCore/dom/ScriptExecutionContext.cpp', u'Source/WebCore/dom/ScriptExecutionContext.h', u'Source/WebCore/dom/StringCallback.cpp', u'Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp', u'Source/WebCore/workers/DefaultSharedWorkerRepository.cpp', u'Source/WebCore/workers/WorkerEventQueue.cpp', u'Source/WebCore/workers/WorkerGlobalScope.cpp', u'Source/WebCore/workers/WorkerGlobalScope.h', u'Source/WebCore/workers/WorkerLoaderProxy.h', u'Source/WebCore/workers/WorkerMessagingProxy.cpp', u'Source/WebCore/workers/WorkerMessagingProxy.h', u'Source/WebCore/workers/WorkerRunLoop.cpp', u'Source/WebCore/workers/WorkerRunLoop.h', u'Source/WebCore/workers/WorkerThread.cpp']" exit_code: 1
Source/WebCore/Modules/webdatabase/SQLStatement.cpp:105:  std::unique_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/Modules/webdatabase/SQLStatement.cpp:106:  std::unique_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:305:  std::unique_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:306:  std::unique_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:307:  std::unique_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

WTF_MAKE_UNIQUE and WTF_MAKE_UNIQUE_ARGS macros work well under Clang 3.2 and up and g++ 4.7 and up. They don't function under g++ 4.6, which some ports still want to support. No idea about MSVC.

[Build Bot]

Comment on attachment 216048 [details]
Patch

Attachment 216048 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/21168027

[Build Bot]

Comment on attachment 216048 [details]
Patch

Attachment 216048 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/21228023

[Build Bot]

Comment on attachment 216048 [details]
Patch

Attachment 216048 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/21168031

[Build Bot]

Comment on attachment 216048 [details]
Patch

Attachment 216048 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/20068107

[Zan Dobersek]

Comment on attachment 216048 [details]
Patch

The Mac build failed due to a couple undefined references which extra explicit template instantiations would fix. MSVC doesn't play ball with the std::make_unique friend declarations though, so that won't be possible to use.

I'll update the patch with public constructors that std::make_unique won't have problems accessing.

[Zan Dobersek]

Created attachment 216362 [details]
Another try

Testing through EWS.

[WebKit Commit Bot]

Attachment 216362 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/wtf/Compiler.h', u'Source/WTF/wtf/StdLibExtras.h', u'Source/WebCore/Modules/quota/StorageErrorCallback.h', u'Source/WebCore/Modules/quota/StorageInfo.cpp', u'Source/WebCore/Modules/webdatabase/Database.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseManager.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseSync.cpp', u'Source/WebCore/Modules/webdatabase/SQLCallbackWrapper.h', u'Source/WebCore/Modules/websockets/ThreadableWebSocketChannelClientWrapper.cpp', u'Source/WebCore/Modules/websockets/ThreadableWebSocketChannelClientWrapper.h', u'Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp', u'Source/WebCore/bindings/js/JSCallbackData.h', u'Source/WebCore/bindings/js/JSDOMGlobalObjectTask.h', u'Source/WebCore/bindings/js/JSDOMWindowBase.cpp', u'Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp', u'Source/WebCore/bindings/scripts/CodeGeneratorJS.pm', u'Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp', u'Source/WebCore/dom/CrossThreadTask.h', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/dom/Document.h', u'Source/WebCore/dom/ScriptExecutionContext.cpp', u'Source/WebCore/dom/ScriptExecutionContext.h', u'Source/WebCore/dom/StringCallback.cpp', u'Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp', u'Source/WebCore/workers/DefaultSharedWorkerRepository.cpp', u'Source/WebCore/workers/WorkerEventQueue.cpp', u'Source/WebCore/workers/WorkerGlobalScope.cpp', u'Source/WebCore/workers/WorkerGlobalScope.h', u'Source/WebCore/workers/WorkerLoaderProxy.h', u'Source/WebCore/workers/WorkerMessagingProxy.cpp', u'Source/WebCore/workers/WorkerMessagingProxy.h', u'Source/WebCore/workers/WorkerRunLoop.cpp', u'Source/WebCore/workers/WorkerRunLoop.h', u'Source/WebCore/workers/WorkerThread.cpp']" exit_code: 1
Source/WTF/wtf/StdLibExtras.h:454:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/StdLibExtras.h:459:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/StdLibExtras.h:464:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/StdLibExtras.h:469:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/StdLibExtras.h:474:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/StdLibExtras.h:479:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/StdLibExtras.h:484:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/StdLibExtras.h:489:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/StdLibExtras.h:494:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/StdLibExtras.h:499:  Missing spaces around &&  [whitespace/operators] [3]
Total errors found: 10 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[kov's GTK+ EWS bot]

Comment on attachment 216362 [details]
Another try

Attachment 216362 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/22368424

[Build Bot]

Comment on attachment 216362 [details]
Another try

Attachment 216362 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/22538395

[Zan Dobersek]

Created attachment 216418 [details]
Third attempt

Fixes the GTK build, dunno why/how the Win build is failing.

[WebKit Commit Bot]

Attachment 216418 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/wtf/Compiler.h', u'Source/WTF/wtf/NeverDestroyed.h', u'Source/WTF/wtf/StdLibExtras.h', u'Source/WebCore/Modules/quota/StorageErrorCallback.h', u'Source/WebCore/Modules/quota/StorageInfo.cpp', u'Source/WebCore/Modules/webdatabase/Database.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseManager.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseSync.cpp', u'Source/WebCore/Modules/webdatabase/SQLCallbackWrapper.h', u'Source/WebCore/Modules/websockets/ThreadableWebSocketChannelClientWrapper.cpp', u'Source/WebCore/Modules/websockets/ThreadableWebSocketChannelClientWrapper.h', u'Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp', u'Source/WebCore/bindings/js/JSCallbackData.h', u'Source/WebCore/bindings/js/JSDOMGlobalObjectTask.h', u'Source/WebCore/bindings/js/JSDOMWindowBase.cpp', u'Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp', u'Source/WebCore/bindings/scripts/CodeGeneratorJS.pm', u'Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp', u'Source/WebCore/dom/CrossThreadTask.h', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/dom/Document.h', u'Source/WebCore/dom/ScriptExecutionContext.cpp', u'Source/WebCore/dom/ScriptExecutionContext.h', u'Source/WebCore/dom/StringCallback.cpp', u'Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp', u'Source/WebCore/workers/DefaultSharedWorkerRepository.cpp', u'Source/WebCore/workers/WorkerEventQueue.cpp', u'Source/WebCore/workers/WorkerGlobalScope.cpp', u'Source/WebCore/workers/WorkerGlobalScope.h', u'Source/WebCore/workers/WorkerLoaderProxy.h', u'Source/WebCore/workers/WorkerMessagingProxy.cpp', u'Source/WebCore/workers/WorkerMessagingProxy.h', u'Source/WebCore/workers/WorkerRunLoop.cpp', u'Source/WebCore/workers/WorkerRunLoop.h', u'Source/WebCore/workers/WorkerThread.cpp']" exit_code: 1
Source/WTF/wtf/StdLibExtras.h:454:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/StdLibExtras.h:459:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/StdLibExtras.h:464:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/StdLibExtras.h:469:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/StdLibExtras.h:474:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/StdLibExtras.h:479:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/StdLibExtras.h:484:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/StdLibExtras.h:489:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/StdLibExtras.h:494:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/StdLibExtras.h:499:  Missing spaces around &&  [whitespace/operators] [3]
Total errors found: 10 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 216418 [details]
Third attempt

Attachment 216418 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/22778508

[Zan Dobersek]

(In reply to comment #15)
> (From update of attachment 216418 [details])
> Attachment 216418 [details] did not pass win-ews (win):
> Output: http://webkit-queues.appspot.com/results/22778508

AFAIU MSVC is crashing. Fun.

This was pretty much the last attempt at trying to add support for std::make_unique friendships that could keep the constructors private. MSVC already required a dumbed-down approach to start with (even MSVC 2012 doesn't process the decltype() approach), so I'm giving up on this for the moment and I'll be uploading a version of this that is making the relevant constructors public.

[Zan Dobersek]

Created attachment 216720 [details]
Patch

[Andreas Kling]

Comment on attachment 216720 [details]
Patch

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

This looks fine, so I'm gonna say r=me.
You'll probably need to fix up some things, like s/OVERRIDE/override/ for it to build.

It would also be good to go over these classes and spam final/override as appropriate.

> Source/WebCore/Modules/webdatabase/Database.cpp:89
> +    DerefContextTask(PassRefPtr<ScriptExecutionContext> context)

explicit

> Source/WebCore/Modules/webdatabase/Database.cpp:203
> +    DeliverPendingCallbackTask(PassRefPtr<SQLTransaction> transaction)

explicit

> Source/WebCore/Modules/websockets/ThreadableWebSocketChannelClientWrapper.cpp:237
> +    Vector<std::unique_ptr<ScriptExecutionContext::Task>> tasks = std::move(m_pendingTasks);
> +    for (auto iter = tasks.begin(); iter != tasks.end(); ++iter)
>          (*iter)->performTask(0);

I'd go all in and write this like so:

auto tasks = std::move(m_pendingTasks);
for (auto& task : tasks)
    task.performTask(0);

> Source/WebCore/bindings/js/JSCallbackData.h:80
> +    DeleteCallbackDataTask(JSCallbackData* data) : m_data(data) { }

explicit

> Source/WebCore/workers/WorkerMessagingProxy.cpp:139
>      WorkerGlobalScopeDestroyedTask(WorkerMessagingProxy* messagingProxy)

explicit

> Source/WebCore/workers/WorkerMessagingProxy.cpp:212
>      NotifyNetworkStateChangeTask(bool isOnLine)

explicit

[Zan Dobersek]

This isn't necessary anymore now that ScriptExecutionContext::Task objects are just lambdas (bug #129795).