WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
198269
Implement MappedFileData for Windows
https://bugs.webkit.org/show_bug.cgi?id=198269
Summary
Implement MappedFileData for Windows
Fujii Hironori
Reported
2019-05-26 22:09:40 PDT
Implement MappedFileData for Windows. MappedFileData has been introduced in
Bug 144192
.
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Fujii Hironori
Comment 1
2019-05-26 22:10:00 PDT
Created
attachment 370666
[details]
WIP patch
Christopher Reid
Comment 2
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.
Darin Adler
Comment 3
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.
Christopher Reid
Comment 4
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.
Christopher Reid
Comment 5
2019-07-08 11:18:47 PDT
Created
attachment 373645
[details]
Patch for landing
EWS Watchlist
Comment 6
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.
Christopher Reid
Comment 7
2019-07-08 11:24:58 PDT
Created
attachment 373647
[details]
Patch for landing
EWS Watchlist
Comment 8
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.
Christopher Reid
Comment 9
2019-07-08 11:32:52 PDT
Created
attachment 373649
[details]
Patch for landing
Christopher Reid
Comment 10
2019-07-08 11:34:57 PDT
land-safely was picking up other commits in my branch, that should be sorted out now.
WebKit Commit Bot
Comment 11
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
>
WebKit Commit Bot
Comment 12
2019-07-08 12:09:32 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug