Bug 176293 – Have getFileMetadata() return a std::optional<FileMetadata>

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Have getFileMetadata() returns a std::optional<FileMetadata> instead of using an out parameter for the metadata.

[Chris Dumez]

Created attachment 319761 [details]
Patch

[Chris Dumez]

Created attachment 319762 [details]
Patch

[Chris Dumez]

Created attachment 319787 [details]
Patch

[Darin Adler]

Comment on attachment 319762 [details]
Patch

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

Looks great. I have many small gripes about this and the surrounding code.

> Source/WebCore/ChangeLog:9
> +        Have getFileMetadata() returns a std::optional<FileMetadata> instead of
> +        using an out parameter for the metadata.

The word "get" roughly means "out parameter" in WebKit code since we don’t use it when the return value is the thing being "gotten". Because of this, the function should probably also be renamed to fileMetadata, readFileMetadata, or something with a word other than get. The word "get" goes along with the old interface.

Also on the subject of naming, I think we should have two different named functions rather than a single function with an enum argument telling it whether to follow symbolic links or not. There is only one call site in all of WebKit that passes something other than a constant to that argument, and that call site is *inside* the FileSystem.cpp file.

> Source/WebCore/Modules/entriesapi/DOMFileSystem.cpp:168
> +    if (!metadata)
>          return Exception { NotFoundError, ASCIILiteral("Path does not exist") };

Seems oversimpified to report all errors as "does not exist". The actual stat call in POSIX has many other error codes for failures other than EENOENT (does not exist).

This is why we might need std::expected rather than std::optional for this. Not sure what the intention of the DOMFileSystem specification is for such things.

> Source/WebCore/Modules/entriesapi/DOMFileSystem.cpp:300
> +        auto metadata = getFileMetadata(fullPath, ShouldFollowSymbolicLinks::No).value_or(FileMetadata { });

I think it would be better to not construct an entire FileMetadata for the failure case, just because we know it will give us a type other than file or directory. I think I would probably write something more like this:

auto type = metadata.has_value() ? metadata.value().type : FileMetadata::TypeUnknown;:

Although as I mention below I am not sure why FileMetadata::Type has a special value for this, so maybe it should be a std::optional in the future.

> Source/WebCore/platform/FileSystem.h:103
> +WEBCORE_EXPORT std::optional<FileMetadata> getFileMetadata(const String&, ShouldFollowSymbolicLinks);

I don’t think it’s obvious that the string argument is a filename/pathname from the function name and type of the argument alone. Maybe to others this is obvious from context, but not to me.

> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:258
> +    FileMetadata metadata;

My first thought is that I would use construction or aggregate initialization rather than defining the structure and then setting values of the fields. But I guess setting values of fields is better because you can see the field names, but otherwise it’s not my favorite idiom.

> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:270
> +    return metadata;

Since the return value is std::optional<FileMetadata> rather than just FileMetadata, the return-value optimization does not kick in. Thus when just reading the source code it looks like it would be slightly more efficient if this said WTFMove(metadata) if there were any non-scalar fields in the FileMetadata struct. But I see now that is not true at the moment; no strings or anything like that.

FileMetadata.h does have many, many tiny defects, though. It has not yet been converted to #pragma once. It includes WTFString.h but there is no use of String in the file. It has a Type enum where all the values have Type as a prefix; that should be enum class instead. It uses "-1" for "length not set" but instead should use std::optional for that. The modification time field has a comment says "0.0" means time is not set, is initialized to invalidFileTime (NaN and not 0.0); we should almost certainly use std::optional<WallTime> or std::optional<Seconds> rather than double. Also not too fond of "TypeUnknown". Do we really need it? Are we using it to indicate "other", or actually using it to indicate "unknown"? Does FileMetadata really need to be default-initializable to something distinct like this? Why is it OK for isHidden to be definitively "false" in a newly allocated FileMetadata, but for type we feed the need to use "unknown" rather than "file" (knowing it might be inaccurate).

The special values like -1 don’t seem to necessarily be fully supported; for example most call sites use the length without checking for -1.

> Tools/TestWebKitAPI/Tests/WebCore/FileSystem.cpp:132
> +    ASSERT_TRUE(!!symlinkMetadata);

I prefer has_value() to !! for this operation.

> Tools/TestWebKitAPI/Tests/WebCore/FileSystem.cpp:137
> +    ASSERT_TRUE(!!targetMetadata);

Ditto.

[Darin Adler]

Comment on attachment 319787 [details]
Patch

See my comments on the earlier version.

