Obey gtk-cursor-blink setting

Use gtk-cursor-blink from GtkSettings to set the
caret blink interval on Linux

This can be set with the "Cursor Blinking" slider and checkbox in gnome-keyboard-properties.  The changed setting will appear in a new renderer, so opening a new tab will see the change.

WebKit side is at https://bugs.webkit.org/show_bug.cgi?id=28931

Patch by Joel Stanley <joel@jms.id.au>;

BUG=20772

[Joel Stanley (old), 2009-09-02 15:44:22]

This is a first pass.  It compiles, and appears to stop the cursor from blinking
on my machine.

Wasn't sure who to add as a reviewer, so added everyone.

[Evan Martin, 2009-09-02 16:05:51]

Seems good so far.  I don't see where SetCursorBlinkInterval is getting called. 
In theory we should obey this on all platforms, so maybe it'd make more sense to
not have #ifdefs around the function definitions, but rather around the code
where you cast to LinuxTheme.

http://codereview.chromium.org/186009/diff/1/2
File chrome/common/gtk_util.cc (right):

http://codereview.chromium.org/186009/diff/1/2#newcode357
Line 357: gint cursor_blink_time = 1200;
Maybe have
static const gint kGtkDefaultCursorBlinkTime = 1200
above and initialize to that?

http://codereview.chromium.org/186009/diff/1/4
File chrome/common/renderer_preferences.h (right):

