On Linux, pick the color of the slider's thumbpart from the GTK theme....

linux: theme scrollbars from GTK theme

Pick the color of the slider's thumbpart and rail from the GTK theme.
We cannot match the exact visual appearance of the GTK theme, as
rendering engines can make arbitrary changes to the actual visual
appearance. But by sampling a representative set of pixels, we ensure
that we will at least match the general color scheme.

BUG=10949

[Markus (顧孟勤), 2009-11-18 01:54:30]

This change works great for me and the scroll bar is now much more visible than
before. But there are a couple of open questions:

 - focus_ring_color() is the best fit that I could find for the scrollbar color.
But it is not actually the exact same color that the system theme uses. For most
of the themes that I tested, it looks great. But for some of the more unusual
themes, it is very different from the color that the system uses. Does anybody
know whether I can actually query the "real" scrollbar color. Or is this good
enough?

 - the scrollbar does not change color on the fly, if the system theme gets
changed. I think, this is probably OK.

 - Adam, you mentioned there is something else I need to do to make WebKit happy
after making this change. Let me know what that is.

[Evan Martin, 2009-11-18 02:02:00]

You might have to create a scroll bar widget (offscreen) then query it for the
slider color.


 
http://library.gnome.org/devel/gtk/unstable/GtkWidget.html#gtk-widget-get-style

But note the color will still be wrong for themes that just use images (maybe
none exist).

[agl, 2009-11-18 02:04:22]

I'll let someone who knows GTK comment on how to get the scrollbar colour.

The 'WebKit thing' you need to do is to rebaseline all the layout tests that
include scrollbars :)

http://codereview.chromium.org/400027/diff/3001/4001
File
third_party/WebKit/WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp
(right):

http://codereview.chromium.org/400027/diff/3001/4001#newcode137
Line 137: hovered ?  0.100 : 0.100,
Both cases are the same value here.

http://codereview.chromium.org/400027/diff/3001/4001#newcode149
Line 149: hovered ?  0.100 : 0.100,
Both cases are the same value here.

[agl, 2009-11-18 02:04:58]

Oh, and this change needs to be WebKit change, of course. (Ask Evan about
uploading to Bugzilla etc.)

[Markus (顧孟勤), 2009-11-25 01:00:48]

Here is a new version of the scrollbar-theming patch. Please take a look at it
and let me know if this makes more sense.

We now get the colors from sampling the actual pixels that the native GTK theme
would render. By definition, this won't match any of the fancy theming that a
theming engine can do (some themes are _very_ elaborate), but at least we now
appear to do a pretty good job at matching the system colors.

I couldn't figure out how to disable theming when we don't use the GTK theme.
So, for now, I always use the GTK colors. See the TODO() in the source for more
details.

I am sure there is a solution to this problem, I just don't know that part of
the code well enough, and it might require some refactoring, that I didn't feel
comfortable doing.

[Evan Martin, 2009-11-25 01:10:58]