[Chris Dumez]

Comment on attachment 319762 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        using an out parameter for the metadata.
> 
> The word "get" roughly means "out parameter" in WebKit code since we don’t use it when the return value is the thing being "gotten". Because of this, the function should probably also be renamed to fileMetadata, readFileMetadata, or something with a word other than get. The word "get" goes along with the old interface.
> 
> Also on the subject of naming, I think we should have two different named functions rather than a single function with an enum argument telling it whether to follow symbolic links or not. There is only one call site in all of WebKit that passes something other than a constant to that argument, and that call site is *inside* the FileSystem.cpp file.

Ok.

>> Source/WebCore/Modules/entriesapi/DOMFileSystem.cpp:168
>>          return Exception { NotFoundError, ASCIILiteral("Path does not exist") };
> 
> Seems oversimpified to report all errors as "does not exist". The actual stat call in POSIX has many other error codes for failures other than EENOENT (does not exist).
> 
> This is why we might need std::expected rather than std::optional for this. Not sure what the intention of the DOMFileSystem specification is for such things.

The spec says we can throw either NotFoundError or TypeMismatchError. The most likely reason for getFileMetata to fails is that the file does not exist, which is why I chose NotFoundError. Other reasons may be:
 - Bad input (unlikely here)
- Permission problems, in which case I think reporting NotFoundError to the Web is still the right behavior.
- For other errors types, I still think NotFoundError is the right behavior here given our 2 choices.

>> Source/WebCore/Modules/entriesapi/DOMFileSystem.cpp:300
>> +        auto metadata = getFileMetadata(fullPath, ShouldFollowSymbolicLinks::No).value_or(FileMetadata { });
> 
> I think it would be better to not construct an entire FileMetadata for the failure case, just because we know it will give us a type other than file or directory. I think I would probably write something more like this:
> 
> auto type = metadata.has_value() ? metadata.value().type : FileMetadata::TypeUnknown;:
> 
> Although as I mention below I am not sure why FileMetadata::Type has a special value for this, so maybe it should be a std::optional in the future.

Ok

>> Source/WebCore/platform/FileSystem.h:103
>> +WEBCORE_EXPORT std::optional<FileMetadata> getFileMetadata(const String&, ShouldFollowSymbolicLinks);
> 
> I don’t think it’s obvious that the string argument is a filename/pathname from the function name and type of the argument alone. Maybe to others this is obvious from context, but not to me.

Ok.

>> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:270
>> +    return metadata;
> 
> Since the return value is std::optional<FileMetadata> rather than just FileMetadata, the return-value optimization does not kick in. Thus when just reading the source code it looks like it would be slightly more efficient if this said WTFMove(metadata) if there were any non-scalar fields in the FileMetadata struct. But I see now that is not true at the moment; no strings or anything like that.
> 
> FileMetadata.h does have many, many tiny defects, though. It has not yet been converted to #pragma once. It includes WTFString.h but there is no use of String in the file. It has a Type enum where all the values have Type as a prefix; that should be enum class instead. It uses "-1" for "length not set" but instead should use std::optional for that. The modification time field has a comment says "0.0" means time is not set, is initialized to invalidFileTime (NaN and not 0.0); we should almost certainly use std::optional<WallTime> or std::optional<Seconds> rather than double. Also not too fond of "TypeUnknown". Do we really need it? Are we using it to indicate "other", or actually using it to indicate "unknown"? Does FileMetadata really need to be default-initializable to something distinct like this? Why is it OK for isHidden to be definitively "false" in a newly allocated FileMetadata, but for type we feed the need to use "unknown" rather than "file" (knowing it might be inaccurate).
> 
> The special values like -1 don’t seem to necessarily be fully supported; for example most call sites use the length without checking for -1.

Ok.

>> Tools/TestWebKitAPI/Tests/WebCore/FileSystem.cpp:132
>> +    ASSERT_TRUE(!!symlinkMetadata);
> 
> I prefer has_value() to !! for this operation.

Ok.

>> Tools/TestWebKitAPI/Tests/WebCore/FileSystem.cpp:137
>> +    ASSERT_TRUE(!!targetMetadata);
> 
> Ditto.

Ok.

[Chris Dumez]

Created attachment 319798 [details]
Patch

[Chris Dumez]

Requesting review again given the amount of changes.

[WebKit Commit Bot]

Comment on attachment 319798 [details]
Patch

Clearing flags on attachment: 319798

Committed r221558: <http://trac.webkit.org/changeset/221558>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34693762>