Bug 213825

Created attachment 403259 [details]
Patch

Comment on attachment 403259 [details]
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?

Comment on attachment 403259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403259&action=review

>> 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?

The typo is fixed. You are right this patch is work towards webkit.org/b/213347. I added this to the ChangeLog.

>> Source/WebCore/ChangeLog:14
>> +        t is important to create the File object with the replacement file because
> 
> typo: "t"

Fixed.

>> Source/WebCore/fileapi/File.cpp:59
>> +    return adoptRef(*new File(WTFMove(internalURL), WTFMove(type), String { effectivePath }, WTFMove(name)));
> 
> Why not WTFMove(effectivePath)?

Fixed.

>> Source/WebCore/fileapi/File.h:45
>> +    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.

Yes I did that by removing the default values for both replacementPath and nameOverride from the create() method. Then I fixed all compilation errors. And finally I put back the default values and removed the unnecessary initializations in the caller side. This way I made sure nameOverride was never passed as the second argument.

>> Source/WebCore/platform/FileChooser.h:54
>>      }
> 
> Why do we need a constructor? Can’t we delete it and just use aggregate initialization instead?

Fixed. But I had to pass three values always in the aggregate initialization because I was getting this error in the caller side:

error: missing field 'replacementPath' initializer [-Werror,-Wmissing-field-initializers]

I am not sure whether -Wmissing-field-initializers is enabled by default or this is WebKit Xcode project settings.

Created attachment 403314 [details]
Patch

Created attachment 403316 [details]
Patch

Comment on attachment 403316 [details]
Patch

Many of the places we have nullString() here we could alternatively have { }.

Created attachment 403322 [details]
Patch

Committed r263830: <https://trac.webkit.org/changeset/263830>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403322 [details].

<rdar://problem/65015820>