Replace currentTime() with monotonicallyIncreasingTime() in WebCore [at appropriate places]
(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
Created attachment 208792 [details] Patch
You should not put those "TIPS". What would be useful instead is actual explanations.
Created attachment 208798 [details] Patch
(In reply to comment #0) > Replace currentTime() with monotonicallyIncreasingTime() in WebCore [at appropriate places] Why?
(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
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.
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.
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.
Created attachment 208849 [details] Patch
Looks like https://bugs.webkit.org/show_bug.cgi?id=114696 is another bug where currentTime is incorrectly used.
(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)
(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.
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.
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
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 :)
Created attachment 208938 [details] Patch
(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,
Comment on attachment 208938 [details] Patch Clearing flags on attachment: 208938 Committed r154201: <http://trac.webkit.org/changeset/154201>
All reviewed patches have been landed. Closing bug.