[webkit-reviews] review granted: [Bug 170775] [JSC][GTK] glib RunLoop does not accept negative start interval : [Attachment 306915] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 170775: [JSC][GTK] glib RunLoop does not accept negative start interval
https://bugs.webkit.org/show_bug.cgi?id=170775

Attachment 306915: Patch

https://bugs.webkit.org/attachment.cgi?id=306915&action=review




--- Comment #9 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 306915
  --> https://bugs.webkit.org/attachment.cgi?id=306915
Patch

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

r=me

>>>>> Source/JavaScriptCore/heap/GCActivityCallback.cpp:92
>>>>> +    m_timer.startOneShot(std::max<Seconds>(secondsUntilFire - delta,
0_s));
>>>> 
>>>> Why is this needed?
>>> 
>>> For `std::max<Seconds>()`, this is because RunLoop::Timer in glib has
assumption that incoming interval is not negative.
>> 
>> Why not remove that assumption since it's wrong? Seems cleaner to me.
Numbers less than current time happen for very natural reasons in code.
> 
> I think `startOneShot(negative)` is ok. But I'm worried about that it becomes
a bit ugly semantics when getting `startRepeating(negative)`.

Ok. It seems reasonable to me to make the change in startOneShot, but I'll
leave it up to you