Avoid calling the GetSystemMetrics API in the renderer process on Windows to get the scrollbar metr…

Avoid calling the GetSystemMetrics API in the renderer process on Windows to get the scrollbar metrics.

It appears that this call takes a long time on certain Windows 8 configurations because the call triggers a load
of the uxtheme.dll which fails and retries every time. This is presumably due to the Win32K lockdown mode on Windows 8+.

Fix is to send the scrollbar metrics values we are interested in as part of the RendererPrefs structure. In the renderer
these are saved as static values in the WebThemeEngineImpl class.

1) Note that not all of the cache settings are currently used - SM_CYHSCROLL,
SM_CYVSCROLL, and SM_CXHSCROLL are cached for possible future usage.

2. These repeated attempts to load uxtheme.dll only occur in some (ill-defined)
situations, having to do with what theme is selected and other factors

BUG=472790
Committed: https://crrev.com/59b9fe7ebc4c814410f1ac4a0c5a7cecb125ce81
Cr-Commit-Position: refs/heads/master@{#323999}

[ananta, 2015-04-02 22:46:32]

ananta@chromium.org changed reviewers:
+ cpu@chromium.org, jam@chromium.org, jschuh@chromium.org

[jam, 2015-04-03 00:35:46]

which parts do you want each of us to look at? cpu is owner for all of them..

[ananta, 2015-04-03 00:59:00]

On 2015/04/03 00:35:46, jam wrote:
> which parts do you want each of us to look at? cpu is owner for all of them..

Please go over content as you are direct owner there.

Thanks
Ananta

[cpu_(ooo_6.6-7.5), 2015-04-03 01:06:30]

cpu@chromium.org changed reviewers:
+ brucedawson@chromium.org

[cpu_(ooo_6.6-7.5), 2015-04-03 01:06:49]

bruce, can you do a first pass reivew? thanks

[jam, 2015-04-03 15:08:20]

https://codereview.chromium.org/1054243002/diff/1/chrome/browser/renderer_pre...
File chrome/browser/renderer_preferences_util.cc (right):

https://codereview.chromium.org/1054243002/diff/1/chrome/browser/renderer_pre...
chrome/browser/renderer_preferences_util.cc:157:
gfx::win::GetSystemMetricsInDIP(SM_CXHSCROLL);
seems this should be done in content layer instead of chrome so that all content
based browsers get this. same for some of the other stuff above


the only code that should be in this method is setting preferences that need
stuff from the chrome layer (i.e. from prefs)

https://codereview.chromium.org/1054243002/diff/1/content/child/webthemeengin...
File content/child/webthemeengine_impl_default.h (right):

https://codereview.chromium.org/1054243002/diff/1/content/child/webthemeengin...
content/child/webthemeengine_impl_default.h:29: // The next 4 functions are only
invoked on Windows.
why not behind OS_WIN?

https://codereview.chromium.org/1054243002/diff/1/content/child/webthemeengin...
content/child/webthemeengine_impl_default.h:41: static void
SetHorizontalArrowBitmapWidth(int32 horizontal_arrow_bitmap_width);
nit 80 chars

https://codereview.chromium.org/1054243002/diff/1/content/child/webthemeengin...
content/child/webthemeengine_impl_default.h:45: static int32
vertical_scroll_bar_width_;
nit: i think the convention is to put these as globals in an anonymous
namespace?

https://codereview.chromium.org/1054243002/diff/1/content/public/common/rende...
File content/public/common/renderer_preferences.h (right):

https://codereview.chromium.org/1054243002/diff/1/content/public/common/rende...
content/public/common/renderer_preferences.h:158: // The width of a vertical
scroll bar in dips.
ditto if windows only

[brucedawson, 2015-04-03 17:00:24]

Suggested addition to description:

These repeated attempts to load uxtheme.dll only occur in some (ill-defined)
situations, having to do with what theme is selected and other factors.

That is, I can only repro this on 32-bit Chrome builds with the Windows theme
selected, not Chromium builds or 64-bit builds or when I have the Google theme
selected. But, I don't know which factors are actually relevant.

https://codereview.chromium.org/1054243002/diff/1/chrome/browser/renderer_pre...
File chrome/browser/renderer_preferences_util.cc (right):

https://codereview.chromium.org/1054243002/diff/1/chrome/browser/renderer_pre...
chrome/browser/renderer_preferences_util.cc:152:
prefs->horizontal_scroll_bar_height_in_dips =
When are SM_CYHSCROLL, SM_CYVSCROLL, and SM_CXHSCROLL used?

https://codereview.chromium.org/1054243002/diff/1/content/child/webthemeengin...
File content/child/webthemeengine_impl_default.cc (right):

https://codereview.chromium.org/1054243002/diff/1/content/child/webthemeengin...
content/child/webthemeengine_impl_default.cc:165: int32
WebThemeEngineImpl::horizontal_scroll_bar_height_ = 0;
I can't see anywhere where this variable (or the two *arrow_bitmap_* variables)
are read from. It looks like we only need to cache SM_CXVSCROLL, I think?

https://codereview.chromium.org/1054243002/diff/1/content/child/webthemeengin...
content/child/webthemeengine_impl_default.cc:183: if
(WebThemeEngineImpl::vertical_scroll_bar_width_) {
When would we not have the scrollbar width cached? Or should we replicate the
"if (size == 0) size = 17;" logic here (from native_theme_win.cc)?

https://codereview.chromium.org/1054243002/diff/1/content/public/common/rende...
File content/public/common/renderer_preferences.cc (right):

https://codereview.chromium.org/1054243002/diff/1/content/public/common/rende...
content/public/common/renderer_preferences.cc:50:
arrow_bitmap_width_horizontal_scroll_bar_in_dips(0) {
This would be cleaner if we initialized these when they are declared, but for
consistency I agree that we should initialize them here. Tempting to change to
initialize all in the declaration... but no.

https://codereview.chromium.org/1054243002/diff/1/content/renderer/render_vie...
File content/renderer/render_view_win.cc (right):

https://codereview.chromium.org/1054243002/diff/1/content/renderer/render_vie...
content/renderer/render_view_win.cc:45:
renderer_preferences_.arrow_bitmap_width_horizontal_scroll_bar_in_dips);
It seems clumsy having four different setter functions, four variables to store
the results, etc. A single function with four parameters would arguably be
better. A single function that takes a four-element struct would probably be
ideal - one function, one variable declaration. Using a four-element struct
instead of four functions/variables also makes extending this in the future far
simpler. But if we drop to just using SM_CXVSCROLL then this becomes irrelevant.

[ananta, 2015-04-03 19:17:04]

https://codereview.chromium.org/1054243002/diff/1/chrome/browser/renderer_pre...
File chrome/browser/renderer_preferences_util.cc (right):

https://codereview.chromium.org/1054243002/diff/1/chrome/browser/renderer_pre...
chrome/browser/renderer_preferences_util.cc:152:
prefs->horizontal_scroll_bar_height_in_dips =
On 2015/04/03 17:00:24, brucedawson wrote:
> When are SM_CYHSCROLL, SM_CYVSCROLL, and SM_CXHSCROLL used?

They are not used as of now. I added those just in case we need them in the
future.

https://codereview.chromium.org/1054243002/diff/1/chrome/browser/renderer_pre...
chrome/browser/renderer_preferences_util.cc:157:
gfx::win::GetSystemMetricsInDIP(SM_CXHSCROLL);
On 2015/04/03 15:08:19, jam wrote:
> seems this should be done in content layer instead of chrome so that all
content
> based browsers get this. same for some of the other stuff above
> 
> 
> the only code that should be in this method is setting preferences that need
> stuff from the chrome layer (i.e. from prefs)

Moved this as well as the font metrics code above to the
RenderViewHostImpl::CreateRenderView function.

https://codereview.chromium.org/1054243002/diff/1/content/child/webthemeengin...
File content/child/webthemeengine_impl_default.cc (right):

https://codereview.chromium.org/1054243002/diff/1/content/child/webthemeengin...
content/child/webthemeengine_impl_default.cc:165: int32
WebThemeEngineImpl::horizontal_scroll_bar_height_ = 0;
On 2015/04/03 17:00:24, brucedawson wrote:
> I can't see anywhere where this variable (or the two *arrow_bitmap_*
variables)
> are read from. It looks like we only need to cache SM_CXVSCROLL, I think?

Caching them on an if needed basis.

https://codereview.chromium.org/1054243002/diff/1/content/child/webthemeengin...
content/child/webthemeengine_impl_default.cc:183: if
(WebThemeEngineImpl::vertical_scroll_bar_width_) {
On 2015/04/03 17:00:24, brucedawson wrote:
> When would we not have the scrollbar width cached? Or should we replicate the
> "if (size == 0) size = 17;" logic here (from native_theme_win.cc)?

Removed the check. The metrics default to 17

https://codereview.chromium.org/1054243002/diff/1/content/child/webthemeengin...
File content/child/webthemeengine_impl_default.h (right):

https://codereview.chromium.org/1054243002/diff/1/content/child/webthemeengin...
content/child/webthemeengine_impl_default.h:29: // The next 4 functions are only
invoked on Windows.
On 2015/04/03 15:08:19, jam wrote:
> why not behind OS_WIN?

Done.

https://codereview.chromium.org/1054243002/diff/1/content/child/webthemeengin...
content/child/webthemeengine_impl_default.h:41: static void
SetHorizontalArrowBitmapWidth(int32 horizontal_arrow_bitmap_width);
On 2015/04/03 15:08:19, jam wrote:
> nit 80 chars

The four functions here have been collapsed to one function
CacheScrollBarMetrics.

https://codereview.chromium.org/1054243002/diff/1/content/child/webthemeengin...
content/child/webthemeengine_impl_default.h:45: static int32
vertical_scroll_bar_width_;
On 2015/04/03 15:08:19, jam wrote:
> nit: i think the convention is to put these as globals in an anonymous
> namespace?

Done.

https://codereview.chromium.org/1054243002/diff/1/content/public/common/rende...
File content/public/common/renderer_preferences.h (right):

https://codereview.chromium.org/1054243002/diff/1/content/public/common/rende...
content/public/common/renderer_preferences.h:158: // The width of a vertical
scroll bar in dips.
On 2015/04/03 15:08:19, jam wrote:
> ditto if windows only

Put these and the font values above under an if defined(OS_WIN) block

https://codereview.chromium.org/1054243002/diff/1/content/renderer/render_vie...
File content/renderer/render_view_win.cc (right):

https://codereview.chromium.org/1054243002/diff/1/content/renderer/render_vie...
content/renderer/render_view_win.cc:45:
renderer_preferences_.arrow_bitmap_width_horizontal_scroll_bar_in_dips);
On 2015/04/03 17:00:24, brucedawson wrote:
> It seems clumsy having four different setter functions, four variables to
store
> the results, etc. A single function with four parameters would arguably be
> better. A single function that takes a four-element struct would probably be
> ideal - one function, one variable declaration. Using a four-element struct
> instead of four functions/variables also makes extending this in the future
far
> simpler. But if we drop to just using SM_CXVSCROLL then this becomes
irrelevant.

Done.

[jam, 2015-04-03 19:26:34]

lgtm

[brucedawson, 2015-04-03 19:42:17]

lgtm, with one nit and one request/question:

It would be good to add a couple of things to the change description. One or
both of these sentences to clarify the description? Think about it.

1) Note that not all of the cache settings are currently used - SM_CYHSCROLL,
SM_CYVSCROLL, and SM_CXHSCROLL are cached for possible future usage.