+erg, gtk theme master (I believe if you're in the gtk theme provider then you
know you're in gtk mode)

[Evan Stade, 2009-11-25 01:15:07]

UseGtkTheme()

[Markus (顧孟勤), 2009-11-25 01:19:06]

On Tue, Nov 24, 2009 at 17:15, <estade@chromium.org> wrote:

> UseGtkTheme()


But that's not a static method. So, I need a reference to the theme provider
in order to call it. And at the place where I need to obtain the scrollbar
colors, I don't see how I can get this reference.

As I said, I am probably missing something and this can be restructured in
some way. I just don't know the code very well.


Markus

[Evan Stade, 2009-11-25 01:24:17]

as you can see with the focus ring color, I just ignored whether we are in gtk
mode and always used it. If you don't want to do this with the scrollbars, I
guess you will have to pass an extra bool into the gtk_util method. From the
call site in the TabContentsView you can get the theme provider easily enough:
GtkThemeProvider::GetFrom(tab_contents()->profile())

[Markus (顧孟勤), 2009-11-25 02:15:37]

Thanks for that tip. I uploaded a new version of the code with these changes
applied.

[Markus (顧孟勤), 2009-12-01 03:07:39]

I made a whole bunch of changes and followed Evan's advice to try to move the
code into Linux-specific changes.

Let me know, if this looks better.

I am particularly uncertain whether the change to WebCore.gypi is correct and
sufficient.

I also don't know if there are any other things that I need to change, as this
changelist is almost certainly going to subtly change the pixel values from what
we had before.

[Elliot Glaysher, 2009-12-01 18:41:40]

I am not sure if this patch works; I patched it in to my checkout, and rebuilt.
At least with Darklooks and a few other themes, I didn't see a change in
appearance.

[Evan Martin, 2009-12-01 18:51:36]

I had written review comments before but I never sent them.  :(

Here they are on your new patch.

http://codereview.chromium.org/400027/diff/17007/18010
File chrome/browser/gtk/gtk_theme_provider.cc (right):

http://codereview.chromium.org/400027/diff/17007/18010#newcode228
chrome/browser/gtk/gtk_theme_provider.cc:228: GdkPixbuf* pb      =
gdk_pixbuf_new_from_data(data, GDK_COLORSPACE_RGB,
Why all the extra space before the equals sign?

http://codereview.chromium.org/400027/diff/17007/18010#newcode237
chrome/browser/gtk/gtk_theme_provider.cc:237: components[c] += data[3*(kWidth/3
+ y*kWidth) + c];
Can you add a comment to this?  TOok me a while to realize the 3 on the left was
for RGB while the 3 in the middle is for going a third of the way across the
widget.

http://codereview.chromium.org/400027/diff/17007/18010#newcode240
chrome/browser/gtk/gtk_theme_provider.cc:240: GdkColor* color    = i == 0 ?
thumb_active_color :
Why all the extra space before the equals sign?

http://codereview.chromium.org/400027/diff/17007/18010#newcode246
chrome/browser/gtk/gtk_theme_provider.cc:246: color->blue        = components[2]
* 65535 / (255*(kHeight-4));
How does kHeight factor into the color?  Super confused.

http://codereview.chromium.org/400027/diff/17007/18009
File chrome/browser/gtk/gtk_theme_provider.h (right):

http://codereview.chromium.org/400027/diff/17007/18009#newcode69
chrome/browser/gtk/gtk_theme_provider.h:69: // scrollbars.
The first sentence is missing a verb.

http://codereview.chromium.org/400027/diff/17007/18014
File chrome/common/renderer_preferences.h (right):

http://codereview.chromium.org/400027/diff/17007/18014#newcode54
chrome/common/renderer_preferences.h:54: // The color of different parts of the
scrollbar. Currently on used on Linux.
s/on/only/

http://codereview.chromium.org/400027/diff/17007/18017
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/400027/diff/17007/18017#newcode3202
chrome/renderer/render_view.cc:3202:
WebCore::ScrollbarThemeChromiumLinuxColors::setScrollbarColors(
Really sorry to keep going around in circles on this.  Is there a reason we
can't match the way the focus ring works?  Is there a reason we need a new
namespace just for the colors of the scrollbar?

http://codereview.chromium.org/400027/diff/17007/18019
File
third_party/WebKit/WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp
(right):

http://codereview.chromium.org/400027/diff/17007/18019#newcode104
third_party/WebKit/WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:104:
// renderer. But even than, some themes would just break.
s/than/then/

[Markus (顧孟勤), 2009-12-01 22:37:54]

A change in themes does not immediately take effect. You need to open a new tab
to see. If that's a big problem, I can probably figure out how to propagate the
change notification and how to make it redraw the scrollbars. But that seemed
not particularly important -- you let me know, if you think otherwise.

Apart from that complication, Darklooks seems somewhat broken on Hardy. I
selected it as my default theme, and every so often it just "disappears" and
falls back on the default GTK theme. This affects not only Chrome, but several
other GTK applications, too. So, I think, I can ignore this issue. In testing
with the default Gnome themes, everything appears to work correctly.

Also, testing on Karmic does not exhibit this particular problem. Darklooks
appears to work correctly on Karmic.

[Elliot Glaysher, 2009-12-01 22:45:08]

On 2009/12/01 22:37:54, Markus (顧孟勤) wrote:
> A change in themes does not immediately take effect.

I'd say it's fairly important since every other GTK interface element changes
theme on a global theme change signal. You'll note that gtk_theme_provider
changes them into internal BROWSER_THEME_CHANGE notifications. You could observe
that notification in TabContentsViewGtk::Observe() and resend the render prefs
on said change (which will have to happen anyway for selection colors, once I
get around to it).

> Apart from that complication, Darklooks seems somewhat broken on Hardy.

Known issue that ubuntu choose not to backport. Workaround: Select Clearlooks,
then immediately select Darklooks.

[Markus (顧孟勤), 2009-12-02 01:09:18]

Here we go again :-) Now with Tony's suggestion on where to store the color
value.

Please take another look

[Evan Martin, 2009-12-02 01:55:08]

review comments on webkit bits only, this looks like it's ready to go

http://codereview.chromium.org/400027/diff/21001/21012
File third_party/WebKit/WebCore/rendering/RenderThemeChromiumLinux.cpp (right):

http://codereview.chromium.org/400027/diff/21001/21012#newcode132
third_party/WebKit/WebCore/rendering/RenderThemeChromiumLinux.cpp:132:
theme_thumb_inactive_color = i;
4 space tabs, no single-letter vars

http://codereview.chromium.org/400027/diff/21001/21013
File third_party/WebKit/WebCore/rendering/RenderThemeChromiumLinux.h (right):

http://codereview.chromium.org/400027/diff/21001/21013#newcode57
third_party/WebKit/WebCore/rendering/RenderThemeChromiumLinux.h:57: static void
setScrollbarColors(unsigned i, unsigned a, unsigned t);
I think webkit (and I know Google) frowns on one-char variable names.

http://codereview.chromium.org/400027/diff/21001/21013#newcode58
third_party/WebKit/WebCore/rendering/RenderThemeChromiumLinux.h:58: static
unsigned thumb_inactive_color() { return theme_thumb_inactive_color; }
thumbInactiveColor

http://codereview.chromium.org/400027/diff/21001/21013#newcode71
third_party/WebKit/WebCore/rendering/RenderThemeChromiumLinux.h:71: static
unsigned theme_thumb_inactive_color;
m_themeThumbInactiveColor

http://codereview.chromium.org/400027/diff/21001/21015
File third_party/WebKit/WebKit/chromium/src/WebViewImpl.cpp (right):

http://codereview.chromium.org/400027/diff/21001/21015#newcode1634
third_party/WebKit/WebKit/chromium/src/WebViewImpl.cpp:1634: void
WebViewImpl::setScrollbarColors(unsigned i, unsigned a, unsigned t) {
single letter

http://codereview.chromium.org/400027/diff/21001/21016
File third_party/WebKit/WebKit/chromium/src/WebViewImpl.h (right):

http://codereview.chromium.org/400027/diff/21001/21016#newcode157
third_party/WebKit/WebKit/chromium/src/WebViewImpl.h:157: virtual void
setScrollbarColors(unsigned i, unsigned a, unsigned t);
no single-letter var names

[Evan Martin, 2009-12-02 01:57:50]

The chrome side looks good to me, but Elliot should probably check.  I will work
on landing the WebKit side of this.

[Markus (顧孟勤), 2009-12-02 02:23:30]

I believe I addressed all of these comments, now. Thanks

[Markus (顧孟勤), 2009-12-02 03:51:46]

On 2009/12/01 22:45:08, Elliot Glaysher wrote:
> Known issue that ubuntu choose not to backport. Workaround: Select Clearlooks,
> then immediately select Darklooks.

That's really sad. A quick web search shows that it is a one-line change to fix
a typo in a configuration file:

  sudo sed -i -e 's/tips_/tip_/g' /usr/share/themes/Darklooks/gtk-2.0/gtkrc

Afterwards, everything appears to work correctly.

[Elliot Glaysher, 2009-12-02 18:17:56]

I haven't tested this version locally and it doesn't look like you implemented
anything that responds to theme change notifications. If that's correct, please
file a bug against me to fix that.

LGTM with that bug filed.

[Markus (顧孟勤), 2009-12-02 18:29:47]

http://code.google.com/p/chromium/issues/detail?id=29197

[Markus (顧孟勤), 2009-12-08 02:07:17]

I updated the code so that it would work with the newly changed preferences
mechanism.

[Evan Stade, 2009-12-08 02:10:52]

things in common/ can't depend on things in browser/

I think the best thing to do is move the platform_util function into browser/. I
am not sure if there is already a file it could go in or if we need to create a
new one. If you want, I can figure it out, since I created this mess.

[Markus (顧孟勤), 2009-12-08 02:19:27]

Yes, I'd love to hand this changelist over to you. I updated it, because I want
to make sure that it doesn't get stale in the time that it takes Evan to get the
WebKit changes submitted.

But in general, I am clearly lacking some of the bigger picture with regards to
how the preferences code works. So, if I can persuade you to actually finalize
the changelist when we get the go-ahead from Evan, that would be great.

I have been running DarkLooks for a few days now, and the themed scrollbars are
so much prettier than what we did before. Now, I am bummed they didn't make the
cut-off for the beta.

[Evan Stade, 2009-12-08 02:28:41]

On Mon, Dec 7, 2009 at 6:19 PM, <markus@chromium.org> wrote:

> Yes, I'd love to hand this changelist over to you. I updated it, because I
> want
> to make sure that it doesn't get stale in the time that it takes Evan to
> get the
> WebKit changes submitted.
>
> But in general, I am clearly lacking some of the bigger picture with
> regards to
> how the preferences code works. So, if I can persuade you to actually
> finalize
> the changelist when we get the go-ahead from Evan, that would be great.
>
> I have been running DarkLooks for a few days now, and the themed scrollbars
> are
> so much prettier than what we did before. Now, I am bummed they didn't make
> the
> cut-off for the beta.
>
>
> http://codereview.chromium.org/400027
>

I think Evan is on vacation, no? Do you know when he'll be back? I was just
going to let Elliot review it since he's the next one who'll have to deal
with it :)

[Markus (顧孟勤), 2009-12-08 03:11:41]

Adam did his magic, and claims the WebKit changes will be in our tree sometime
tomorrow :-)

[agl, 2009-12-08 03:13:25]

On Mon, Dec 7, 2009 at 7:11 PM,  <markus@chromium.org> wrote:
> Adam did his magic, and claims the WebKit changes will be in our tree
> sometime
> tomorrow :-)

They have landed upstream: http://trac.webkit.org/changeset/51827

It's just a question of the gardener rolling them in now.


AGL

[Evan Martin, 2009-12-08 15:20:01]

On Mon, Dec 7, 2009 at 7:13 PM, Adam Langley <agl@chromium.org> wrote:
> On Mon, Dec 7, 2009 at 7:11 PM,  <markus@chromium.org> wrote:
>> Adam did his magic, and claims the WebKit changes will be in our tree
>> sometime
>> tomorrow :-)
>
> They have landed upstream: http://trac.webkit.org/changeset/51827
>
> It's just a question of the gardener rolling them in now.

