[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