2) These repeated attempts to load uxtheme.dll only occur in some (ill-defined)
situations, having to do with what theme is selected and other factors.

https://codereview.chromium.org/1054243002/diff/60001/chrome/browser/renderer...
File chrome/browser/renderer_preferences_util.cc (right):

https://codereview.chromium.org/1054243002/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_preferences_util.cc:37: #include "ui/gfx/win/dpi.h"
Why is the only addition to this file to add a #include?

[ananta, 2015-04-03 20:16:33]

Updated the patch set as per bruce's suggestion.

https://codereview.chromium.org/1054243002/diff/60001/chrome/browser/renderer...
File chrome/browser/renderer_preferences_util.cc (right):

https://codereview.chromium.org/1054243002/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_preferences_util.cc:37: #include "ui/gfx/win/dpi.h"
On 2015/04/03 19:42:17, brucedawson wrote:
> Why is the only addition to this file to add a #include?

Left over from the previous patch set. Removed. I also removed the other
includes under OS_WIN as the code has moved to render_view_host_impl.cc now.

Thanks

[jschuh, 2015-04-03 20:49:15]

ipc security lgtm. (notes: browser -> renderer integers)

[cpu_(ooo_6.6-7.5), 2015-04-06 20:22:08]

lgtm w/ nits

https://codereview.chromium.org/1054243002/diff/80001/content/child/webthemee...
File content/child/webthemeengine_impl_default.h (right):

