Bug 193602

Summary: Move FileSystem to WTF
Product: WebKit Reporter: Ross Kirsling <ross.kirsling>
Component: New BugsAssignee: Ross Kirsling <ross.kirsling>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, don.olmstead, ews-watchlist, fujii.hironori, keith_miller, mcatanzaro, saam, tsavell, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=193789
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Ross Kirsling
Reported 2019-01-18 16:52:01 PST
Move FileSystem to WTF
Attachments
Patch (414.25 KB, patch)
2019-01-18 16:55 PST, Ross Kirsling 		no flags
DetailsFormatted DiffDiff
Patch (414.28 KB, patch)
2019-01-18 17:04 PST, Ross Kirsling 		no flags
DetailsFormatted DiffDiff
Patch (424.95 KB, patch)
2019-01-18 22:45 PST, Ross Kirsling 		no flags
DetailsFormatted DiffDiff
Patch (425.97 KB, patch)
2019-01-21 19:07 PST, Ross Kirsling 		no flags
DetailsFormatted DiffDiff
Patch (426.38 KB, patch)
2019-01-21 22:07 PST, Ross Kirsling 		no flags
DetailsFormatted DiffDiff
Patch (427.03 KB, patch)
2019-01-22 18:03 PST, Ross Kirsling 		no flags
DetailsFormatted DiffDiff
Patch (427.10 KB, patch)
2019-01-22 18:17 PST, Ross Kirsling 		no flags
DetailsFormatted DiffDiff
Patch (426.50 KB, patch)
2019-01-22 19:41 PST, Ross Kirsling 		no flags
DetailsFormatted DiffDiff
Patch (426.70 KB, patch)
2019-01-23 02:09 PST, Fujii Hironori 		no flags
DetailsFormatted DiffDiff
Patch (426.84 KB, patch)
2019-01-23 11:34 PST, Ross Kirsling 		no flags
DetailsFormatted DiffDiff
Patch (426.80 KB, patch)
2019-01-23 14:40 PST, Ross Kirsling 		no flags
DetailsFormatted DiffDiff
Patch (426.75 KB, patch)
2019-01-23 19:12 PST, Ross Kirsling 		no flags
DetailsFormatted DiffDiff
Show Obsolete (11) View All Add attachment proposed patch, testcase, etc.
Ross Kirsling
Comment 1 2019-01-18 16:55:41 PST
Created attachment 359556 [details] Patch
Ross Kirsling
Comment 2 2019-01-18 17:04:59 PST
Created attachment 359559 [details] Patch
EWS Watchlist
Comment 3 2019-01-18 17:08:17 PST Comment hidden (obsolete)
Ross Kirsling
Comment 4 2019-01-18 17:10:46 PST
(In reply to Build Bot from comment #3) > Attachment 359559 [details] did not pass style-queue: > > > ERROR: Source/WTF/wtf/win/PathWalker.cpp:27: Found other header before a > header this file implements. Should be: config.h, primary header, blank > line, and then alphabetically sorted. [build/include_order] [4] > ERROR: Source/WTF/wtf/win/PathWalker.cpp:28: Alphabetical sorting problem. > [build/include_order] [4] > ERROR: Source/WTF/wtf/FileSystem.cpp:28: Found other header before a header > this file implements. Should be: config.h, primary header, blank line, and > then alphabetically sorted. [build/include_order] [4] > ERROR: Source/WTF/wtf/FileSystem.cpp:29: Alphabetical sorting problem. > [build/include_order] [4] > ERROR: Source/WTF/wtf/win/FileSystemWin.cpp:32: Alphabetical sorting > problem. [build/include_order] [4] > ERROR: Source/WTF/wtf/posix/FileSystemPOSIX.cpp:31: Alphabetical sorting > problem. [build/include_order] [4] > ERROR: Source/WTF/wtf/glib/FileSystemGlib.cpp:25: Alphabetical sorting > problem. [build/include_order] [4] > ERROR: Source/WTF/wtf/cf/FileSystemCF.cpp:31: Alphabetical sorting problem. > [build/include_order] [4] > Total errors found: 8 in 183 files > > > If any of these errors are false positives, please file a bug against > check-webkit-style. IIUC, these are all the canonical style for WTF.
Ross Kirsling
Comment 5 2019-01-18 22:45:10 PST
Created attachment 359589 [details] Patch
EWS Watchlist
Comment 6 2019-01-18 22:50:13 PST Comment hidden (obsolete)
Ross Kirsling
Comment 7 2019-01-21 19:07:39 PST
Created attachment 359718 [details] Patch
EWS Watchlist
Comment 8 2019-01-21 19:11:07 PST Comment hidden (obsolete)
Ross Kirsling
Comment 9 2019-01-21 22:07:32 PST
Created attachment 359719 [details] Patch
EWS Watchlist
Comment 10 2019-01-21 22:10:34 PST Comment hidden (obsolete)
Ross Kirsling
Comment 11 2019-01-22 14:59:57 PST
It's really perplexing why GTK and WPE continue to have a problem here -- seems like the compiler doesn't understand namespace aliases: > In file included from ../../Source/WebCore/Modules/cache/DOMCacheEngine.h:34:0, > from ../../Source/WebCore/Modules/cache/CacheStorageConnection.h:29, > from ../../Source/WebCore/workers/WorkerGlobalScope.h:30, > from ../../Source/WebCore/inspector/InspectorInstrumentation.h:49, > from ../../Source/WebCore/dom/EventTarget.cpp:38, > from /home/ews/gtk-wk2-ews/WebKit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource-be65d27a-8.cpp:5: > ../../Source/WebCore/platform/SharedBuffer.h:139:40: error: reference to ‘FileSystem’ is ambiguous > static Ref<DataSegment> create(FileSystem::MappedFileData&& data) { return adoptRef(*new DataSegment(WTFMove(data))); } > ^~~~~~~~~~ > In file included from ../../Source/WebCore/platform/SharedBuffer.h:30:0, > from ../../Source/WebCore/Modules/cache/DOMCacheEngine.h:34, > from ../../Source/WebCore/Modules/cache/CacheStorageConnection.h:29, > from ../../Source/WebCore/workers/WorkerGlobalScope.h:30, > from ../../Source/WebCore/inspector/InspectorInstrumentation.h:49, > from ../../Source/WebCore/dom/EventTarget.cpp:38, > from /home/ews/gtk-wk2-ews/WebKit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource-be65d27a-8.cpp:5: > DerivedSources/ForwardingHeaders/wtf/FileSystem.h:226:39: note: candidates are: namespace FileSystem = WTF::WTF::FileSystem; > namespace FileSystem = WTF::FileSystem; > ^ > In file included from ../../Source/WebCore/platform/SharedBuffer.h:30:0, > from ../../Source/WebCore/Modules/cache/DOMCacheEngine.h:34, > from ../../Source/WebCore/Modules/cache/CacheStorageConnection.h:29, > from ../../Source/WebCore/workers/WorkerGlobalScope.h:30, > from ../../Source/WebCore/inspector/InspectorInstrumentation.h:49, > from ../../Source/WebCore/dom/EventTarget.cpp:38, > from /home/ews/gtk-wk2-ews/WebKit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource-be65d27a-8.cpp:5: > DerivedSources/ForwardingHeaders/wtf/FileSystem.h:63:11: note: namespace WTF::FileSystem { } > namespace FileSystem { > ^~~~~~~~~~ EventTarget.cpp does have `using namespace WTF;` but removing it in the latest patch here didn't help. This isn't really surprising though, since it's sitting safely within the `namespace WebCore {}` block.
Ross Kirsling
Comment 12 2019-01-22 18:03:06 PST
Created attachment 359827 [details] Patch
EWS Watchlist
Comment 13 2019-01-22 18:06:54 PST Comment hidden (obsolete)
Ross Kirsling
Comment 14 2019-01-22 18:17:28 PST
Created attachment 359830 [details] Patch
Ross Kirsling
Comment 15 2019-01-22 19:41:30 PST
Created attachment 359839 [details] Patch
EWS Watchlist
Comment 16 2019-01-22 19:45:37 PST Comment hidden (obsolete)
Fujii Hironori
Comment 17 2019-01-23 02:09:17 PST
Created attachment 359862 [details] Patch * Fixed GTK+ builds.
EWS Watchlist
Comment 18 2019-01-23 02:12:45 PST Comment hidden (obsolete)
Ross Kirsling
Comment 19 2019-01-23 10:08:15 PST
(In reply to Ross Kirsling from comment #11) > It's really perplexing why GTK and WPE continue to have a problem here -- > seems like the compiler doesn't understand namespace aliases: > > ... > > EventTarget.cpp does have `using namespace WTF;` but removing it in the > latest patch here didn't help. This isn't really surprising though, since > it's sitting safely within the `namespace WebCore {}` block. Ugh, it seems to be a bug in GCC 6 and 7: https://godbolt.org/z/-00gZR Not sure what to do about this. I guess for WTF::Unicode we don't use a namespace alias and instead sprinkle `using namespace WTF::Unicode;` all over the place... The other possibility is to remove `using namespace WTF;` throughout WebCore. If we do the latter, I suppose it would make sense to split it out as its own patch first.
Ross Kirsling
Comment 20 2019-01-23 11:34:52 PST
Created attachment 359913 [details] Patch
EWS Watchlist
Comment 21 2019-01-23 11:38:32 PST Comment hidden (obsolete)
Ross Kirsling
Comment 22 2019-01-23 13:26:50 PST
I guess pleasing GCC 6 was as simple as renaming the inner namespace to FileSystemImpl, following the precedent here: https://github.com/WebKit/webkit/blob/master/Source/WTF/wtf/JSONValues.h#L45 Now if I could just figure out why this patch causes API test timeouts for WinCairo...
Alex Christensen
Comment 23 2019-01-23 14:12:19 PST
Comment on attachment 359913 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359913&action=review > Source/WTF/ChangeLog:3 > + Move FileSystem to WTF Why? To me it seems like PAL is the right place for it.
Ross Kirsling
Comment 24 2019-01-23 14:40:33 PST
Created attachment 359945 [details] Patch
EWS Watchlist
Comment 25 2019-01-23 14:44:05 PST Comment hidden (obsolete)
Yusuke Suzuki
Comment 26 2019-01-23 16:06:39 PST
Comment on attachment 359913 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359913&action=review >> Source/WTF/ChangeLog:3 >> + Move FileSystem to WTF > > Why? To me it seems like PAL is the right place for it. I like the idea moving FileSystem to WTF because JSC shell starts testing persistent bytecode cache which resides in the FileSystem anyway. https://trac.webkit.org/changeset/240255/webkit' If some functions can be shared, it is nice.
Ross Kirsling
Comment 27 2019-01-23 16:22:31 PST
(In reply to Ross Kirsling from comment #22) > Now if I could just figure out why this patch causes API test timeouts for > WinCairo... Scratch that; evidently this is an existing issue. (In reply to Yusuke Suzuki from comment #26) > >> Source/WTF/ChangeLog:3 > >> + Move FileSystem to WTF > > > > Why? To me it seems like PAL is the right place for it. > > I like the idea moving FileSystem to WTF because JSC shell starts testing > persistent bytecode cache which resides in the FileSystem anyway. > https://trac.webkit.org/changeset/240255/webkit' > If some functions can be shared, it is nice. Awesome. :D We're also intending to use FileSystem for PlayStation port's JSC shell.
Ross Kirsling
Comment 28 2019-01-23 19:12:56 PST
Created attachment 359985 [details] Patch
EWS Watchlist
Comment 29 2019-01-23 19:17:12 PST
Attachment 359985 [details] did not pass style-queue: ERROR: Source/WTF/wtf/win/PathWalker.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WTF/wtf/win/PathWalker.cpp:28: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/FileSystem.cpp:28: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WTF/wtf/FileSystem.cpp:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/win/FileSystemWin.cpp:32: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/posix/FileSystemPOSIX.cpp:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/glib/FileSystemGlib.cpp:25: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/cf/FileSystemCF.cpp:31: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 8 in 184 files If any of these errors are false positives, please file a bug against check-webkit-style.
Yusuke Suzuki
Comment 30 2019-01-23 19:29:12 PST
Comment on attachment 359985 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359985&action=review r=me. This is beneficial for JSC shell testing and Cached bytecodes implementations too. > Source/WTF/ChangeLog:17 > + * wtf/PlatformWin.cmake: Can you add PlatformJSCOnly.cmake changes too?
Ross Kirsling
Comment 31 2019-01-23 19:52:32 PST
(In reply to Yusuke Suzuki from comment #30) > Comment on attachment 359985 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=359985&action=review > > r=me. This is beneficial for JSC shell testing and Cached bytecodes > implementations too. Thanks! > > Source/WTF/ChangeLog:17 > > + * wtf/PlatformWin.cmake: > > Can you add PlatformJSCOnly.cmake changes too? Hmm, do you think it's better to add them now, or should we wait until JSC actually consumes FileSystem?
Ross Kirsling
Comment 32 2019-01-24 09:13:20 PST
(In reply to Ross Kirsling from comment #31) > (In reply to Yusuke Suzuki from comment #30) > > > Source/WTF/ChangeLog:17 > > > + * wtf/PlatformWin.cmake: > > > > Can you add PlatformJSCOnly.cmake changes too? > > Hmm, do you think it's better to add them now, or should we wait until JSC > actually consumes FileSystem? Since the current patch is pleasing EWS and JSCOnly will require separate verification, I'll land this one now and handle the rest in a separate patch this morning. :)
WebKit Commit Bot
Comment 33 2019-01-24 09:26:07 PST
Comment on attachment 359985 [details] Patch Clearing flags on attachment: 359985 Committed r240437: <https://trac.webkit.org/changeset/240437>
WebKit Commit Bot
Comment 34 2019-01-24 09:26:09 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 35 2019-01-24 09:27:36 PST
<rdar://problem/47518054>
Truitt Savell
Comment 36 2019-01-24 14:07:57 PST
Looks like the changes in https://trac.webkit.org/changeset/240437/webkit has caused an API failure on Mac Debug. Log: https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20WK1%20%28Tests%29/builds/7412/steps/run-api-tests/logs/stdio reproduced with: run-api-tests --root debug-240437 --debug I received no failure on 240436.
Ross Kirsling
Comment 37 2019-01-24 14:28:14 PST
(In reply to Truitt Savell from comment #36) > Looks like the changes in https://trac.webkit.org/changeset/240437/webkit > > has caused an API failure on Mac Debug. > > Log: > https://build.webkit.org/builders/ > Apple%20High%20Sierra%20Debug%20WK1%20%28Tests%29/builds/7412/steps/run-api- > tests/logs/stdio > > reproduced with: > run-api-tests --root debug-240437 --debug > > I received no failure on 240436. I don't understand how this patch can have any impact on TestWTF.WTF.StringOperators, but I will take a look... :(
Ross Kirsling
Comment 38 2019-01-24 15:19:40 PST
(In reply to Ross Kirsling from comment #37) > (In reply to Truitt Savell from comment #36) > > Looks like the changes in https://trac.webkit.org/changeset/240437/webkit > > > > has caused an API failure on Mac Debug. > > > > Log: > > https://build.webkit.org/builders/ > > Apple%20High%20Sierra%20Debug%20WK1%20%28Tests%29/builds/7412/steps/run-api- > > tests/logs/stdio > > > > reproduced with: > > run-api-tests --root debug-240437 --debug > > > > I received no failure on 240436. > > I don't understand how this patch can have any impact on > TestWTF.WTF.StringOperators, but I will take a look... :( Evidently this is a notoriously fragile test file: https://bugs.webkit.org/show_bug.cgi?id=158623 https://bugs.webkit.org/show_bug.cgi?id=180211#c11 https://bugs.webkit.org/show_bug.cgi?id=192361
Ross Kirsling
Comment 39 2019-01-24 17:18:13 PST
Apparently TestWTF.WTF.StringOperators has a monopoly on String::operator+. What a nightmare. Committed r240459: <https://trac.webkit.org/changeset/240459>
Note You need to log in before you can comment on or make changes to this bug.