[Webkit-unassigned] [Bug 119785] [WebCore] Replace currentTime() with monotonicallyIncreasingTime() in all possible places - (Part 1)

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=119785


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #208798|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #8 from Alexey Proskuryakov <ap at webkit.org>  2013-08-15 09:54:47 PST ---
(From update of attachment 208798)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.