http://codereview.chromium.org/186009/diff/1/4#newcode63
Line 63: caret_blink_interval(0.5) {
Where does the 0.5 come from?  Should it also be pulled out into a constant?

[tony, 2009-09-03 00:14:27]

LGTM, just a nit.

http://codereview.chromium.org/186009/diff/1/2
File chrome/common/gtk_util.cc (right):

http://codereview.chromium.org/186009/diff/1/2#newcode378
Line 378: // XXX http://tinyurl.com/nges73 (mozilla's version)
Nit: Did you mean to keep these URLs here?

[Joel Stanley (old), 2009-09-03 02:02:41]

On 2009/09/02 16:05:51, Evan Martin wrote:
> Seems good so far.  I don't see where SetCursorBlinkInterval is getting
called. 

webkit/glue/webview_impl.cc, line 1785.

> In theory we should obey this on all platforms, so maybe it'd make more sense
to
> not have #ifdefs around the function definitions, but rather around the code
> where you cast to LinuxTheme.

I don't disagree.  I was following estade's focus ring color patch (r25278),
which ifdefs out the entire function.  I've rebased now that his change has
landed, perhaps it makes more sense now.  Let me know what you think.

[Joel Stanley (old), 2009-09-03 02:03:45]

Here's the review comments.

http://codereview.chromium.org/186009/diff/1/2
File chrome/common/gtk_util.cc (right):

http://codereview.chromium.org/186009/diff/1/2#newcode357
Line 357: gint cursor_blink_time = 1200;
On 2009/09/02 16:05:51, Evan Martin wrote:
> Maybe have
> static const gint kGtkDefaultCursorBlinkTime = 1200
> above and initialize to that?

Done.

http://codereview.chromium.org/186009/diff/1/2#newcode378
Line 378: // XXX http://tinyurl.com/nges73 (mozilla's version)
On 2009/09/03 00:14:27, tony wrote:
> Nit: Did you mean to keep these URLs here?

No. Fixed.

http://codereview.chromium.org/186009/diff/1/4
File chrome/common/renderer_preferences.h (right):

http://codereview.chromium.org/186009/diff/1/4#newcode63
Line 63: caret_blink_interval(0.5) {
On 2009/09/02 16:05:51, Evan Martin wrote:
> Where does the 0.5 come from?  Should it also be pulled out into a constant?

It comes from WebCore:

WebCore/rendering/RenderTheme.h
146:    virtual double caretBlinkInterval() const { return 0.5; }

I am setting it here so that platforms that aren't setting it specifically get
the right value.

OTOH, !Linux doesn't implement a
RenderThemeChromium$PLATFORM::caretBlinkInterval() so they will be getting the
default from from RenderTheme.h anyway.  

Do I leave out the default setting? Or just set it to 0?

[Evan Martin, 2009-09-03 21:10:10]

LGTM

http://codereview.chromium.org/186009/diff/5008/6009
File chrome/common/renderer_preferences.h (right):

http://codereview.chromium.org/186009/diff/5008/6009#newcode58
Line 58: // Currently only changed from default Linux.  Uses |gtk-cursor-blink|
from
typo: "default Linux" => "default on Linux"

[Joel Stanley (old), 2009-09-05 15:22:28]

You were right in your first comment; SetCursorBlinkInterval was not being
called.

I also updated the millisecond -> second scaling.  Doing the straight unit
conversion gave a much lower frequency compared the url bar, which looks wrong. 
So we match what the webkit gtk port does and divide by 2 as well.

Also, the webkit side has now landed upstream.

[dimitri.glazkov, 2009-09-06 04:13:52]

Joel, I had to roll out the webkit side. It caused layout test failures and I
needed to roll past it to fix a regression.

[Joel Stanley (old), 2009-09-08 12:49:09]

he patch has landed upstream and stuck the second time.

Can I get this reviewed one last time (there's some changes since Evan LGTM'd)
so it's ready to commit once the new webkit revision is pulled in?

Thanks

[Evan Martin, 2009-09-08 18:43:23]

LGTM++

http://codereview.chromium.org/186009/diff/9016/7004
File chrome/common/gtk_util.cc (right):

http://codereview.chromium.org/186009/diff/9016/7004#newcode357
Line 357: static const gint kGtkDefaultCursorBlinkTime = 1200;
Maybe reference where you got this number from?  (E.g. "from gtkfoobar.c,
variable is called blah")

[Joel Stanley (old), 2009-09-09 02:08:42]

Thanks.  Please commit for me.

http://codereview.chromium.org/186009/diff/9016/7004
File chrome/common/gtk_util.cc (right):

http://codereview.chromium.org/186009/diff/9016/7004#newcode357
Line 357: static const gint kGtkDefaultCursorBlinkTime = 1200;
On 2009/09/08 18:43:23, Evan Martin wrote:
> Maybe reference where you got this number from?  (E.g. "from gtkfoobar.c,
> variable is called blah")

Done.

[Evan Martin, 2009-09-09 22:07:17]

r25797

[Evan Martin, 2009-09-09 22:56:24]

reverted:
16:14:14 memcheck_analyze.py [ERROR] UninitCondition
Conditional jump or move depends on uninitialised value(s)
  WebCore::TimerBase::setNextFireTime(double)
(third_party/WebKit/WebCore/platform/Timer.cpp:295)
  WebCore::TimerBase::start(double, double)
(third_party/WebKit/WebCore/platform/Timer.cpp:184)
  WebCore::Frame::selectionLayoutChanged()
(third_party/WebKit/WebCore/platform/Timer.h:45)
  WebCore::SelectionController::setSelection(WebCore::VisibleSelection const&,
bool, bool, bool)
(third_party/WebKit/WebCore/editing/SelectionController.cpp:147)
  WebCore::SelectionController::setSelectedRange(WebCore::Range*,
WebCore::EAffinity, bool)
(third_party/WebKit/WebCore/editing/SelectionController.cpp:1219)
  WebCore::Editor::setComposition(WebCore::String const&,
WTF::Vector<WebCore::CompositionUnderline, 0u> const&, unsigned int, unsigned
int) (third_party/WebKit/WebCore/editing/Editor.cpp:1415)
  WebViewImpl::handleCompositionEvent(WebKit::WebCompositionCommand, int, int,
int, WebKit::WebString const&) (webkit/glue/webview_impl.cc:1190)
  RenderWidget::OnImeSetComposition(WebKit::WebCompositionCommand, int, int,
int, std::basic_string<unsigned short, base::string16_char_traits,
std::allocator<unsigned short> > const&) (chrome/renderer/render_widget.cc:747)
  RenderViewTest_ImeComposition_Test::TestBody()
(chrome/renderer/render_view_unittest.cc:241)
  testing::Test::Run() (testing/gtest/src/gtest.cc:2056)
  testing::internal::TestInfoImpl::Run() (testing/gtest/src/gtest.cc:2287)
  testing::internal::TestInfoImpl::RunTest(testing::TestInfo*)
(testing/gtest/src/gtest-internal-inl.h:655)
  testing::TestCase::Run() (testing/gtest/src/gtest-internal-inl.h:397)
  testing::TestCase::RunTestCase(testing::TestCase*)
(testing/gtest/src/gtest-internal-inl.h:776)
  testing::internal::UnitTestImpl::RunAllTests()
(testing/gtest/src/gtest-internal-inl.h:397)
  testing::UnitTest::Run() (testing/gtest/src/gtest.cc:3404)
  main (base/test_suite.h:93)

[Joel Stanley (old), 2009-09-17 04:22:25]

I've uploaded a rebased version.  It now initialises  caret_blink_interval in
renderer_preferences.h, which was the only initialised variable added in this
patch.

I tried to valgrind it locally, but I'm hitting a bug with 64bit binaries.  Will
try building for 32bit.

[Evan Martin, 2009-09-17 19:14:59]

I patched this in but it didn't compile (typo "LINUX" for "OS_LINUX" in one
place).  I have fixed that and I am just gonna run it by one other person before
I commit.

[Evan Martin, 2009-09-17 19:29:13]

I think I recall Darin saying that these sorts of settings need to go through
the web preferences, but my memory is so vague I cannot tell you what that even
means.

[tony, 2009-09-17 19:35:25]

WebPreferences are preferences in WebCore that all ports can set (like default
font size or whether javascript is enabled).  We've been using
RendererPreferences for settings that are specific to the port (like whether we
should anti-alias or not Linux).  If this is something that all ports would want
to be able to control, then we should push for getting this into the WebCore
Settings object.

[darin (slow to review), 2009-09-17 19:39:09]

As with the focus ring color, this property is really a global property of the
renderer.  While, Page has a theme, it really is always returning the default
theme.  (Only Safari for Windows has multiple themes, and then it only has two.)

It seems to me that we should just have a place in the WebKit API for setting
properties of the global theme.  We should use that instead of adding preference
setters on WebView.

Also, I think we should only have to initialize these global settings once per
renderer process instead of once per RenderView as we currently do.

Technically, WebPreferences is meant to map to WebKit::WebSettings, and
RendererPreferences corresponds to the rest.  I think it would be nice if we
could change things so that RendererPreferences could contain only the global
settings for a Renderer and WebPreferences could contain the preferences for a
particular RenderView/WebView.

This is a bigger change I'm advocating, but I think cleaning this up is long
overdue.

[tony, 2009-09-17 20:41:24]

I see.  Would that mean that some WebKit::WebSettings would be in WebPreferences
and some would be in RendererPreferences?  We treat some settings as global to
the browser (like plugins enabled or not
or default font faces) even though they are in WebKit::WebSettings.


On 2009/09/17 19:39:09, darin wrote:
> As with the focus ring color, this property is really a global property of the
> renderer.  While, Page has a theme, it really is always returning the default
> theme.  (Only Safari for Windows has multiple themes, and then it only has
two.)
> 
> It seems to me that we should just have a place in the WebKit API for setting
> properties of the global theme.  We should use that instead of adding
preference
> setters on WebView.
> 
> Also, I think we should only have to initialize these global settings once per
> renderer process instead of once per RenderView as we currently do.
> 
> Technically, WebPreferences is meant to map to WebKit::WebSettings, and
> RendererPreferences corresponds to the rest.  I think it would be nice if we
> could change things so that RendererPreferences could contain only the global
> settings for a Renderer and WebPreferences could contain the preferences for a
> particular RenderView/WebView.
> 
> This is a bigger change I'm advocating, but I think cleaning this up is long
> overdue.

[darin (slow to review), 2009-09-17 21:20:58]

Hmm...  maybe we should step back and create two structures:
 RenderProcessSettings and RenderViewSettings.

Then it would be really obvious which things go where based on whether they
are per process or per view.  When constructing a new WebView, we'd probably
draw from each structure to populate the WebSettings for the WebView.

WDYT?

-Darin


On Thu, Sep 17, 2009 at 1:41 PM, <tony@chromium.org> wrote:

> I see.  Would that mean that some WebKit::WebSettings would be in
> WebPreferences
> and some would be in RendererPreferences?  We treat some settings as global
> to
> the browser (like plugins enabled or not
> or default font faces) even though they are in WebKit::WebSettings.
>
>
>
> On 2009/09/17 19:39:09, darin wrote:
>
>> As with the focus ring color, this property is really a global property of
>> the
>> renderer.  While, Page has a theme, it really is always returning the
>> default
>> theme.  (Only Safari for Windows has multiple themes, and then it only has
>>
> two.)
>
>  It seems to me that we should just have a place in the WebKit API for
>> setting
>> properties of the global theme.  We should use that instead of adding
>>
> preference
>
>> setters on WebView.
>>
>
>  Also, I think we should only have to initialize these global settings once
>> per
>> renderer process instead of once per RenderView as we currently do.
>>
>
>  Technically, WebPreferences is meant to map to WebKit::WebSettings, and
>> RendererPreferences corresponds to the rest.  I think it would be nice if
>> we
>> could change things so that RendererPreferences could contain only the
>> global
>> settings for a Renderer and WebPreferences could contain the preferences
>> for a
>> particular RenderView/WebView.
>>
>
>  This is a bigger change I'm advocating, but I think cleaning this up is
>> long
>> overdue.
>>
>
>
>
> http://codereview.chromium.org/186009
>

[tony, 2009-09-17 21:31:00]

Thinking about this a bit more, what would actually go into
RenderViewSettings?  It looks like everything that's currently in
WebPreferences and RendererPreferences would fall under
RenderProcessSettings.

On Thu, Sep 17, 2009 at 2:20 PM, Darin Fisher <darin@chromium.org> wrote:
> Hmm... =A0maybe we should step back and create two structures:
> =A0RenderProcessSettings and RenderViewSettings.
> Then it would be really obvious which things go where based on whether th=
ey
> are per process or per view. =A0When constructing a new WebView, we'd pro=
bably
> draw from each structure to populate the WebSettings for the WebView.
> WDYT?
> -Darin
>
> On Thu, Sep 17, 2009 at 1:41 PM, <tony@chromium.org> wrote:
>>
>> I see. =A0Would that mean that some WebKit::WebSettings would be in
>> WebPreferences
>> and some would be in RendererPreferences? =A0We treat some settings as
>> global to
>> the browser (like plugins enabled or not
>> or default font faces) even though they are in WebKit::WebSettings.
>>
>>
>> On 2009/09/17 19:39:09, darin wrote:
>>>
>>> As with the focus ring color, this property is really a global property
>>> of the
>>> renderer. =A0While, Page has a theme, it really is always returning the
>>> default
>>> theme. =A0(Only Safari for Windows has multiple themes, and then it onl=
y
>>> has
>>
>> two.)
>>
>>> It seems to me that we should just have a place in the WebKit API for
>>> setting
>>> properties of the global theme. =A0We should use that instead of adding
>>
>> preference
>>>
>>> setters on WebView.
>>
>>> Also, I think we should only have to initialize these global settings
>>> once per
>>> renderer process instead of once per RenderView as we currently do.
>>
>>> Technically, WebPreferences is meant to map to WebKit::WebSettings, and
>>> RendererPreferences corresponds to the rest. =A0I think it would be nic=
e if
>>> we
>>> could change things so that RendererPreferences could contain only the
>>> global
>>> settings for a Renderer and WebPreferences could contain the preference=
s
>>> for a
>>> particular RenderView/WebView.
>>
>>> This is a bigger change I'm advocating, but I think cleaning this up is
>>> long
>>> overdue.
>>
>>
>>
>> http://codereview.chromium.org/186009
>
>

[darin (slow to review), 2009-09-17 21:50:03]

Wow, I think you are right!  I guess the purpose of glue/webpreferences.h
went outthe door with the advent of WebSettings, so we should just lump
WebPreferences
into RendererPreferences, and only send it once when starting up a renderer.

Naming it RenderProcessSettings might be nice in case we ever do need to
have
view-specific settings.

-Darin


On Thu, Sep 17, 2009 at 2:30 PM, Tony Chang <tony@chromium.org> wrote:

> Thinking about this a bit more, what would actually go into
> RenderViewSettings?  It looks like everything that's currently in
> WebPreferences and RendererPreferences would fall under
> RenderProcessSettings.
>
> On Thu, Sep 17, 2009 at 2:20 PM, Darin Fisher <darin@chromium.org> wrote:
> > Hmm...  maybe we should step back and create two structures:
> >  RenderProcessSettings and RenderViewSettings.
> > Then it would be really obvious which things go where based on whether
> they
> > are per process or per view.  When constructing a new WebView, we'd
> probably
> > draw from each structure to populate the WebSettings for the WebView.
> > WDYT?
> > -Darin
> >
> > On Thu, Sep 17, 2009 at 1:41 PM, <tony@chromium.org> wrote:
> >>
> >> I see.  Would that mean that some WebKit::WebSettings would be in
> >> WebPreferences
> >> and some would be in RendererPreferences?  We treat some settings as
> >> global to
> >> the browser (like plugins enabled or not
> >> or default font faces) even though they are in WebKit::WebSettings.
> >>
> >>
> >> On 2009/09/17 19:39:09, darin wrote:
> >>>
> >>> As with the focus ring color, this property is really a global property
> >>> of the
> >>> renderer.  While, Page has a theme, it really is always returning the
> >>> default
> >>> theme.  (Only Safari for Windows has multiple themes, and then it only
> >>> has
> >>
> >> two.)
> >>
> >>> It seems to me that we should just have a place in the WebKit API for
> >>> setting
> >>> properties of the global theme.  We should use that instead of adding
> >>
> >> preference
> >>>
> >>> setters on WebView.
> >>
> >>> Also, I think we should only have to initialize these global settings
> >>> once per
> >>> renderer process instead of once per RenderView as we currently do.
> >>
> >>> Technically, WebPreferences is meant to map to WebKit::WebSettings, and
> >>> RendererPreferences corresponds to the rest.  I think it would be nice
> if
> >>> we
> >>> could change things so that RendererPreferences could contain only the
> >>> global
> >>> settings for a Renderer and WebPreferences could contain the
> preferences
> >>> for a
> >>> particular RenderView/WebView.
> >>
> >>> This is a bigger change I'm advocating, but I think cleaning this up is
> >>> long
> >>> overdue.
> >>
> >>
> >>
> >> http://codereview.chromium.org/186009
> >
> >
>

[tony, 2009-09-17 22:03:52]

That sounds good to me!  We'd still need to resend if a preference
changes, but that's already true with WebPreferences.  I guess
RenderProcessSettings would just include WebPreferences as a member.
I will file a bug.

On Thu, Sep 17, 2009 at 2:49 PM, Darin Fisher <darin@chromium.org> wrote:
> Wow, I think you are right! =A0I guess the purpose of glue/webpreferences=
.h
> went out
> the door with the advent of WebSettings, so we should just lump
> WebPreferences
> into RendererPreferences, and only send it once when starting up a render=
er.
> Naming it RenderProcessSettings might be nice in case we ever do need to
> have
> view-specific settings.
> -Darin
>
> On Thu, Sep 17, 2009 at 2:30 PM, Tony Chang <tony@chromium.org> wrote:
>>
>> Thinking about this a bit more, what would actually go into
>> RenderViewSettings? =A0It looks like everything that's currently in
>> WebPreferences and RendererPreferences would fall under
>> RenderProcessSettings.
>>
>> On Thu, Sep 17, 2009 at 2:20 PM, Darin Fisher <darin@chromium.org> wrote=
:
>> > Hmm... =A0maybe we should step back and create two structures:
>> > =A0RenderProcessSettings and RenderViewSettings.
>> > Then it would be really obvious which things go where based on whether
>> > they
>> > are per process or per view. =A0When constructing a new WebView, we'd
>> > probably
>> > draw from each structure to populate the WebSettings for the WebView.
>> > WDYT?
>> > -Darin
>> >
>> > On Thu, Sep 17, 2009 at 1:41 PM, <tony@chromium.org> wrote:
>> >>
>> >> I see. =A0Would that mean that some WebKit::WebSettings would be in
>> >> WebPreferences
>> >> and some would be in RendererPreferences? =A0We treat some settings a=
s
>> >> global to
>> >> the browser (like plugins enabled or not
>> >> or default font faces) even though they are in WebKit::WebSettings.
>> >>
>> >>
>> >> On 2009/09/17 19:39:09, darin wrote:
>> >>>
>> >>> As with the focus ring color, this property is really a global
>> >>> property
>> >>> of the
>> >>> renderer. =A0While, Page has a theme, it really is always returning =
the
>> >>> default
>> >>> theme. =A0(Only Safari for Windows has multiple themes, and then it =
only
>> >>> has
>> >>
>> >> two.)
>> >>
>> >>> It seems to me that we should just have a place in the WebKit API fo=
r
>> >>> setting
>> >>> properties of the global theme. =A0We should use that instead of add=
ing
>> >>
>> >> preference
>> >>>
>> >>> setters on WebView.
>> >>
>> >>> Also, I think we should only have to initialize these global setting=
s
>> >>> once per
>> >>> renderer process instead of once per RenderView as we currently do.
>> >>
>> >>> Technically, WebPreferences is meant to map to WebKit::WebSettings,
>> >>> and
>> >>> RendererPreferences corresponds to the rest. =A0I think it would be =
nice
>> >>> if
>> >>> we
>> >>> could change things so that RendererPreferences could contain only t=
he
>> >>> global
>> >>> settings for a Renderer and WebPreferences could contain the
>> >>> preferences
>> >>> for a
>> >>> particular RenderView/WebView.
>> >>
>> >>> This is a bigger change I'm advocating, but I think cleaning this up
>> >>> is
>> >>> long
>> >>> overdue.
>> >>
>> >>
>> >>
>> >> http://codereview.chromium.org/186009
>> >
>> >
>
>