Bug 192287 – Use WallTime for file time

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

Use WallTime for file time

[Yusuke Suzuki]

Created attachment 356337 [details]
Patch

[Darin Adler]

Comment on attachment 356337 [details]
Patch

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

> Source/WebCore/Modules/webdatabase/Database.cpp:641
> +    return DatabaseDetails(stringIdentifier(), displayName(), estimatedSize(), 0, WallTime::fromRawSeconds(0), WallTime::fromRawSeconds(0));

Is the result of WallTime::fromRawSeconds(0) different from default-constructing a WallTime with "WallTime { }"? I seem to see a mix of things below where in some cases a 0 turns into WallTime::fromRawSeconds(0), but in other cases it turns into default initialization. I don’t understand the distinction between the two cases.

> Source/WebCore/page/Page.cpp:1384
> +    if (m_didLoadUserStyleSheet && modificationTime <= m_userStyleSheetModificationTime)

Does "<=" work here without explicitly calling "value()" on the std::optional?

> Source/WebCore/page/Page.h:814
> +    mutable WallTime m_userStyleSheetModificationTime;

Maybe this should use std::optional instead of the magic value of 0?

> Source/WebCore/platform/FileStream.cpp:59
> -        if (static_cast<time_t>(expectedModificationTime) != modificationTime)
> +        if (expectedModificationTime.secondsSinceEpoch().secondsAs<time_t>() != modificationTime->secondsSinceEpoch().secondsAs<time_t>())

No "!=" operator available on WallTime itself?

> Source/WebCore/platform/FileSystem.h:136
> +inline WallTime invalidFileTime() { return WallTime::nan(); }
> +inline bool isValidFileTime(WallTime time) { return std::isnan(time); }

Do we really need these? What are they used for? Why not use std::optional instead?

> Source/WebCore/platform/sql/SQLiteFileSystem.h:101
> +    WEBCORE_EXPORT static WallTime databaseCreationTime(const String& fileName);
> +    WEBCORE_EXPORT static WallTime databaseModificationTime(const String& fileName);

Change to return std::optional?

> Source/WebKit/WebProcess/MediaCache/WebMediaKeyStorageManager.cpp:111
> +    removeAllMediaKeyStorageForOriginPath(originPath, WallTime::fromRawSeconds(std::numeric_limits<double>::lowest()), WallTime::fromRawSeconds(std::numeric_limits<double>::max()));

Maybe WallTime itself should offer a lowest and max equivalent?

> Source/WebKit/WebProcess/MediaCache/WebMediaKeyStorageManager.cpp:131
> +        removeAllMediaKeyStorageForOriginPath(originPath, WallTime::fromRawSeconds(std::numeric_limits<double>::lowest()), WallTime::fromRawSeconds(std::numeric_limits<double>::max()));

Ditto.

[Yusuke Suzuki]

Comment on attachment 356337 [details]
Patch

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

Thank you for your review!

>> Source/WebCore/Modules/webdatabase/Database.cpp:641
>> +    return DatabaseDetails(stringIdentifier(), displayName(), estimatedSize(), 0, WallTime::fromRawSeconds(0), WallTime::fromRawSeconds(0));
> 
> Is the result of WallTime::fromRawSeconds(0) different from default-constructing a WallTime with "WallTime { }"? I seem to see a mix of things below where in some cases a 0 turns into WallTime::fromRawSeconds(0), but in other cases it turns into default initialization. I don’t understand the distinction between the two cases.

WallTime() and WallTime::fromRawSeconds(0) are the same. They return WallTime with 0.
In this case, WallTime(0) is used as a placeholder for unknown WallTime. I think `std::optional<>` is better. Fixed.

>> Source/WebCore/page/Page.cpp:1384
>> +    if (m_didLoadUserStyleSheet && modificationTime <= m_userStyleSheetModificationTime)
> 
> Does "<=" work here without explicitly calling "value()" on the std::optional?

Internally, operator<= checks nullopt and calls value() if it is not nullopt. But in this case, we should call `value()` in the caller side since this nullopt check is unnecessary. Fixed.

>> Source/WebCore/page/Page.h:814
>> +    mutable WallTime m_userStyleSheetModificationTime;
> 
> Maybe this should use std::optional instead of the magic value of 0?

Sounds good. Fixed.

>> Source/WebCore/platform/FileStream.cpp:59
>> +        if (expectedModificationTime.secondsSinceEpoch().secondsAs<time_t>() != modificationTime->secondsSinceEpoch().secondsAs<time_t>())
> 
> No "!=" operator available on WallTime itself?

Here, I keep the original semantics in the code, using `time_t` cast. This is because typical FileSystem can only save the time in time_t precision... seconds.

>> Source/WebCore/platform/FileSystem.h:136
>> +inline bool isValidFileTime(WallTime time) { return std::isnan(time); }
> 
> Do we really need these? What are they used for? Why not use std::optional instead?

Yeah, this is not necessary. Dropped.

>> Source/WebCore/platform/sql/SQLiteFileSystem.h:101
>> +    WEBCORE_EXPORT static WallTime databaseModificationTime(const String& fileName);
> 
> Change to return std::optional?

Sounds nice, fixed.

>> Source/WebKit/WebProcess/MediaCache/WebMediaKeyStorageManager.cpp:111
>> +    removeAllMediaKeyStorageForOriginPath(originPath, WallTime::fromRawSeconds(std::numeric_limits<double>::lowest()), WallTime::fromRawSeconds(std::numeric_limits<double>::max()));
> 
> Maybe WallTime itself should offer a lowest and max equivalent?

After looking into the code, I think we can replace them with `-WallTime::infinity()` and `WallTime::infinity()`. Fixed.

>> Source/WebKit/WebProcess/MediaCache/WebMediaKeyStorageManager.cpp:131
>> +        removeAllMediaKeyStorageForOriginPath(originPath, WallTime::fromRawSeconds(std::numeric_limits<double>::lowest()), WallTime::fromRawSeconds(std::numeric_limits<double>::max()));
> 
> Ditto.

Fixed.

[Yusuke Suzuki]

Created attachment 356368 [details]
Patch

[Yusuke Suzuki]

Created attachment 356370 [details]
Patch

[Yusuke Suzuki]

Created attachment 356376 [details]
Patch

[Darin Adler]

Comment on attachment 356376 [details]
Patch

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

> Source/WebCore/fileapi/File.cpp:116
> +    if (modificationTime && !std::isnan(modificationTime.value()))

Do we need to allow FileSystem::getFileModificationTime implementations to return NAN instead of std::nullopt? If so, what is the distinct meaning of the NAN?

[Yusuke Suzuki]

Comment on attachment 356376 [details]
Patch

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

>> Source/WebCore/fileapi/File.cpp:116
>> +    if (modificationTime && !std::isnan(modificationTime.value()))
> 
> Do we need to allow FileSystem::getFileModificationTime implementations to return NAN instead of std::nullopt? If so, what is the distinct meaning of the NAN?

Never happens. So we can remove this check :)

[Yusuke Suzuki]

Committed r238802: <https://trac.webkit.org/changeset/238802>

[Radar WebKit Bug Importer]

<rdar://problem/46421683>