Bug 193602 - Move FileSystem to WTF
Summary: Move FileSystem to WTF
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-18 16:52 PST by Ross Kirsling
Modified: 2019-01-24 17:18 PST (History)
12 users (show)

See Also:


Attachments
Patch (414.25 KB, patch)
2019-01-18 16:55 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (414.28 KB, patch)
2019-01-18 17:04 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (424.95 KB, patch)
2019-01-18 22:45 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (425.97 KB, patch)
2019-01-21 19:07 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (426.38 KB, patch)
2019-01-21 22:07 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (427.03 KB, patch)
2019-01-22 18:03 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (427.10 KB, patch)
2019-01-22 18:17 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (426.50 KB, patch)
2019-01-22 19:41 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (426.70 KB, patch)
2019-01-23 02:09 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (426.84 KB, patch)
2019-01-23 11:34 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (426.80 KB, patch)
2019-01-23 14:40 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (426.75 KB, patch)
2019-01-23 19:12 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2019-01-18 16:52:01 PST
Move FileSystem to WTF
Comment 1 Ross Kirsling 2019-01-18 16:55:41 PST
Created attachment 359556 [details]
Patch
Comment 2 Ross Kirsling 2019-01-18 17:04:59 PST
Created attachment 359559 [details]
Patch
Comment 3 EWS Watchlist 2019-01-18 17:08:17 PST Comment hidden (obsolete)
Comment 4 Ross Kirsling 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.
Comment 5 Ross Kirsling 2019-01-18 22:45:10 PST
Created attachment 359589 [details]
Patch
Comment 6 EWS Watchlist 2019-01-18 22:50:13 PST Comment hidden (obsolete)
Comment 7 Ross Kirsling 2019-01-21 19:07:39 PST
Created attachment 359718 [details]
Patch
Comment 8 EWS Watchlist 2019-01-21 19:11:07 PST Comment hidden (obsolete)
Comment 9 Ross Kirsling 2019-01-21 22:07:32 PST
Created attachment 359719 [details]
Patch
Comment 10 EWS Watchlist 2019-01-21 22:10:34 PST Comment hidden (obsolete)
Comment 11 Ross Kirsling 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.
Comment 12 Ross Kirsling 2019-01-22 18:03:06 PST
Created attachment 359827 [details]
Patch
Comment 13 EWS Watchlist 2019-01-22 18:06:54 PST Comment hidden (obsolete)
Comment 14 Ross Kirsling 2019-01-22 18:17:28 PST
Created attachment 359830 [details]
Patch
Comment 15 Ross Kirsling 2019-01-22 19:41:30 PST
Created attachment 359839 [details]
Patch
Comment 16 EWS Watchlist 2019-01-22 19:45:37 PST Comment hidden (obsolete)
Comment 17 Fujii Hironori 2019-01-23 02:09:17 PST
Created attachment 359862 [details]
Patch

* Fixed GTK+ builds.
Comment 18 EWS Watchlist 2019-01-23 02:12:45 PST Comment hidden (obsolete)
Comment 19 Ross Kirsling 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.
Comment 20 Ross Kirsling 2019-01-23 11:34:52 PST
Created attachment 359913 [details]
Patch
Comment 21 EWS Watchlist 2019-01-23 11:38:32 PST Comment hidden (obsolete)
Comment 22 Ross Kirsling 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...
Comment 23 Alex Christensen 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.
Comment 24 Ross Kirsling 2019-01-23 14:40:33 PST
Created attachment 359945 [details]
Patch
Comment 25 EWS Watchlist 2019-01-23 14:44:05 PST Comment hidden (obsolete)
Comment 26 Yusuke Suzuki 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.
Comment 27 Ross Kirsling 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.
Comment 28 Ross Kirsling 2019-01-23 19:12:56 PST
Created attachment 359985 [details]
Patch
Comment 29 EWS Watchlist 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.
Comment 30 Yusuke Suzuki 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?
Comment 31 Ross Kirsling 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?
Comment 32 Ross Kirsling 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. :)
Comment 33 WebKit Commit Bot 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>
Comment 34 WebKit Commit Bot 2019-01-24 09:26:09 PST
All reviewed patches have been landed.  Closing bug.
Comment 35 Radar WebKit Bug Importer 2019-01-24 09:27:36 PST
<rdar://problem/47518054>
Comment 36 Truitt Savell 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.
Comment 37 Ross Kirsling 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... :(
Comment 38 Ross Kirsling 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
Comment 39 Ross Kirsling 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>