Bug 170723 – [JSC][GTK] Use RunLoop::Timer in GTK port

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC][GTK] Use RunLoop::Timer in GTK port

[Yusuke Suzuki]

Created attachment 306794 [details]
Patch

[Yusuke Suzuki]

Created attachment 306795 [details]
Patch

[Yusuke Suzuki]

Created attachment 306796 [details]
Patch

[Yusuke Suzuki]

Created attachment 306799 [details]
Patch

[Yusuke Suzuki]

Created attachment 306800 [details]
Patch

[Yusuke Suzuki]

Created attachment 306801 [details]
Patch

[Yusuke Suzuki]

Created attachment 306802 [details]
Patch

[Yusuke Suzuki]

Created attachment 306803 [details]
Patch

[Yusuke Suzuki]

Created attachment 306805 [details]
Patch

[Yusuke Suzuki]

Created attachment 306806 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 306803 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:135
> +    cancelTimer();

I'm not sure we really want to explicitly cancel the timer here, because it also sets m_isScheduled to false. We were not doing that before, we only changed the ready time to s_decade

[Carlos Garcia Campos]

Comment on attachment 306806 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:135
> +    cancelTimer();

Please, see my comment in previous review.

[Yusuke Suzuki]

Comment on attachment 306803 [details]
Patch

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

Thanks

>> Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:135
>> +    cancelTimer();
> 
> I'm not sure we really want to explicitly cancel the timer here, because it also sets m_isScheduled to false. We were not doing that before, we only changed the ready time to s_decade

OK, let's revert this part in this patch. But I'm now a bit thinking about it.
In CF port, we always call setRunLoop. It is non-null for the first time (since we fill it with CFRunLoopGetCurrent() in Heap.cpp).
So, CF timer starts with CFRunLoopTimerCreate & CFRunLoopAddTimer. It's storange that the timer starts with `m_isScheduled = false`.
I think this is a bug and we need to fix it. At that time, we should fix the both CF & non-CF ports.

[Yusuke Suzuki]

Committed r215228: <http://trac.webkit.org/changeset/215228>

[Ryan Haddad]

(In reply to Yusuke Suzuki from comment #14)
> Committed r215228: <http://trac.webkit.org/changeset/215228>

This change appears to have broken the Windows build:

https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/592

[Yusuke Suzuki]

(In reply to Ryan Haddad from comment #15)
> (In reply to Yusuke Suzuki from comment #14)
> > Committed r215228: <http://trac.webkit.org/changeset/215228>
> 
> This change appears to have broken the Windows build:
> 
> https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/
> 592

Fixing it now.

[Yusuke Suzuki]

Committed r215230: <http://trac.webkit.org/changeset/215230>

[Yusuke Suzuki]

Committed r215232: <http://trac.webkit.org/changeset/215232>

[Yusuke Suzuki]

Debug build assertions should be fixed by https://bugs.webkit.org/show_bug.cgi?id=170775