Bug 28931 – [chromium] Linux: Expose functions to set the caret blink interval

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Joel Stanley]

Chromium on Linux will use gtk-cursor-blink from GtkSettings for setting the blink interval.

[Joel Stanley]

Created attachment 38959 [details]
patch

[Joel Stanley]

Created attachment 38962 [details]
patch

[Eric Seidel (no email)]

Comment on attachment 38962 [details]
patch

RenderTheme is private to WebCore, how is WebKit ever going to use this method to set this interval?  It seems to me that we would need a different way to pumb this down into WebCore, no?

[Joel Stanley]

(In reply to comment #3)
> (From update of attachment 38962 [details])
> RenderTheme is private to WebCore, how is WebKit ever going to use this method
> to set this interval?  It seems to me that we would need a different way to
> pumb this down into WebCore, no?

I don't understand.  Is this not possible, or the wrong way to do it?

The chromium side of this patch is: http://codereview.chromium.org/186009/show

The user is in webkit/glue/webview_impl.cc:

void WebViewImpl::SetCaretBlinkInterval(double interval) {
  reinterpret_cast<RenderThemeChromiumLinux*>(theme())->
      setCaretBlinkInterval(interval);
}

[Eric Seidel (no email)]

I guess in other ports RenderTheme is not exposed to the WebKit layer.  It's not the end of the world that it's exposed here.

[Joel Stanley]

Comment on attachment 38962 [details]
patch

Can this be looked at again?

Thanks.

[Eric Seidel (no email)]

Comment on attachment 38962 [details]
patch

Rejecting patch 38962 from commit-queue.  This patch will require manual commit.

Patch https://bugs.webkit.org/attachment.cgi?id=38962 from bug 28931 failed to download and apply.

[Eric Seidel (no email)]

Applying 38962 from bug 28931.
patching file WebCore/ChangeLog
patch: **** malformed patch at line 20:          Reviewed by Darin Adler.

[Eric Seidel (no email)]

Looks like you manually edited the patch file to remove a line w/o updating the line numbers. :)

[Joel Stanley]

Yes, I did :(

Sorry about that.  Thanks for your help.

[Joel Stanley]

Do you need me to upload a fixed patch?

[Eric Seidel (no email)]

Our tools (like the commit-queue) cannot automatically handle this patch.  So either someone can land this manually, or you can upload a fixed version.  The latter is probably quicker for you.

[Joel Stanley]

Created attachment 39042 [details]
fixed patch

Here is the same patch with correct line numbers.

[Eric Seidel (no email)]

Comment on attachment 39042 [details]
fixed patch

Rejecting patch 39042 from commit-queue.  This patch will require manual commit.

['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

[Eric Seidel (no email)]

media/video-source-error.html -> timed out
Bah. You were bitten by bug 28845 I think.

[Eric Seidel (no email)]

Comment on attachment 39042 [details]
fixed patch

Bitten again!
media/video-source-error.html -> timed out

[Eric Seidel (no email)]

Comment on attachment 39042 [details]
fixed patch

Clearing flags on attachment: 39042

Committed r48094: <http://trac.webkit.org/changeset/48094>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Dimitri Glazkov (Google)]

Landing this broke a bunch of layout tests on Linux:

http://build.chromium.org/buildbot/waterfall.fyi/waterfall?builder=Webkit%20Linux%20(webkit.org)

Any idea why?

[Dimitri Glazkov (Google)]

Oy. What's the value if you never call the setter? Like in test_shell?

[Dimitri Glazkov (Google)]

Rolled out in http://trac.webkit.org/changeset/48098.

Please make sure to rely on RenderTheme::caretBlinkInterval as a default value in case setCaretBlinkInterval hasn't been called yet.

[Joel Stanley]

Created attachment 39118 [details]
fix

I should have implemented caretBlinkInterval instead of caretBlinkIntervalInternal as blinking wasn't being disabled for the layout tests (see WebCore/rendering/RenderThemeChromiumSkia.cpp 194).

There was no default value being set, like you asked.

I am using git so this patch might not be formatted the way you need.  Is there an equivalent of svn-create-patch for git?

[Joel Stanley]

Created attachment 39119 [details]
second version

[Dimitri Glazkov (Google)]

Comment on attachment 39119 [details]
second version

r=me.

[Eric Seidel (no email)]

Comment on attachment 39119 [details]
second version

Rejecting patch 39119 from commit-queue.  This patch will require manual commit.

['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

[Eric Seidel (no email)]

Comment on attachment 39119 [details]
second version

Clearing flags on attachment: 39119

Committed r48105: <http://trac.webkit.org/changeset/48105>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

The failure was:
media/video-loop.html -> failed
Which is possibly just another symptom of bug 28845.