Bug 170723 - [JSC][GTK] Use RunLoop::Timer in GTK port
Summary: [JSC][GTK] Use RunLoop::Timer in GTK port
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-11 03:33 PDT by Yusuke Suzuki
Modified: 2017-04-12 09:59 PDT (History)
11 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2017-04-11 03:33:06 PDT 
[JSC][GTK] Use RunLoop::Timer in GTK port
Comment 1 Yusuke Suzuki 2017-04-11 03:46:40 PDT 
Created attachment 306794 [details]
Patch
Comment 2 Yusuke Suzuki 2017-04-11 03:52:12 PDT 
Created attachment 306795 [details]
Patch
Comment 3 Yusuke Suzuki 2017-04-11 03:54:45 PDT 
Created attachment 306796 [details]
Patch
Comment 4 Yusuke Suzuki 2017-04-11 04:03:13 PDT 
Created attachment 306799 [details]
Patch
Comment 5 Yusuke Suzuki 2017-04-11 04:26:59 PDT 
Created attachment 306800 [details]
Patch
Comment 6 Yusuke Suzuki 2017-04-11 04:29:10 PDT 
Created attachment 306801 [details]
Patch
Comment 7 Yusuke Suzuki 2017-04-11 04:36:16 PDT 
Created attachment 306802 [details]
Patch
Comment 8 Yusuke Suzuki 2017-04-11 04:40:19 PDT 
Created attachment 306803 [details]
Patch
Comment 9 Yusuke Suzuki 2017-04-11 05:13:46 PDT 
Created attachment 306805 [details]
Patch
Comment 10 Yusuke Suzuki 2017-04-11 05:18:01 PDT 
Created attachment 306806 [details]
Patch
Comment 11 Carlos Garcia Campos 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
Comment 12 Carlos Garcia Campos 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.
Comment 13 Yusuke Suzuki 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.
Comment 14 Yusuke Suzuki 2017-04-11 07:56:26 PDT 
Committed r215228: <http://trac.webkit.org/changeset/215228>
Comment 15 Ryan Haddad 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
Comment 16 Yusuke Suzuki 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.
Comment 17 Yusuke Suzuki 2017-04-11 08:48:11 PDT 
Committed r215230: <http://trac.webkit.org/changeset/215230>
Comment 18 Yusuke Suzuki 2017-04-11 08:59:55 PDT 
Committed r215232: <http://trac.webkit.org/changeset/215232>
Comment 19 Yusuke Suzuki 2017-04-12 09:59:56 PDT 
Debug build assertions should be fixed by https://bugs.webkit.org/show_bug.cgi?id=170775