Bug 31704 - [chromium] Add setCaretBlinkInterval API
Summary: [chromium] Add setCaretBlinkInterval API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-19 21:03 PST by Joel Stanley
Modified: 2010-02-03 07:40 PST (History)
3 users (show)

See Also:


Attachments
patch (2.26 KB, patch)
2009-11-19 21:05 PST, Joel Stanley
fishd: review-
Details | Formatted Diff | Diff
updated (8.22 KB, patch)
2010-02-02 05:13 PST, Joel Stanley
no flags Details | Formatted Diff | Diff
patch (8.22 KB, patch)
2010-02-02 15:16 PST, Joel Stanley
levin: review+
levin: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joel Stanley 2009-11-19 21:03:13 PST
With this, the gtk port of chromium will set the caret blink rate.

Chromium side: http://codereview.chromium.org/398003/show
Comment 1 Joel Stanley 2009-11-19 21:05:44 PST
Created attachment 43547 [details]
patch
Comment 2 Evan Martin 2009-11-19 21:21:14 PST
(I am not a reviewer)

The changelog description should probably mention what port it's for.

The comment in WebView.h is probably unnecessary.

LGTM otherwise
Comment 3 Darin Fisher (:fishd, Google) 2009-11-19 21:27:33 PST
Comment on attachment 43547 [details]
patch

> +++ b/WebKit/chromium/src/WebViewImpl.cpp
...
> +void WebViewImpl::setCaretBlinkInterval(double interval) {
> +#if PLATFORM(LINUX)
> +    reinterpret_cast<RenderThemeChromiumLinux*>(theme())->setCaretBlinkInterval(interval);
> +#endif
> +}

theme() actually returns a singleton, so mutating this singleton via
a WebView instance is a bit confusing since it would cause the value
to be changed for all WebView instances.

since this is Linux only, perhaps you should just create a linux specific
header file in public/linux that exposes a static setter for this propery.

-darin
Comment 4 Joel Stanley 2010-02-02 05:13:30 PST
Created attachment 47924 [details]
updated

Here is another try at this patch.

Note that it doesn't pass check-webkit-style due to unnecessary indentation in WebCore/rendering/RenderThemeChromiumLinux.h, which was preexisting.  I left it as is to minimise noise in the patch.
Comment 5 Joel Stanley 2010-02-02 15:16:46 PST
Created attachment 47977 [details]
patch
Comment 6 David Levin 2010-02-02 16:18:16 PST
Comment on attachment 47977 [details]
patch

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +2010-02-02  Joel Stanley  <joel@jms.id.au>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Chromium] Make setCaretBlinkInterval static.

This should include a bug link here like you did in the other changelog in this patch.
Comment 7 David Levin 2010-02-02 16:24:17 PST
I'll land this and do the fix suggested.
Comment 8 David Levin 2010-02-03 07:40:00 PST
Committed as http://trac.webkit.org/changeset/54280