WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
119785
Replace currentTime() with monotonicallyIncreasingTime() in WebCore
https://bugs.webkit.org/show_bug.cgi?id=119785
Summary
Replace currentTime() with monotonicallyIncreasingTime() in WebCore
Arunprasad Rajkumar
Reported
2013-08-14 00:20:32 PDT
Replace currentTime() with monotonicallyIncreasingTime() in WebCore [at appropriate places]
Attachments
Patch
(23.17 KB, patch)
2013-08-14 23:26 PDT
,
Arunprasad Rajkumar
no flags
Details
Formatted Diff
Diff
Patch
(23.16 KB, patch)
2013-08-15 02:29 PDT
,
Arunprasad Rajkumar
no flags
Details
Formatted Diff
Diff
Patch
(29.26 KB, patch)
2013-08-15 13:50 PDT
,
Arunprasad Rajkumar
no flags
Details
Formatted Diff
Diff
Patch
(31.23 KB, patch)
2013-08-16 11:43 PDT
,
Arunprasad Rajkumar
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Arunprasad Rajkumar
Comment 1
2013-08-14 13:20:36 PDT
(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
Comment 2
2013-08-14 23:26:50 PDT
Created
attachment 208792
[details]
Patch
Benjamin Poulain
Comment 3
2013-08-15 00:15:38 PDT
You should not put those "TIPS". What would be useful instead is actual explanations.
Arunprasad Rajkumar
Comment 4
2013-08-15 02:29:55 PDT
Created
attachment 208798
[details]
Patch
Jer Noble
Comment 5
2013-08-15 08:33:54 PDT
(In reply to
comment #0
)
> Replace currentTime() with monotonicallyIncreasingTime() in WebCore [at appropriate places]
Why?
Arunprasad Rajkumar
Comment 6
2013-08-15 08:45:37 PDT
(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 7
2013-08-15 09:19:41 PDT
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 8
2013-08-15 09:54:47 PDT
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 9
2013-08-15 10:33:54 PDT
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
Comment 10
2013-08-15 13:50:18 PDT
Created
attachment 208849
[details]
Patch
Anders Carlsson
Comment 11
2013-08-16 08:27:22 PDT
Looks like
https://bugs.webkit.org/show_bug.cgi?id=114696
is another bug where currentTime is incorrectly used.
Arunprasad Rajkumar
Comment 12
2013-08-16 08:38:58 PDT
(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
Comment 13
2013-08-16 10:53:59 PDT
(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 14
2013-08-16 10:58:02 PDT
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 15
2013-08-16 11:07:43 PDT
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 16
2013-08-16 11:42:36 PDT
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
Comment 17
2013-08-16 11:43:51 PDT
Created
attachment 208938
[details]
Patch
Arunprasad Rajkumar
Comment 18
2013-08-16 11:49:23 PDT
(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 19
2013-08-16 12:32:52 PDT
Comment on
attachment 208938
[details]
Patch Clearing flags on attachment: 208938 Committed
r154201
: <
http://trac.webkit.org/changeset/154201
>
WebKit Commit Bot
Comment 20
2013-08-16 12:32:56 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug