This has recently been added and the patch is good. It's just revealing a problem with our timers. The test is posting a message recursively, and also starts a timeout timer to finish the test. The timeout timer is never fired for us, because WebCore timers have lower priority than the one used by postMessage. ScriptExecutionContext uses Document::postTask, that uses scheduleOnMainThread, that uses RunLoop::dispatch(). We are not setting any priority for the timer used by RunLoop::dispatch, so it's using the default. RunLoop::dispatch is normally used to schedule tasks between threads, or just to ensure something is run in a different run loop iteration, but in general nothing urgent as a graphics redraw or something like that. It's quite common to use g_idle_add to schedule tasks between threads, so I think it makes sense to use G_PRIORITY_DEFAULT_IDLE for the RunLoop timer.
Created attachment 302073 [details] Patch
I wonder what this will break!
(In reply to comment #2) > I wonder what this will break! You always so negative, I wonder what other things will fix.
Committed r212622: <http://trac.webkit.org/changeset/212622>
Reverted r212622 for reason: Caused several test failures Committed r212627: <http://trac.webkit.org/changeset/212627>
So, yes, this introduced a few failures (~10) most of them image only failures. I'm still not sure if they are actual regressions, or the patch actually revealed those bugs. But timer priorities is tricky thing that could also impact the performance, so we should think more carefully about it.
(In reply to comment #6) > So, yes, this introduced a few failures (~10) most of them image only > failures. I'm still not sure if they are actual regressions, or the patch > actually revealed those bugs. But timer priorities is tricky thing that > could also impact the performance, so we should think more carefully about > it. I get to be right every once in a while! I'm actually most concerned about the original test, because it's testing the behavior of the timers themselves, and it shows us that our timers are definitely broken without your patch. But clearly we need to think about this some more and understand what's going on with the other tests. Timers are hard.
Created attachment 302252 [details] Patch
Comment on attachment 302252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302252&action=review > Source/WTF/wtf/glib/MainThreadGLib.cpp:38 > +class MainThreadDispatcher { Please add a comment. You could take it from your commit message. > Source/WTF/wtf/glib/MainThreadGLib.cpp:43 > + m_timer.setPriority(G_PRIORITY_HIGH_IDLE + 20); Too fragile. It has to be exactly equal to WebCore timers? Then you should fine some common header to define it in.
(In reply to comment #9) > Comment on attachment 302252 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=302252&action=review > > > Source/WTF/wtf/glib/MainThreadGLib.cpp:38 > > +class MainThreadDispatcher { > > Please add a comment. You could take it from your commit message. Ok. > > Source/WTF/wtf/glib/MainThreadGLib.cpp:43 > > + m_timer.setPriority(G_PRIORITY_HIGH_IDLE + 20); > > Too fragile. It has to be exactly equal to WebCore timers? Then you should > fine some common header to define it in. Not really, I'm just conservative here and using at least the same priority. I still think we should re-think all the timer priorities anyway.
Created attachment 302260 [details] Patch
Committed r212814: <http://trac.webkit.org/changeset/212814>