Bug 182911 – Remove monotonicallyIncreasingTime

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 334123 [details]
Patch

[Yusuke Suzuki]

Created attachment 334507 [details]
Patch

[Yusuke Suzuki]

Created attachment 334508 [details]
Patch

[Yusuke Suzuki]

Created attachment 334509 [details]
Patch

[Yusuke Suzuki]

Created attachment 334510 [details]
Patch

[Yusuke Suzuki]

Great. During this refactoring, I found several bugs mixing

1. MediaTime and MonotonicTime
2. Seconds and MonotonicTime.

[Yusuke Suzuki]

Created attachment 334520 [details]
Patch

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 334525 [details]
Patch

[Yusuke Suzuki]

Created attachment 334526 [details]
Patch

[Yusuke Suzuki]

Created attachment 334530 [details]
Patch

[Yusuke Suzuki]

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?

[Michael Catanzaro]

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.

[Yusuke Suzuki]

Created attachment 334576 [details]
Patch

[Yusuke Suzuki]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Michael Catanzaro]

Would be super if you could report a bug to fix TextureMapperAnimation.

[Yusuke Suzuki]

Created attachment 334587 [details]
Patch

[Yusuke Suzuki]

Created attachment 334588 [details]
Patch

[Yusuke Suzuki]

(In reply to Michael Catanzaro from comment #18)
> Would be super if you could report a bug to fix TextureMapperAnimation.

OK, filed.

[Zan Dobersek]

Filed as bugs #183112 and #183113. Probably better to wait for this to land.

[Michael Catanzaro]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 334882 [details]
Patch for landing

Patch for landing

[Yusuke Suzuki]

Add CCs for WebKit (old WebKit2) part reviews.

[youenn fablet]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Committed r229174: <https://trac.webkit.org/changeset/229174>

[Radar WebKit Bug Importer]

<rdar://problem/38070449>

[Chris Dumez]

\o/

[Yusuke Suzuki]

(In reply to Chris Dumez from comment #32)
> \o/

Yay! It is really fun since this change actually finds several bugs :D