Bug 170723

Summary:

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

Product:

WebKit

Reporter:

Yusuke Suzuki <ysuzuki>

Component:

New Bugs

Assignee:

Yusuke Suzuki <ysuzuki>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, buildbot, cdumez, cgarcia, cmarcelo, dbates, keith_miller, mark.lam, msaboff, ryanhaddad, saam

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	cgarcia: review+

Yusuke Suzuki

Reported 2017-04-11 03:33:06 PDT

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

Attachments
Patch (25.66 KB, patch) 2017-04-11 03:46 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (26.66 KB, patch) 2017-04-11 03:52 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (26.76 KB, patch) 2017-04-11 03:54 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (26.72 KB, patch) 2017-04-11 04:03 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (27.13 KB, patch) 2017-04-11 04:26 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (27.24 KB, patch) 2017-04-11 04:29 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (27.16 KB, patch) 2017-04-11 04:36 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (27.16 KB, patch) 2017-04-11 04:40 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (27.33 KB, patch) 2017-04-11 05:13 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (27.42 KB, patch) 2017-04-11 05:18 PDT, Yusuke Suzuki	cgarcia: review+	Details Formatted Diff Diff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.

Yusuke Suzuki

Comment 1 2017-04-11 03:46:40 PDT

Created attachment 306794 [details] Patch

Yusuke Suzuki

Comment 2 2017-04-11 03:52:12 PDT

Created attachment 306795 [details] Patch

Yusuke Suzuki

Comment 3 2017-04-11 03:54:45 PDT

Created attachment 306796 [details] Patch

Yusuke Suzuki

Comment 4 2017-04-11 04:03:13 PDT

Created attachment 306799 [details] Patch

Yusuke Suzuki

Comment 5 2017-04-11 04:26:59 PDT

Created attachment 306800 [details] Patch

Yusuke Suzuki

Comment 6 2017-04-11 04:29:10 PDT

Created attachment 306801 [details] Patch

Yusuke Suzuki

Comment 7 2017-04-11 04:36:16 PDT

Created attachment 306802 [details] Patch

Yusuke Suzuki

Comment 8 2017-04-11 04:40:19 PDT

Created attachment 306803 [details] Patch

Yusuke Suzuki

Comment 9 2017-04-11 05:13:46 PDT

Created attachment 306805 [details] Patch

Yusuke Suzuki

Comment 10 2017-04-11 05:18:01 PDT

Created attachment 306806 [details] Patch

Carlos Garcia Campos

Comment 11 2017-04-11 05:21:42 PDT

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 12 2017-04-11 05:28:51 PDT

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 13 2017-04-11 05:39:00 PDT

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

Comment 14 2017-04-11 07:56:26 PDT

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

Ryan Haddad

Comment 15 2017-04-11 08:45:25 PDT

(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

Comment 16 2017-04-11 08:45:43 PDT

(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

Comment 17 2017-04-11 08:48:11 PDT

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