I wonder if this is gonna break all our current scrollbar baselines.  :(

[Elliot Glaysher, 2009-12-08 17:49:28]

If the default values for the scrollbar colors are the same, it
shouldn't. The test shell shouldn't be touching this.

-- Elliot

On Tue, Dec 8, 2009 at 7:19 AM, Evan Martin <evan@chromium.org> wrote:
> On Mon, Dec 7, 2009 at 7:13 PM, Adam Langley <agl@chromium.org> wrote:
>> On Mon, Dec 7, 2009 at 7:11 PM,  <markus@chromium.org> wrote:
>>> Adam did his magic, and claims the WebKit changes will be in our tree
>>> sometime
>>> tomorrow :-)
>>
>> They have landed upstream: http://trac.webkit.org/changeset/51827
>>
>> It's just a question of the gardener rolling them in now.
>
> I wonder if this is gonna break all our current scrollbar baselines.  :(
>

[Markus (顧孟勤), 2009-12-08 17:54:52]

Hmm, I suspect the default colors will be very similar but subtly different. I
don't think they will be sufficiently different for our users to notice, but a
pixel-by-pixel comparison will in fact notice.

Trying to make the values exactly the same is going to result in rather ugly
special-cased code. We could add it, but it is going to be awkward.


Markus

[Elliot Glaysher, 2009-12-08 21:45:32]

BTW, if you're waiting for an LGTM from me, go for it. I realized I never really
gave an explicit one...