[webkit-reviews] review granted: [Bug 213825] Allow the File object to be created with a replacement file : [Attachment 403259] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jul 1 10:04:11 PDT 2020
Darin Adler <darin at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 213825: Allow the File object to be created with a replacement file
https://bugs.webkit.org/show_bug.cgi?id=213825
Attachment 403259: Patch
https://bugs.webkit.org/attachment.cgi?id=403259&action=review
--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 403259
--> https://bugs.webkit.org/attachment.cgi?id=403259
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=403259&action=review
"objet" -> "object"
> Source/WebCore/ChangeLog:3
> + Allow the File objet to be created with a replacement file
type: "objet"
Am I correct that this adds unused code, to be used later? Is that why there is
no test coverage?
> Source/WebCore/ChangeLog:14
> + t is important to create the File object with the replacement file
because
typo: "t"
> Source/WebCore/fileapi/File.cpp:59
> + return adoptRef(*new File(WTFMove(internalURL), WTFMove(type), String {
effectivePath }, WTFMove(name)));
Why not WTFMove(effectivePath)?
> Source/WebCore/fileapi/File.h:45
> - WEBCORE_EXPORT static Ref<File> create(const String& path, const String&
nameOverride = { });
> + WEBCORE_EXPORT static Ref<File> create(const String& path, const String&
replacementPath = { }, const String& nameOverride = { });
With this change, we have to inspect every File::create call site, because
anyone specifying a nameOverride might now be specifying a replacementPath
instead by accident. I presume you did that. This is the kind of refactoring
that is risky since it silently changes the behavior of existing code.
> Source/WebCore/platform/FileChooser.h:54
> - FileChooserFileInfo(const String& path, const String& displayName =
String())
> + FileChooserFileInfo(const String& path, const String& replacementPath =
{ }, const String& displayName = { })
> : path(path)
> + , replacementPath(replacementPath)
> , displayName(displayName)
> {
> }
Why do we need a constructor? Can’t we delete it and just use aggregate
initialization instead?
More information about the webkit-reviews
mailing list