Summary: | [GTK] Use GMainLoopSource in WebKitTestRunner | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | ap, clopez, commit-queue, gustavo, pnormand, svillar | ||||
Priority: | P2 | Keywords: | Gtk | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | 139124 | ||||||
Bug Blocks: | |||||||
Attachments: |
|
Description
Carlos Garcia Campos
2014-11-18 04:19:48 PST
Created attachment 241782 [details]
Patch
Comment on attachment 241782 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241782&action=review > Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp:70 > + }, std::chrono::duration_cast<std::chrono::microseconds>(std::chrono::duration<double>(timeout))); I think you forgot to remove the timeoutCallback static function now that you have the lambda. (In reply to comment #2) > Comment on attachment 241782 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=241782&action=review > > > Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp:70 > > + }, std::chrono::duration_cast<std::chrono::microseconds>(std::chrono::duration<double>(timeout))); > > I think you forgot to remove the timeoutCallback static function now that > you have the lambda. Right! Committed r176566: <http://trac.webkit.org/changeset/176566> (In reply to comment #4) > Committed r176566: <http://trac.webkit.org/changeset/176566> This broke the performance bot. Reported here: https://bugs.webkit.org/show_bug.cgi?id=139122 Re-opened since this is blocked by bug 139124 I reverted r176566 on r176591 <https://trac.webkit.org/r176591> after talking with Carlos Garcia. (In reply to comment #7) > I reverted r176566 on r176591 <https://trac.webkit.org/r176591> after > talking with Carlos Garcia. Ok, I forgot to handle the case of no-timeout :-P. I'll land a right version. Committed r176921: <http://trac.webkit.org/changeset/176921> Comment on attachment 241782 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241782&action=review > Tools/WebKitTestRunner/InjectedBundle/gtk/TestRunnerGtk.cpp:52 > + m_waitToDumpWatchdogTimer.scheduleAfterDelay("[WTR] waitToDumpWatchdogTimerCallback", [this] { waitToDumpWatchdogTimerFired(); }, > + std::chrono::duration_cast<std::chrono::microseconds>(std::chrono::duration<double>(waitToDumpWatchdogTimerInterval))); waitToDumpWatchdogTimerInterval is 30, for 30 seconds. Is the cast to microseconds the right one here? > Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp:71 > gtk_main(); Ditto, why microseconds? (In reply to comment #10) > Comment on attachment 241782 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=241782&action=review > > > Tools/WebKitTestRunner/InjectedBundle/gtk/TestRunnerGtk.cpp:52 > > + m_waitToDumpWatchdogTimer.scheduleAfterDelay("[WTR] waitToDumpWatchdogTimerCallback", [this] { waitToDumpWatchdogTimerFired(); }, > > + std::chrono::duration_cast<std::chrono::microseconds>(std::chrono::duration<double>(waitToDumpWatchdogTimerInterval))); > > waitToDumpWatchdogTimerInterval is 30, for 30 seconds. Is the cast to > microseconds the right one here? Yes, the scheduleAfterDelay() version that receives the delay in microseconds creates a glib source using microseconds. For this particular case we could use milliseconds, but we moved all sources using a double value to microseconds to avoid truncations (we had cases in which timers were scheduled immediately because the conversion to milliseconds was 0, see bug #137782). > > Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp:71 > > gtk_main(); > > Ditto, why microseconds? |