Bug 123804

Summary:

Manage ScriptExecutionContext::Task derivatives through std::unique_ptr

Product:

WebKit

Reporter:

Zan Dobersek <zan>

Component:

New Bugs

Assignee:

Zan Dobersek <zan>

Status:

RESOLVED INVALID

Severity:

Normal

CC:

andersca, bfulgham, buildbot, commit-queue, gtk-ews, kling, koivisto, rego+ews, rniwa, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

128007

Attachments:

Description	Flags
Patch	buildbot: commit-queue-
Another try	gtk-ews: commit-queue-
Third attempt	none
Patch	kling: review+

Zan Dobersek

Reported 2013-11-05 08:42:11 PST

Manage ScriptExecutionContext::Task derivatives through std::unique_ptr

Attachments
Patch (88.76 KB, patch) 2013-11-05 09:34 PST, Zan Dobersek	buildbot: commit-queue-	Details Formatted Diff Diff
Another try (77.92 KB, patch) 2013-11-07 23:38 PST, Zan Dobersek	gtk-ews: commit-queue-	Details Formatted Diff Diff
Third attempt (78.41 KB, patch) 2013-11-08 11:30 PST, Zan Dobersek	no flags	Details Formatted Diff Diff
Patch (81.34 KB, patch) 2013-11-12 13:56 PST, Zan Dobersek	kling: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Zan Dobersek

Comment 1 2013-11-05 09:34:12 PST

Created attachment 216048 [details] Patch

WebKit Commit Bot

Comment 2 2013-11-05 09:36:29 PST

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

Comment 3 2013-11-05 09:36:45 PST

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 4 2013-11-05 10:26:07 PST

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 5 2013-11-05 10:31:46 PST

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 6 2013-11-05 11:12:43 PST

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 7 2013-11-05 12:45:46 PST

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 8 2013-11-05 23:54:37 PST

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

Comment 9 2013-11-07 23:38:39 PST

Created attachment 216362 [details] Another try Testing through EWS.

WebKit Commit Bot

Comment 10 2013-11-07 23:43:33 PST

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 11 2013-11-07 23:54:25 PST

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 12 2013-11-08 00:33:04 PST

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

Comment 13 2013-11-08 11:30:38 PST

Created attachment 216418 [details] Third attempt Fixes the GTK build, dunno why/how the Win build is failing.

WebKit Commit Bot

Comment 14 2013-11-08 11:31:41 PST

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 15 2013-11-08 12:31:37 PST

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

Comment 16 2013-11-08 14:24:24 PST

(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

Comment 17 2013-11-12 13:56:12 PST

Created attachment 216720 [details] Patch

Andreas Kling

Comment 18 2014-02-05 17:35:16 PST

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

Comment 19 2014-08-04 09:24:24 PDT

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

Note You need to log in before you can comment on or make changes to this bug.