Implement MappedFileData for Windows. MappedFileData has been introduced in Bug 144192.
Created attachment 370666 [details] WIP patch
Created attachment 373420 [details] Patch It looks like the only FileSystem Mapping test that was failing was MappingExistingEmptyFile but it passes after adding the `if (!size) {` check from the mmap implementation. I also moved the mmap implementation back to FileSystem.cpp from FileSystemPOSIX. It would be nice to split it up to posix and glib implementations. The only FileSystem tests failing on windows now are FileSystemTest.UnicodeDirectoryName and FileSystemTest.GetFileMetadataSymlink.
Comment on attachment 373420 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373420&action=review > Source/WTF/wtf/FileSystem.cpp:277 > +#if HAVE(MMAP) Should have a blank line after this. > Source/WTF/wtf/FileSystem.cpp:326 > +#endif Should have a blank line before this. > Source/WTF/wtf/FileSystem.h:36 > +#include <wtf/Noncopyable.h> See comment below. > Source/WTF/wtf/FileSystem.h:196 > + WTF_MAKE_NONCOPYABLE(MappedFileData); Given my understanding of C++, this should not be necessary. Defining a move constructor and a move assignment operator as this class does should implicitly cause the copy constructor and copy assignment operator to not be generated. So there should be no additional need to use WTF_MAKE_NOCOPYABLE. Was there some concrete problem that this fixed on Windows? Maybe my understanding of C++ is wrong, or maybe this works around a bug in the Windows C++ compiler. I’d prefer to not make the change unless it’s required.
(In reply to Darin Adler from comment #3) > Comment on attachment 373420 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=373420&action=review > > > Source/WTF/wtf/FileSystem.cpp:277 > > +#if HAVE(MMAP) > > Should have a blank line after this. > Will add. > > Source/WTF/wtf/FileSystem.cpp:326 > > +#endif > > Should have a blank line before this. > Will add. > > Source/WTF/wtf/FileSystem.h:36 > > +#include <wtf/Noncopyable.h> > > See comment below. > > > Source/WTF/wtf/FileSystem.h:196 > > + WTF_MAKE_NONCOPYABLE(MappedFileData); > > Given my understanding of C++, this should not be necessary. Defining a move > constructor and a move assignment operator as this class does should > implicitly cause the copy constructor and copy assignment operator to not be > generated. So there should be no additional need to use WTF_MAKE_NOCOPYABLE. > > Was there some concrete problem that this fixed on Windows? Maybe my > understanding of C++ is wrong, or maybe this works around a bug in the > Windows C++ compiler. > > I’d prefer to not make the change unless it’s required. There doesn't seem to be any compiler issues without the WTF_MAKE_NOCOPYABLE in Windows. I'll take those changes out.
Created attachment 373645 [details] Patch for landing
Attachment 373645 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jsc.cpp:1015: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/jsc.cpp:1083: Missing space inside { }. [whitespace/braces] [5] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 373647 [details] Patch for landing
Attachment 373647 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jsc.cpp:1015: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/jsc.cpp:1083: Missing space inside { }. [whitespace/braces] [5] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 373649 [details] Patch for landing
land-safely was picking up other commits in my branch, that should be sorted out now.
Comment on attachment 373649 [details] Patch for landing Clearing flags on attachment: 373649 Committed r247225: <https://trac.webkit.org/changeset/247225>
All reviewed patches have been landed. Closing bug.