Fix the size of scrollbars when using the Windows Classic theme.

Fix the size of scrollbars when using the Windows Classic theme.

BUG=175335
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=183990

[kevers, 2013-02-20 19:49:03]

Hi Scott,

Can you please have a look at this CL.  Should address perf regression that
bounced https://codereview.chromium.org/12256011/.

[sky, 2013-02-20 22:22:46]

https://codereview.chromium.org/12330018/diff/1/ui/native_theme/native_theme_...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/12330018/diff/1/ui/native_theme/native_theme_...
ui/native_theme/native_theme_win.cc:77:
static_cast<int>(GetSystemMetrics(SM_CXVSCROLL) /
Can't GetSystemMetrics change?

[kevers, 2013-02-20 22:37:22]

https://codereview.chromium.org/12330018/diff/1/ui/native_theme/native_theme_...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/12330018/diff/1/ui/native_theme/native_theme_...
ui/native_theme/native_theme_win.cc:77:
static_cast<int>(GetSystemMetrics(SM_CXVSCROLL) /
On 2013/02/20 22:22:47, sky wrote:
> Can't GetSystemMetrics change?

The system metric could change if you update your theme while running chrome;
however, checking the system metric for each part size call would reintroduce
the perf regression.

A bit of background...  WebKit used to pick up the scrollbar with a single call
to GetSystemMetrics and cache the value.  The problem is that the system metrics
is in pixels and WebKit in DIP.  Thus, we got fat scrollbars in Windows when
using high-DPI mode.  This fix addresses the fat scrollbars problem, while
retaining much of the efficiency that we had earlier in the WebKit
implementation by using a cached value.  Seems like a reasonable compromise.

[sky, 2013-02-20 22:58:59]

https://codereview.chromium.org/12330018/diff/1/ui/native_theme/native_theme_...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/12330018/diff/1/ui/native_theme/native_theme_...
ui/native_theme/native_theme_win.cc:77:
static_cast<int>(GetSystemMetrics(SM_CXVSCROLL) /
On 2013/02/20 22:37:22, kevers wrote:
> On 2013/02/20 22:22:47, sky wrote:
> > Can't GetSystemMetrics change?
> 
> The system metric could change if you update your theme while running chrome;
> however, checking the system metric for each part size call would reintroduce
> the perf regression.
> 
> A bit of background...  WebKit used to pick up the scrollbar with a single
call
> to GetSystemMetrics and cache the value.  The problem is that the system
metrics
> is in pixels and WebKit in DIP.  Thus, we got fat scrollbars in Windows when
> using high-DPI mode.  This fix addresses the fat scrollbars problem, while
> retaining much of the efficiency that we had earlier in the WebKit
> implementation by using a cached value.  Seems like a reasonable compromise.

When that happens don't we get a WM_SYSCOLORCHANGE? You should make this a
member and update it when the system colors changes.

[kevers, 2013-02-21 16:25:33]

On further testing, it became apparent that the previous fix of caching metrics
did not address the performance on Windows Classic.  Changed to an alternative
strategy of simply validating the part size before passing it on to WebKit. 
Paint performance on Windows Classic is greatly improved.

[tony, 2013-02-21 18:46:26]

https://codereview.chromium.org/12330018/diff/4002/webkit/glue/webthemeengine...
File webkit/glue/webthemeengine_impl_win.cc (right):

https://codereview.chromium.org/12330018/diff/4002/webkit/glue/webthemeengine...
webkit/glue/webthemeengine_impl_win.cc:996: // Part size retrieval can fail with
the Windows Classic theme.
Does it always fail when using the Windows Classic theme?  It would be nice if
this comment explained why this happens.

https://codereview.chromium.org/12330018/diff/4002/webkit/glue/webthemeengine...
webkit/glue/webthemeengine_impl_win.cc:997: if(size.width() == 0) {
Nit: Space missing after "if".

[kevers, 2013-02-21 19:12:23]

https://codereview.chromium.org/12330018/diff/4002/webkit/glue/webthemeengine...
File webkit/glue/webthemeengine_impl_win.cc (right):

https://codereview.chromium.org/12330018/diff/4002/webkit/glue/webthemeengine...
webkit/glue/webthemeengine_impl_win.cc:996: // Part size retrieval can fail with
the Windows Classic theme.
On 2013/02/21 18:46:26, tony wrote:
> Does it always fail when using the Windows Classic theme?  It would be nice if
> this comment explained why this happens.

Updated comment.  Yes, the size of a scrollbar button is always returned as (0,
0) if not using a themed style and Windows Classic theme falls into this
category.

https://codereview.chromium.org/12330018/diff/4002/webkit/glue/webthemeengine...
webkit/glue/webthemeengine_impl_win.cc:997: if(size.width() == 0) {
On 2013/02/21 18:46:26, tony wrote:
> Nit: Space missing after "if".

Fixed.

[tony, 2013-02-21 19:45:55]

LGTM, it would be nice to update the change description to explain what is
happening.  Specifically, it sounds like this fixes the size of scrollbars when
using the Windows Classic theme.

[commit-bot: I haz the power, 2013-02-21 20:14:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kevers@chromium.org/12330018/11001

[commit-bot: I haz the power, 2013-02-21 22:23:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kevers@chromium.org/12330018/11001

[commit-bot: I haz the power, 2013-02-22 02:37:26]

Change committed as 183990