[webkit-reviews] review denied: [Bug 119785] [WebCore] Replace currentTime() with monotonicallyIncreasingTime() in all possible places - (Part 1) : [Attachment 208798] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Arunprasad Rajkumar
<arurajku at cisco.com>'s request for review:
Bug 119785: [WebCore] Replace currentTime() with monotonicallyIncreasingTime()
in all possible places - (Part 1)
https://bugs.webkit.org/show_bug.cgi?id=119785

Attachment 208798: Patch
https://bugs.webkit.org/attachment.cgi?id=208798&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=208798&action=review


These changes all look good to me, thank you for tackling this!

Media timing is something I don’t deeply understand, so I’d like Eric or Jer to
sign off too.

> Source/WebCore/ChangeLog:3
> +	   <https://webkit.org/b/119785> [WebCore] Replace currentTime() with
monotonicallyIncreasingTime() in all possible places - (Part 1)

We use bracketed prefixes to denote bugs that are of limited interest, so that
people who are not interested in a specific platoform could ignore these with a
quick glance. Please don’t use prefixes on bugs that touch general
cross-platform code.

> Source/WebCore/ChangeLog:7
> +	   No new tests needed, behaviour is not changed.

This is not accurate, the intention of this patch is to change behavior. There
is no reasonable way to test the changes in regression tests, and this is what
you should say here. Or just omit the explanation.

> Source/WTF/wtf/CurrentTime.h:55
> +// It is highly recommended to use this instead of currenTime() for elapsed
time
> +// measurement.

Typo: "currenTime".

It’s not enough to say "highly recommended", this doesn’t explain anything to
the reader, and someone who uses the wrong function won’t see this comment
anyway. Perhaps you could say something like "result of this function increases
monotonically even when clock time goes back due to DST changes or NTP
adjustments".

> Source/WTF/wtf/CurrentTime.h:58
> +// Monotonic time in milliseconds.

This comment is not helpful, please just remove it.

> Source/WebCore/html/HTMLMediaElement.cpp:1563
> -    m_previousProgressTime = WTF::currentTime();
> +    m_previousProgressTime = WTF::monotonicallyIncreasingTime();

Please remove the WTF:: prefix. We sould never use it, except perhaps in some
platform code to disambiguate with names from OS headers.

> Source/WebCore/html/HTMLMediaElement.cpp:2074
> -    double time = WTF::currentTime();
> +    double time = WTF::monotonicallyIncreasingTime();

Ditto.

> Source/WebCore/html/HTMLMediaElement.cpp:2264
> -    m_cachedTimeWallClockUpdateTime = WTF::currentTime();
> +    m_cachedTimeClockUpdateTime = WTF::monotonicallyIncreasingTime();

Ditto.

> Source/WebCore/html/HTMLMediaElement.cpp:2276
> -    m_minimumWallClockTimeToCacheMediaTime = WTF::currentTime() +
minimumTimePlayingBeforeCacheSnapshot;
> +    m_minimumClockTimeToCacheMediaTime = WTF::monotonicallyIncreasingTime()
+ minimumTimePlayingBeforeCacheSnapshot;

Ditto.

> Source/WebCore/html/HTMLMediaElement.cpp:2305
> -    double now = WTF::currentTime();
> +    double now = WTF::monotonicallyIncreasingTime();

Ditto.

> Source/WebCore/html/HTMLMediaElement.cpp:2799
> -    m_previousProgressTime = WTF::currentTime();
> +    m_previousProgressTime = WTF::monotonicallyIncreasingTime();

Ditto.

> Source/WebCore/html/HTMLMediaElement.cpp:2831
> -    double now = WTF::currentTime();
> +    double now = WTF::monotonicallyIncreasingTime();

Ditto.

> Source/WebCore/html/HTMLMediaElement.h:677
> +    mutable double m_cachedTimeClockUpdateTime; // Value is based on
WTF::monotonicallyIncreasingTime.
> +    mutable double m_minimumClockTimeToCacheMediaTime; // Value is based on
WTF::monotonicallyIncreasingTime.

I don’t think that these comments are helpful.

> Source/WebCore/html/MediaController.cpp:676
> -    double now = WTF::currentTime();
> +    double now = WTF::monotonicallyIncreasingTime();

Please remove the WTF:: prefix.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:184
> -    m_currentMatchData.startTime = WTF::currentTimeMS();
> +    m_currentMatchData.startTime = WTF::monotonicallyIncreasingTimeMS();

Ditto.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:189
> -    double matchTimeMs = WTF::currentTimeMS() -
m_currentMatchData.startTime;
> +    double matchTimeMs = WTF::monotonicallyIncreasingTimeMS() -
m_currentMatchData.startTime;

Ditto. Also, please change Ms to MS while touching this code.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:203
> -    double processingTimeMs = WTF::currentTimeMS() -
m_currentMatchData.startTime;
> +    double processingTimeMs = WTF::monotonicallyIncreasingTimeMS() -
m_currentMatchData.startTime;

Ditto.

> Source/WebCore/platform/ClockGeneric.cpp:82
> -    return WTF::currentTime();
> +    return WTF::monotonicallyIncreasingTime();

Please remove the WTF:: prefix.