Bug 198269 – Implement MappedFileData for Windows

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Fujii Hironori]

Implement MappedFileData for Windows.
MappedFileData has been introduced in Bug 144192.

[Fujii Hironori]

Created attachment 370666 [details]
WIP patch

[Christopher Reid]

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.

[Darin Adler]

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.

[Christopher Reid]

(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.

[Christopher Reid]

Created attachment 373645 [details]
Patch for landing

[EWS Watchlist]

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.

[Christopher Reid]

Created attachment 373647 [details]
Patch for landing

[EWS Watchlist]

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.

[Christopher Reid]

Created attachment 373649 [details]
Patch for landing

[Christopher Reid]

land-safely was picking up other commits in my branch, that should be sorted out now.

[WebKit Commit Bot]

Comment on attachment 373649 [details]
Patch for landing

Clearing flags on attachment: 373649

Committed r247225: <https://trac.webkit.org/changeset/247225>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.