Bug 119785 – Replace currentTime() with monotonicallyIncreasingTime() in WebCore

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

[Arunprasad Rajkumar]

Replace currentTime() with monotonicallyIncreasingTime() in WebCore [at appropriate places]

[Arunprasad Rajkumar]

(In reply to comment #0)
> Replace currentTime() with monotonicallyIncreasingTime() in WebCore [at appropriate places]

Changes includes,

1. Document
2. Element
3. Core Media Element implementation(not includes platform specific MediaPlayerPrivate*)
4. FrameView
5. html parser
6. platform/clock generic

[Arunprasad Rajkumar]

Created attachment 208792 [details]
Patch

[Benjamin Poulain]

You should not put those "TIPS". What would be useful instead is actual explanations.

[Arunprasad Rajkumar]

Created attachment 208798 [details]
Patch

[Jer Noble]

(In reply to comment #0)
> Replace currentTime() with monotonicallyIncreasingTime() in WebCore [at appropriate places]

Why?

[Arunprasad Rajkumar]

(In reply to comment #5)
> (In reply to comment #0)
> > Replace currentTime() with monotonicallyIncreasingTime() in WebCore [at appropriate places]
> 
> Why?

Hi, please refer the below bug comments,

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

[Eric Carlson]

Comment on attachment 208798 [details]
Patch

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

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

A note about why this change is being made would be helpful.

[Alexey Proskuryakov]

Comment on attachment 208798 [details]
Patch

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.

[Arunprasad Rajkumar]

Comment on attachment 208798 [details]
Patch

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

Thanks for your comments, I will submit the new patch soon :)

>> Source/WTF/wtf/CurrentTime.h:55
>> +// 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".

Thanks Alexey, I will correct this.

[Arunprasad Rajkumar]

Created attachment 208849 [details]
Patch

[Anders Carlsson]

Looks like https://bugs.webkit.org/show_bug.cgi?id=114696 is another bug where currentTime is incorrectly used.

[Arunprasad Rajkumar]

(In reply to comment #11)
> Looks like https://bugs.webkit.org/show_bug.cgi?id=114696 is another bug where currentTime is incorrectly used.

Oops, I'm unaware of that. But it seems to be in active for months.

(FYI, The current patch is not a complete one, still lot of places to be replaced in WebCore, I splitted patches to ease the review and avoid fatal regressions)

[Eric Carlson]

(In reply to comment #8)
> (From update of attachment 208798 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208798&action=review
> 
> Media timing is something I don’t deeply understand, so I’d like Eric or Jer to sign off too.
> 
The changes to the media element look fine to me.

[Jer Noble]

Comment on attachment 208849 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:283
> -    , m_cachedTimeWallClockUpdateTime(0)
> -    , m_minimumWallClockTimeToCacheMediaTime(0)
> +    , m_cachedTimeClockUpdateTime(0)
> +    , m_minimumClockTimeToCacheMediaTime(0)

These names are now confusing with the removal of "Wall".  E.g., what is a "TimeClock"?  (Arguably, they were confusing before, but they are definitely much more confusing now.)

I suggest "m_clockTimeAtLastCachedTimeUpdate" and "m_minimumClockTimeToUpdateCachedTime"

> Source/WebCore/html/HTMLMediaElement.cpp:2326
> -    if (maximumDurationToCacheMediaTime && now > m_minimumWallClockTimeToCacheMediaTime && m_cachedTime != MediaPlayer::invalidTime()) {
> -        double wallClockDelta = now - m_cachedTimeWallClockUpdateTime;
> +    if (maximumDurationToCacheMediaTime && now > m_minimumClockTimeToCacheMediaTime && m_cachedTime != MediaPlayer::invalidTime()) {
> +        double wallClockDelta = now - m_cachedTimeClockUpdateTime;

You removed "wall" from everywhere else, why not here?

Apart from the above, the changes to HTMLMediaElement and MediaController LGTM.

[Arunprasad Rajkumar]

Comment on attachment 208798 [details]
Patch

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

Hi Alexey, Corrected and uploaded a patch.

>> Source/WTF/ChangeLog:3
>> +        <https://webkit.org/b/119785> [WebCore] Replace currentTime() with monotonicallyIncreasingTime() in all possible places - (Part 1)
> 
> A note about why this change is being made would be helpful.

Eric, added a note by using hints provided by Alexey

>> 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.

Thanks for changing the title.(I will consider this point while raising bugs)

>> 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.

Removed

>> Source/WTF/wtf/CurrentTime.h:58
>> +// Monotonic time in milliseconds.
> 
> This comment is not helpful, please just remove it.

Done

>> Source/WebCore/html/HTMLMediaElement.cpp:1563
>> +    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.

Done

>> Source/WebCore/html/HTMLMediaElement.h:677
>> +    mutable double m_minimumClockTimeToCacheMediaTime; // Value is based on WTF::monotonicallyIncreasingTime.
> 
> I don’t think that these comments are helpful.

Removed

>> Source/WebCore/html/MediaController.cpp:676
>> +    double now = WTF::monotonicallyIncreasingTime();
> 
> Please remove the WTF:: prefix.

Done

>> Source/WebCore/inspector/InspectorCSSAgent.cpp:189
>> +    double matchTimeMs = WTF::monotonicallyIncreasingTimeMS() - m_currentMatchData.startTime;
> 
> Ditto. Also, please change Ms to MS while touching this code.

Done

>> Source/WebCore/platform/ClockGeneric.cpp:82
>> +    return WTF::monotonicallyIncreasingTime();
> 
> Please remove the WTF:: prefix.

Done

[Arunprasad Rajkumar]

Comment on attachment 208849 [details]
Patch

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

Modfied as per Jer's comments.

>> Source/WebCore/html/HTMLMediaElement.cpp:283
>> +    , m_minimumClockTimeToCacheMediaTime(0)
> 
> These names are now confusing with the removal of "Wall".  E.g., what is a "TimeClock"?  (Arguably, they were confusing before, but they are definitely much more confusing now.)
> 
> I suggest "m_clockTimeAtLastCachedTimeUpdate" and "m_minimumClockTimeToUpdateCachedTime"

LGTM as well. Corrected.

>> Source/WebCore/html/HTMLMediaElement.cpp:2326
>> +        double wallClockDelta = now - m_cachedTimeClockUpdateTime;
> 
> You removed "wall" from everywhere else, why not here?
> 
> Apart from the above, the changes to HTMLMediaElement and MediaController LGTM.

Done :)

[Arunprasad Rajkumar]

Created attachment 208938 [details]
Patch

[Arunprasad Rajkumar]

(In reply to comment #17)
> Created an attachment (id=208938) [details]
> Patch

Hi Jer & Alexey,

Hope my new patch attended your review comments, Please take a look.

Thanks,

[WebKit Commit Bot]

Comment on attachment 208938 [details]
Patch

Clearing flags on attachment: 208938

Committed r154201: <http://trac.webkit.org/changeset/154201>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.