https://codereview.chromium.org/1054243002/diff/80001/content/child/webthemee...
content/child/webthemeengine_impl_default.h:31: static void
CacheScrollBarMetrics(int32 vertical_scroll_bar_width,
looks like this file is webkit style so please change to webkit style naming, I
guess in this case cacheScrollBarMetrics

https://codereview.chromium.org/1054243002/diff/80001/content/public/common/r...
File content/public/common/renderer_preferences.cc (right):

https://codereview.chromium.org/1054243002/diff/80001/content/public/common/r...
content/public/common/renderer_preferences.cc:44: small_caption_font_height = 0;
move this back into initializer list,

[ananta, 2015-04-06 20:42:03]

https://codereview.chromium.org/1054243002/diff/80001/content/child/webthemee...
File content/child/webthemeengine_impl_default.h (right):

https://codereview.chromium.org/1054243002/diff/80001/content/child/webthemee...
content/child/webthemeengine_impl_default.h:31: static void
CacheScrollBarMetrics(int32 vertical_scroll_bar_width,
On 2015/04/06 20:22:08, cpu wrote:
> looks like this file is webkit style so please change to webkit style naming,
I
> guess in this case cacheScrollBarMetrics

Done.

https://codereview.chromium.org/1054243002/diff/80001/content/public/common/r...
File content/public/common/renderer_preferences.cc (right):

https://codereview.chromium.org/1054243002/diff/80001/content/public/common/r...
content/public/common/renderer_preferences.cc:44: small_caption_font_height = 0;
On 2015/04/06 20:22:08, cpu wrote:
> move this back into initializer list,

Done.

[ananta, 2015-04-06 20:42:43]

The CQ bit was checked by ananta@chromium.org

[ananta, 2015-04-06 20:42:43]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
brucedawson@chromium.org, cpu@chromium.org, jschuh@chromium.org
Link to the patchset: https://codereview.chromium.org/1054243002/#ps100001
(title: "Address cpu review comments")

[commit-bot: I haz the power, 2015-04-06 20:43:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1054243002/100001

[commit-bot: I haz the power, 2015-04-06 20:53:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-06 20:53:45]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ananta, 2015-04-06 21:57:56]

The CQ bit was checked by ananta@chromium.org

[ananta, 2015-04-06 21:57:57]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org,
jam@chromium.org, jschuh@chromium.org, brucedawson@chromium.org
Link to the patchset: https://codereview.chromium.org/1054243002/#ps120001
(title: "Fix presubmit")

[commit-bot: I haz the power, 2015-04-06 21:58:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1054243002/120001

[commit-bot: I haz the power, 2015-04-06 22:12:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-06 22:12:57]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[ananta, 2015-04-06 22:40:47]

The CQ bit was checked by ananta@chromium.org

[ananta, 2015-04-06 22:40:47]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org,
jam@chromium.org, jschuh@chromium.org, brucedawson@chromium.org
Link to the patchset: https://codereview.chromium.org/1054243002/#ps140001
(title: "Fix build error")

[commit-bot: I haz the power, 2015-04-06 22:40:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1054243002/140001

[commit-bot: I haz the power, 2015-04-06 22:56:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-06 22:56:48]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[ananta, 2015-04-06 23:36:39]

The CQ bit was checked by ananta@chromium.org

[ananta, 2015-04-06 23:36:39]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org,
jam@chromium.org, jschuh@chromium.org, brucedawson@chromium.org
Link to the patchset: https://codereview.chromium.org/1054243002/#ps160001
(title: "Fix build error")

[commit-bot: I haz the power, 2015-04-06 23:37:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1054243002/160001

[ananta, 2015-04-06 23:45:59]

The CQ bit was unchecked by ananta@chromium.org

[ananta, 2015-04-06 23:49:32]

The CQ bit was checked by ananta@chromium.org

[ananta, 2015-04-06 23:49:33]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org,
jam@chromium.org, jschuh@chromium.org, brucedawson@chromium.org
Link to the patchset: https://codereview.chromium.org/1054243002/#ps180001
(title: "Fix build error")

[commit-bot: I haz the power, 2015-04-06 23:49:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1054243002/180001

[commit-bot: I haz the power, 2015-04-07 00:09:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-07 00:09:47]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[ananta, 2015-04-07 00:38:07]

The CQ bit was checked by ananta@chromium.org

[ananta, 2015-04-07 00:38:08]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org,
jam@chromium.org, jschuh@chromium.org, brucedawson@chromium.org
Link to the patchset: https://codereview.chromium.org/1054243002/#ps200001
(title: "Fix build error in view_messages")

[commit-bot: I haz the power, 2015-04-07 00:38:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1054243002/200001

[commit-bot: I haz the power, 2015-04-07 01:33:42]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-04-07 01:34:29]

Patchset 11 (id:??) landed as
https://crrev.com/59b9fe7ebc4c814410f1ac4a0c5a7cecb125ce81
Cr-Commit-Position: refs/heads/master@{#323999}