Currently the GTK theme code returns 0 from caretBlinkFrequency when the caret shouldn't blink. This means the caret should always be drawn. I would like to do something similar in the Windows Chromium port. In Windows you can set the cursor to never blink. Currently returning 0 causes the timer to constantly be hit, since a startRepeating(0) causes the timer to fire right away, get removed (since the interval is 0), and then go through selectionLayoutChanged, which causes the timer to be set again since !d->m_caretBlinkTimer.isActive(). This is causing both a paint and timer storm. I believe the fix should be simple, something along the lines of (Frame.cpp): // Only setup timers if we actually want to blink. if (double blinkInterval = theme()->caretBlinkFrequency()) { d->m_caretBlinkTimer.startRepeating(blinkInterval); } I will send out a patch for review. Additionally I would like to change the misleading name caretBlinkFrequency to be the more accurate caretBlinkInterval. Let me know if this sounds ok.
Created attachment 25472 [details] Handle a non-blinking interval.
Comment on attachment 25472 [details] Handle a non-blinking interval. I'm not sure why you're renaming the method. Doesn't really matter to me so long as all callers and implementers were successfully renamed. Tabs in your changelog: + Renamed caretBlinkFrequency to caretBlinkInterval. The description in the bug was much nicer than the one in the Changelog, perhaps you write something similar in the ChangeLog itself, and make sure to reference this bug number in the ChangeLog. WebKit Style: http://webkit.org/coding/coding-style.html has no { } around single-line ifs: + if (float blinkInterval = theme()->caretBlinkInterval()) { + d->m_caretBlinkTimer.startRepeating(blinkInterval); + } Thanks for the patch. It's hard for me to tell if this is right or not. Is it possible to have a non-blinking caret on Windows as well? Will that cause a "blink/timer storm" like the Gtk setting will?
I renamed the method because it is not the frequency (blinks per second), it is the interval / period (amount of time between the caret toggling on or off). Windows allows you to set a non-blinking cursor in the control panel, so yes, this case is important if you want to obey the system settings on Windows. I will try to improve the changelog and have a new patch up in a second.
Created attachment 25492 [details] Incorporated review feedback
Comment on attachment 25492 [details] Incorporated review feedback Gorgeous.
Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/page/Frame.cpp M WebCore/platform/Theme.h M WebCore/platform/gtk/RenderThemeGtk.cpp M WebCore/platform/gtk/RenderThemeGtk.h M WebCore/rendering/RenderTheme.h Committed r38911