Bug 28931

Summary: [chromium] Linux: Expose functions to set the caret blink interval
Product: WebKit Reporter: Joel Stanley <joel>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eric, fishd, levin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
patch
none
patch
levin: review+, eric: commit-queue-
fixed patch
none
fix
none
second version none

Joel Stanley
Reported 2009-09-02 18:30:55 PDT
Chromium on Linux will use gtk-cursor-blink from GtkSettings for setting the blink interval.
Attachments
patch (2.15 KB, patch)
2009-09-02 18:32 PDT, Joel Stanley 		no flags
DetailsFormatted DiffDiff
patch (2.20 KB, patch)
2009-09-02 19:14 PDT, Joel Stanley 		levin: review+
eric: commit-queue-
DetailsFormatted DiffDiff
fixed patch (2.20 KB, patch)
2009-09-04 00:15 PDT, Joel Stanley 		no flags
DetailsFormatted DiffDiff
fix (2.38 KB, patch)
2009-09-05 21:19 PDT, Joel Stanley 		no flags
DetailsFormatted DiffDiff
second version (2.74 KB, patch)
2009-09-05 22:15 PDT, Joel Stanley 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Joel Stanley
Comment 1 2009-09-02 18:32:26 PDT
Created attachment 38959 [details] patch
Joel Stanley
Comment 2 2009-09-02 19:14:19 PDT
Created attachment 38962 [details] patch
Eric Seidel (no email)
Comment 3 2009-09-03 00:08:32 PDT
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
Comment 4 2009-09-03 00:18:16 PDT
(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)
Comment 5 2009-09-03 02:15:56 PDT
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 6 2009-09-03 21:16:49 PDT
Comment on attachment 38962 [details] patch Can this be looked at again? Thanks.
Eric Seidel (no email)
Comment 7 2009-09-03 21:50:11 PDT
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)
Comment 8 2009-09-04 00:01:11 PDT
Applying 38962 from bug 28931. patching file WebCore/ChangeLog patch: **** malformed patch at line 20: Reviewed by Darin Adler.
Eric Seidel (no email)
Comment 9 2009-09-04 00:02:43 PDT
Looks like you manually edited the patch file to remove a line w/o updating the line numbers. :)
Joel Stanley
Comment 10 2009-09-04 00:07:34 PDT
Yes, I did :( Sorry about that. Thanks for your help.
Joel Stanley
Comment 11 2009-09-04 00:08:43 PDT
Do you need me to upload a fixed patch?
Eric Seidel (no email)
Comment 12 2009-09-04 00:09:47 PDT
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
Comment 13 2009-09-04 00:15:05 PDT
Created attachment 39042 [details] fixed patch Here is the same patch with correct line numbers.
Eric Seidel (no email)
Comment 14 2009-09-04 00:59:39 PDT
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)
Comment 15 2009-09-04 03:21:05 PDT
media/video-source-error.html -> timed out Bah. You were bitten by bug 28845 I think.
Eric Seidel (no email)
Comment 16 2009-09-05 01:24:23 PDT
Comment on attachment 39042 [details] fixed patch Bitten again! media/video-source-error.html -> timed out
Eric Seidel (no email)
Comment 17 2009-09-05 02:06:04 PDT
Comment on attachment 39042 [details] fixed patch Clearing flags on attachment: 39042 Committed r48094: <http://trac.webkit.org/changeset/48094>
Eric Seidel (no email)
Comment 18 2009-09-05 02:06:09 PDT
All reviewed patches have been landed. Closing bug.
Dimitri Glazkov (Google)
Comment 19 2009-09-05 16:06:45 PDT
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)
Comment 20 2009-09-05 18:58:45 PDT
Oy. What's the value if you never call the setter? Like in test_shell?
Dimitri Glazkov (Google)
Comment 21 2009-09-05 21:10:40 PDT
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
Comment 22 2009-09-05 21:19:44 PDT
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
Comment 23 2009-09-05 22:15:46 PDT
Created attachment 39119 [details] second version
Dimitri Glazkov (Google)
Comment 24 2009-09-06 18:56:15 PDT
Comment on attachment 39119 [details] second version r=me.
Eric Seidel (no email)
Comment 25 2009-09-06 19:08:09 PDT
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 26 2009-09-06 21:15:45 PDT
Comment on attachment 39119 [details] second version Clearing flags on attachment: 39119 Committed r48105: <http://trac.webkit.org/changeset/48105>
Eric Seidel (no email)
Comment 27 2009-09-06 21:15:49 PDT
All reviewed patches have been landed. Closing bug.
Eric Seidel (no email)
Comment 28 2009-09-07 00:12:05 PDT
The failure was: media/video-loop.html -> failed Which is possibly just another symptom of bug 28845.
Note You need to log in before you can comment on or make changes to this bug.