Summary: | Add WTF::crossThreadCopy(T&&) to utilize String::isolatedCopy() && | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Fujii Hironori <Hironori.Fujii> | ||||||||
Component: | Web Template Framework | Assignee: | Fujii Hironori <Hironori.Fujii> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, Basuke.Suzuki, beidson, cdumez, darin, ews-watchlist, ggaren, mitz, rniwa, sam, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=133968 | ||||||||||
Attachments: |
|
Description
Fujii Hironori
2019-06-18 00:27:48 PDT
Created attachment 372330 [details]
WIP patch
Attachment 372330 [details] did not pass style-queue:
ERROR: Tools/TestWebKitAPI/Tests/WTF/CrossThreadCopier.cpp:28: You should add a blank line after implementation file's own header. [build/include_order] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/CrossThreadCopier.cpp:28: You should not add a blank line before implementation file's own header. [build/include_order] [4]
Total errors found: 2 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 372332 [details]
Patch
Comment on attachment 372332 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372332&action=review Great optimization. r=me > Tools/TestWebKitAPI/Tests/WTF/CrossThreadCopier.cpp:48 > + // moved-out RefPtr becomes nullptr This comment isn't needed. Could you also add a test that has two references to original, moves one into crossThreadCopy, and verifies that an isolated copy is indeed made? > Tools/TestWebKitAPI/Tests/WTF/CrossThreadCopier.cpp:66 > + // moved-out RefPtr becomes nullptr Ditto, unnecessary comment. Comment on attachment 372332 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372332&action=review To actually improve WebKit, we have more tasks: 1. Replace crossThreadCopy(something) → crossThreadCopy(WTFMove(something)) if possible 2. Add &&-qualified isolatedCopy method to some classes. Volunteers are welcome. >> Tools/TestWebKitAPI/Tests/WTF/CrossThreadCopier.cpp:48 >> + // moved-out RefPtr becomes nullptr > > This comment isn't needed. > Could you also add a test that has two references to original, moves one into crossThreadCopy, and verifies that an isolated copy is indeed made? It's a good idea. Will do. Comment on attachment 372332 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372332&action=review > Source/WTF/wtf/CrossThreadCopier.h:158 > + static Type copy(Type&& source) I can use a forwarding reference for this by fixing CrossThreadTask issue. Will fix. Just for the record, here is the compilation error of CrossThreadTask I met. > [4/808] Building CXX object Tools\TestWebKitAPI\CMakeFiles\TestWTFLib.dir\Tests\WTF\CrossThreadTask.cpp.obj > FAILED: Tools/TestWebKitAPI/CMakeFiles/TestWTFLib.dir/Tests/WTF/CrossThreadTask.cpp.obj > C:\PROGRA~1\LLVM\bin\clang-cl.exe /nologo -TP -DBUILDING_TestWTF -DBUILDING_WITH_CMAKE=1 -DGTEST_CREATE_SHARED_LIBRARY=0 -DGTEST_HAS_PTHREAD=0 -DGTEST_HAS_RTTI=0 -DHAVE_CONFIG_H=1 -DNOMINMAX -DTestWTFLib_EXPORTS -DUNICODE -DWINVER=0x601 -DWTF_PLATFORM_WIN_CAIRO=1 -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_UNICODE -D_WIN32_WINNT=0x601 -D_WINDOWS -D_WINSOCKAPI_="" -I. -I..\..\Tools\TestWebKitAPI -I..\..\Source\ThirdParty\gtest\include -IWTF\Headers -IDerivedSources -I..\..\Source\ThirdParty -I..\..\WebKitLibraries\win\include /W4 -fdiagnostics-color=always -fcolor-diagnostics -Wno-parentheses-equality -Wno-noexcept-type -Qunused-arguments -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -Wno-unknown-argument -Wno-nonportable-include-path -Wno-unknown-pragmas -Wno-macro-redefined -Wno-undef /DWIN32 /D_WINDOWS /GR- /EHsc- -fno-strict-aliasing /MD /Zi /Ob0 /Od /RTC1 /wd4018 /wd4068 /wd4099 /wd4100 /wd4127 /wd4138 /wd4146 /wd4180 /wd4189 /wd4201 /wd4206 /wd4244 /wd4251 /wd4267 /wd4275 /wd4288 /wd4291 /wd4305 /wd4309 /wd4344 /wd4355 /wd4389 /wd4396 /wd4456 /wd4457 /wd4458 /wd4459 /wd4481 /wd4503 /wd4505 /wd4510 /wd4512 /wd4530 /wd4610 /wd4611 /wd4646 /wd4702 /wd4706 /wd4722 /wd4800 /wd4819 /wd4951 /wd4952 /wd4996 /wd6011 /wd6031 /wd6211 /wd6246 /wd6255 /wd6387 /Zi /GS /EHa- /EHc- /EHs- /fp:except- /analyze- /bigobj /std:c++17 /utf-8 /validate-charset -fmsc-version=1911 /showIncludes /FoTools\TestWebKitAPI\CMakeFiles\TestWTFLib.dir\Tests\WTF\CrossThreadTask.cpp.obj /FdTools\TestWebKitAPI\CMakeFiles\TestWTFLib.dir\ -c ..\..\Tools\TestWebKitAPI\Tests\WTF\CrossThreadTask.cpp > In file included from ..\..\Tools\TestWebKitAPI\Tests\WTF\CrossThreadTask.cpp:28: > WTF\Headers\wtf/CrossThreadTask.h(70,65): error: no matching function for call to 'crossThreadCopy' > return CrossThreadTask([method, arguments = std::make_tuple(crossThreadCopy<Arguments>(arguments)...)]() mutable { > ^~~~~~~~~~~~~~~~~~~~~~~~~~ > ..\..\Tools\TestWebKitAPI\Tests\WTF\CrossThreadTask.cpp(111,21): note: in instantiation of function template specialization 'WTF::createCrossThreadTask<const TestWebKitAPI::LifetimeLogger &, const TestWebKitAPI::LifetimeLogger &, const TestWebKitAPI::LifetimeLogger &, TestWebKitAPI::LifetimeLogger, TestWebKitAPI::LifetimeLogger, TestWebKitAPI::LifetimeLogger>' requested here > auto task = createCrossThreadTask(testFunction, logger1, logger2, logger3); > ^ > WTF\Headers\wtf/CrossThreadCopier.h(166,27): note: candidate function template not viable: 1st argument ('const TestWebKitAPI::LifetimeLogger') would lose const qualifier > template<typename T> auto crossThreadCopy(T&& source) > ^ > 1 error generated. Created attachment 372422 [details]
Patch for landing
Committed r246623: <https://trac.webkit.org/changeset/246623> |