Drop monotonicallyIncreasingTime from our tree. This is very nice since now we only have MonotonicTime::now() for this time. This effectively avoids the use of `double` for our time, seconds etc.
Created attachment 334123 [details] Patch
Created attachment 334507 [details] Patch
Created attachment 334508 [details] Patch
Created attachment 334509 [details] Patch
Created attachment 334510 [details] Patch
Great. During this refactoring, I found several bugs mixing 1. MediaTime and MonotonicTime 2. Seconds and MonotonicTime.
Created attachment 334520 [details] Patch
Comment on attachment 334520 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334520&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperAnimation.cpp:321 > + animation.pause(time - animation.startTime()); Still considering the way to fix it.
Created attachment 334525 [details] Patch
Created attachment 334526 [details] Patch
Created attachment 334530 [details] Patch
Comment on attachment 334530 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334530&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperAnimation.cpp:252 > - m_pauseTime = 0; > + m_pauseTime = 0_s; > m_totalRunningTime = m_pauseTime; > - m_lastRefreshedTime = monotonicallyIncreasingTime(); > + m_lastRefreshedTime = MonotonicTime::now(); This also seems wrong to me. We clear m_totalRunningTime by setting `m_totalRunningTime = m_pauseTime`, where `m_pauseTime` is `0_s`. Is it correct? > Source/WebCore/platform/graphics/texmap/TextureMapperAnimation.cpp:322 > +void TextureMapperAnimations::suspend(MonotonicTime time) > { > for (auto& animation : m_animations) > - animation.pause(offset); > + animation.pause(time - animation.startTime()); > } I think this was wrong. The given double is MonotonicTime, not Seconds (timeOffset). But the old code uses it as Seconds. I think the current quick change in this patch is also wrong (maybe, offset should be `time - animation.lastRefleshedTime()`?). @KaL, @mcatanzaro, could you take a look this thing?
Comment on attachment 334530 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334530&action=review >> Source/WebCore/platform/graphics/texmap/TextureMapperAnimation.cpp:252 >> + m_lastRefreshedTime = MonotonicTime::now(); > > This also seems wrong to me. We clear m_totalRunningTime by setting `m_totalRunningTime = m_pauseTime`, where `m_pauseTime` is `0_s`. Is it correct? That makes no sense; the existing code is obviously not right. (CC: Zan and Miguel) >> Source/WebCore/platform/graphics/texmap/TextureMapperAnimation.cpp:322 >> } > > I think this was wrong. The given double is MonotonicTime, not Seconds (timeOffset). > But the old code uses it as Seconds. > > I think the current quick change in this patch is also wrong (maybe, offset should be `time - animation.lastRefleshedTime()`?). > @KaL, @mcatanzaro, could you take a look this thing? I don't know what it's supposed to be.
Created attachment 334576 [details] Patch
Comment on attachment 334530 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334530&action=review >>> Source/WebCore/platform/graphics/texmap/TextureMapperAnimation.cpp:252 >>> + m_lastRefreshedTime = MonotonicTime::now(); >> >> This also seems wrong to me. We clear m_totalRunningTime by setting `m_totalRunningTime = m_pauseTime`, where `m_pauseTime` is `0_s`. Is it correct? > > That makes no sense; the existing code is obviously not right. (CC: Zan and Miguel) OK, in this patch, I keep this logic as is. (with FIXME). >>> Source/WebCore/platform/graphics/texmap/TextureMapperAnimation.cpp:322 >>> } >> >> I think this was wrong. The given double is MonotonicTime, not Seconds (timeOffset). >> But the old code uses it as Seconds. >> >> I think the current quick change in this patch is also wrong (maybe, offset should be `time - animation.lastRefleshedTime()`?). >> @KaL, @mcatanzaro, could you take a look this thing? > > I don't know what it's supposed to be. Ditto. In this patch, I keep this logic as is. (with FIXME).
Comment on attachment 334576 [details] Patch Attachment 334576 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/6661658 New failing tests: http/tests/preload/download_resources.html
Created attachment 334578 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Would be super if you could report a bug to fix TextureMapperAnimation.
Created attachment 334587 [details] Patch
Created attachment 334588 [details] Patch
(In reply to Michael Catanzaro from comment #18) > Would be super if you could report a bug to fix TextureMapperAnimation. OK, filed.
Filed as bugs #183112 and #183113. Probably better to wait for this to land.
Comment on attachment 334588 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334588&action=review I've skimmed over the entire diff and I don't see any obvious mistakes. Looks like you've been pretty careful, and the code is clearly now safer than it was before, so r=me. But you do need to get this acked by a WK2 owner before landing. > Source/WTF/wtf/MonotonicTime.h:49 > + // Call this if you know for sure that the double represents time according to the monotonic time > + // in second. It must be in seconds and it must be from the same time source. "Call this if you know for sure that the double represents monotonic time according to the same time source as MonotonicTime. It must be in seconds." > Source/WTF/wtf/cocoa/MemoryPressureHandlerCocoa.mm:162 > + // FIXME: The final argument `s_minimumHoldOffTime.seconds()` seems wrong. > + dispatch_source_set_timer(_timer_event_source, dispatch_time(DISPATCH_TIME_NOW, seconds.seconds() * NSEC_PER_SEC), DISPATCH_TIME_FOREVER, s_minimumHoldOffTime.seconds()); This needs a bug report, too. But I'm not actually sure that it's wrong... the comment up above indicates that it's supposed to be seconds.
Comment on attachment 334588 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334588&action=review Thanks for the review. >> Source/WTF/wtf/MonotonicTime.h:49 >> + // in second. It must be in seconds and it must be from the same time source. > > "Call this if you know for sure that the double represents monotonic time according to the same time source as MonotonicTime. It must be in seconds." Sounds nice. Changed. >> Source/WTF/wtf/cocoa/MemoryPressureHandlerCocoa.mm:162 >> + dispatch_source_set_timer(_timer_event_source, dispatch_time(DISPATCH_TIME_NOW, seconds.seconds() * NSEC_PER_SEC), DISPATCH_TIME_FOREVER, s_minimumHoldOffTime.seconds()); > > This needs a bug report, too. But I'm not actually sure that it's wrong... the comment up above indicates that it's supposed to be seconds. OK, filed. In the old code, we use s_minimumHoldOffTime for the last parameter of dispatch_source_set_timer. It should accept time in nanoseconds. But this s_minimumHoldOffTime is also used for the parameter of this holdOff function, which means s_minimumHoldOffTime is recognized in seconds. So the old code seems wrong.
Created attachment 334882 [details] Patch for landing Patch for landing
Add CCs for WebKit (old WebKit2) part reviews.
Comment on attachment 334882 [details] Patch for landing That is a big patch :) LGTM too. View in context: https://bugs.webkit.org/attachment.cgi?id=334882&action=review > Source/WebKit/Platform/IPC/ArgumentCoders.cpp:40 > +bool ArgumentCoder<Seconds>::decode(Decoder& decoder, Seconds& seconds) Sad that we still need this one. We usually try to implement this variant based on the std::optional variant. > Source/WebKit/Platform/IPC/ArgumentCoders.cpp:64 > +bool ArgumentCoder<MonotonicTime>::decode(Decoder& decoder, MonotonicTime& time) Ditto. > Source/WebKit/Platform/IPC/ArgumentCoders.h:528 > +}; We usually try to move these encode/decode methods closer to their actual class, i.e. Seconds.h.
Comment on attachment 334882 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=334882&action=review Thanks! >> Source/WebKit/Platform/IPC/ArgumentCoders.cpp:40 >> +bool ArgumentCoder<Seconds>::decode(Decoder& decoder, Seconds& seconds) > > Sad that we still need this one. > We usually try to implement this variant based on the std::optional variant. OK, I'll drop this legacy interface. >> Source/WebKit/Platform/IPC/ArgumentCoders.cpp:64 >> +bool ArgumentCoder<MonotonicTime>::decode(Decoder& decoder, MonotonicTime& time) > > Ditto. Ditto. >> Source/WebKit/Platform/IPC/ArgumentCoders.h:528 >> +}; > > We usually try to move these encode/decode methods closer to their actual class, i.e. Seconds.h. I've moved them to Seconds.h and MonotonicTime.h.
Comment on attachment 334882 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=334882&action=review >>> Source/WebKit/Platform/IPC/ArgumentCoders.cpp:40 >>> +bool ArgumentCoder<Seconds>::decode(Decoder& decoder, Seconds& seconds) >> >> Sad that we still need this one. >> We usually try to implement this variant based on the std::optional variant. > > OK, I'll drop this legacy interface. Ah, still need the old interface. I'll keep it as is (define legacy / modern decode functions in Seconds.h / MonotonicTime.h).
Committed r229174: <https://trac.webkit.org/changeset/229174>
<rdar://problem/38070449>
\o/
(In reply to Chris Dumez from comment #32) > \o/ Yay! It is really fun since this change actually finds several bugs :D