Bug 198269 - Implement MappedFileData for Windows
Summary: Implement MappedFileData for Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-05-26 22:09 PDT by Fujii Hironori
Modified: 2019-07-08 12:10 PDT (History)
5 users (show)

See Also:


Attachments
WIP patch (5.25 KB, patch)
2019-05-26 22:10 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (4.67 KB, patch)
2019-07-03 15:15 PDT, Christopher Reid
darin: review+
Details | Formatted Diff | Diff
Patch for landing (12.35 KB, patch)
2019-07-08 11:18 PDT, Christopher Reid
chris.reid: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (12.35 KB, patch)
2019-07-08 11:24 PDT, Christopher Reid
chris.reid: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (4.36 KB, patch)
2019-07-08 11:32 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2019-05-26 22:09:40 PDT
Implement MappedFileData for Windows.
MappedFileData has been introduced in Bug 144192.
Comment 1 Fujii Hironori 2019-05-26 22:10:00 PDT
Created attachment 370666 [details]
WIP patch
Comment 2 Christopher Reid 2019-07-03 15:15:05 PDT
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 3 Darin Adler 2019-07-03 15:20:36 PDT
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.
Comment 4 Christopher Reid 2019-07-08 11:11:24 PDT
(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.
Comment 5 Christopher Reid 2019-07-08 11:18:47 PDT
Created attachment 373645 [details]
Patch for landing
Comment 6 Build Bot 2019-07-08 11:20:42 PDT
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.
Comment 7 Christopher Reid 2019-07-08 11:24:58 PDT
Created attachment 373647 [details]
Patch for landing
Comment 8 Build Bot 2019-07-08 11:27:53 PDT
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.
Comment 9 Christopher Reid 2019-07-08 11:32:52 PDT
Created attachment 373649 [details]
Patch for landing
Comment 10 Christopher Reid 2019-07-08 11:34:57 PDT
land-safely was picking up other commits in my branch, that should be sorted out now.
Comment 11 WebKit Commit Bot 2019-07-08 12:09:30 PDT
Comment on attachment 373649 [details]
Patch for landing

Clearing flags on attachment: 373649

Committed r247225: <https://trac.webkit.org/changeset/247225>
Comment 12 WebKit Commit Bot 2019-07-08 12:09:32 PDT
All reviewed patches have been landed.  Closing bug.