Bug 213825 – Allow the File object to be created with a replacement file

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

This work is towards bug 213347.

The goal is to enable the UIProcess to pass an original path and an optional replacement path to the WebProcess, e.g. a HIEF image and its transcoded PNG image. The Web process will create the File object with the replacement file but will register the Blob in the NetworkProcess with both: the original file and the replacement file. The Blob in the NetworkProcess will delete the replacement file when it is destroyed.

It is important to create the File object with the replacement file because it needs to get the size and the bytes of the replacement file not the original files.

[Said Abou-Hallawa]

Created attachment 403259 [details]
Patch

[Darin Adler]

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?

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 403314 [details]
Patch

[Said Abou-Hallawa]

Created attachment 403316 [details]
Patch

[Darin Adler]

Comment on attachment 403316 [details]
Patch

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

[Said Abou-Hallawa]

Created attachment 403322 [details]
Patch

[EWS]

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

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

[Radar WebKit Bug Importer]

<rdar://problem/65015820>