Retrieve and pass subpixel lcd geometry on windows

Retrieve and pass subpixel lcd geometry on windows

Refactor a little to share some ui->skia converters.

BUG=468509
TEST=set HKLM\Software[\Wow6432Node]\Microsoft\Avalon.Graphics\DISPLAY1\PixelStructure to 1 or 2, zoom in and confirm RGB or BGR fringes respectively.

Committed: https://crrev.com/a04a4b3f43eecb49e144cf194577af0192d83b35
Cr-Commit-Position: refs/heads/master@{#322489}

[scottmg, 2015-03-26 00:08:26]

scottmg@chromium.org changed reviewers:
+ ananta@chromium.org, jam@chromium.org, msw@chromium.org

[scottmg, 2015-03-26 00:10:28]

ananta: windows stuff in ui/gfx/font_render_params_win.cc and
ui/gfx/platform_font_win.cc
jam: chrome/ and content/
msw: ui/gfx/

[jam, 2015-03-26 15:42:08]

lgtm

[msw, 2015-03-26 20:41:26]

https://codereview.chromium.org/1021643002/diff/40001/ui/gfx/font_render_para...
File ui/gfx/font_render_params.cc (right):

https://codereview.chromium.org/1021643002/diff/40001/ui/gfx/font_render_para...
ui/gfx/font_render_params.cc:33: default:
nit: omit default case to get compile errors instead; drop logging.h

https://codereview.chromium.org/1021643002/diff/40001/ui/gfx/font_render_para...
ui/gfx/font_render_params.cc:51: default:
nit: omit default case to get compile errors instead; drop logging.h

https://codereview.chromium.org/1021643002/diff/40001/ui/gfx/font_render_para...
File ui/gfx/font_render_params_win.cc (right):

https://codereview.chromium.org/1021643002/diff/40001/ui/gfx/font_render_para...
ui/gfx/font_render_params_win.cc:26: (L"SOFTWARE\\Microsoft\\Avalon.Graphics\\"
+ trimmed.value()).c_str(),
Hmm, I wonder why I don't have "Avalon.Graphics" in
"HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\" locally with Win7+Aero with ClearType
enabled... It's described at
https://msdn.microsoft.com/en-us/library/aa970267%28v=vs.110%29.aspx so it
should be legitimate, but I wonder if there's a newer/older/alternate key to
check. I worry about disabling subpixel rendering without this key; it seems
safer to assume RGB (as before) unless the key exists and specifies BGR.

https://codereview.chromium.org/1021643002/diff/40001/ui/gfx/font_render_para...
ui/gfx/font_render_params_win.cc:33: else if (pixel_structure == 2)
nit: no else after return.

https://codereview.chromium.org/1021643002/diff/40001/ui/gfx/font_render_para...
ui/gfx/font_render_params_win.cc:35: }
nit: can you break the loop of enumerating devices at the end of this block,
since we only handle the primary device for now?

https://codereview.chromium.org/1021643002/diff/40001/ui/gfx/font_render_para...
ui/gfx/font_render_params_win.cc:39: // ClearType is not enabled, or retrieval
failed.
nit: I have ClearType on but don't have this key... revise comment?

https://codereview.chromium.org/1021643002/diff/40001/ui/gfx/platform_font_wi...
File ui/gfx/platform_font_win.cc (left):

https://codereview.chromium.org/1021643002/diff/40001/ui/gfx/platform_font_wi...
ui/gfx/platform_font_win.cc:495: paint.setAntiAlias(!!antialiasing);
nit: should font_render_params_win.cc:36 use the !! trick?

[scottmg, 2015-03-26 21:16:17]

https://codereview.chromium.org/1021643002/diff/40001/ui/gfx/font_render_para...
File ui/gfx/font_render_params.cc (right):

https://codereview.chromium.org/1021643002/diff/40001/ui/gfx/font_render_para...
ui/gfx/font_render_params.cc:33: default:
On 2015/03/26 20:41:25, msw wrote:
> nit: omit default case to get compile errors instead; drop logging.h

I can never figure out which compiler wants what. If I remove the default:s on
Windows, I get:

[4704->175/4912 ~33] CXX obj\ui\gfx\gfx.font_render_params.obj
FAILED: ninja -t msvc -e environment.x86 --
"d:\src\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe" /nologo
/showIncludes /FC @obj\ui\gfx\gfx.font_render_params.obj.rsp /c
..\..\ui\gfx\font_render_params.cc /Foobj\ui\gfx\gfx.font_render_params.obj
/Fdobj\ui\gfx\gfx.cc.pdb
d:\src\cr3\src\ui\gfx\font_render_params.cc(32) : error C2220: warning treated
as error - no 'object' file generated
d:\src\cr3\src\ui\gfx\font_render_params.cc(47) : error C2220: warning treated
as error - no 'object' file generated
d:\src\cr3\src\ui\gfx\font_render_params.cc(32) : warning C4715:
'gfx::FontRenderParams::SubpixelRenderingToSkiaLCDOrder' : not all control paths
return a value
d:\src\cr3\src\ui\gfx\font_render_params.cc(47) : warning C4715:
'gfx::FontRenderParams::SubpixelRenderingToSkiaLCDOrientation' : not all control
paths return
a value

But I think if I just have a return after the switch, clang might complain about
unreachable code (and then I'd probably want a NOTREACHED() there anyway too).
Do you prefer this version?

https://codereview.chromium.org/1021643002/diff/40001/ui/gfx/font_render_para...
ui/gfx/font_render_params.cc:51: default:
On 2015/03/26 20:41:25, msw wrote:
> nit: omit default case to get compile errors instead; drop logging.h

As above.

https://codereview.chromium.org/1021643002/diff/40001/ui/gfx/font_render_para...
File ui/gfx/font_render_params_win.cc (right):

https://codereview.chromium.org/1021643002/diff/40001/ui/gfx/font_render_para...
ui/gfx/font_render_params_win.cc:26: (L"SOFTWARE\\Microsoft\\Avalon.Graphics\\"
+ trimmed.value()).c_str(),
On 2015/03/26 20:41:25, msw wrote:
> Hmm, I wonder why I don't have "Avalon.Graphics" in
> "HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\" locally with Win7+Aero with ClearType
> enabled... It's described at
> https://msdn.microsoft.com/en-us/library/aa970267%28v=vs.110%29.aspx so it
> should be legitimate, but I wonder if there's a newer/older/alternate key to
> check. I worry about disabling subpixel rendering without this key; it seems
> safer to assume RGB (as before) unless the key exists and specifies BGR.

Oh, good catch. I just confirmed on a clean Win7 VM that the key doesn't show up
until the cleartype tuner has been run. Switched to RGB default.

https://codereview.chromium.org/1021643002/diff/40001/ui/gfx/font_render_para...
ui/gfx/font_render_params_win.cc:33: else if (pixel_structure == 2)
On 2015/03/26 20:41:25, msw wrote:
> nit: no else after return.

Done.

https://codereview.chromium.org/1021643002/diff/40001/ui/gfx/font_render_para...
ui/gfx/font_render_params_win.cc:35: }
On 2015/03/26 20:41:25, msw wrote:
> nit: can you break the loop of enumerating devices at the end of this block,
> since we only handle the primary device for now?

Done.

https://codereview.chromium.org/1021643002/diff/40001/ui/gfx/font_render_para...
ui/gfx/font_render_params_win.cc:39: // ClearType is not enabled, or retrieval
failed.
On 2015/03/26 20:41:25, msw wrote:
> nit: I have ClearType on but don't have this key... revise comment?

Done.

https://codereview.chromium.org/1021643002/diff/40001/ui/gfx/platform_font_wi...
File ui/gfx/platform_font_win.cc (left):

https://codereview.chromium.org/1021643002/diff/40001/ui/gfx/platform_font_wi...
ui/gfx/platform_font_win.cc:495: paint.setAntiAlias(!!antialiasing);
On 2015/03/26 20:41:26, msw wrote:
> nit: should font_render_params_win.cc:36 use the !! trick?

You meant :63, right? No need, it's non-zero either way. I'm guessing there was
a c4800 warning here at some point because it was explicitly converting to a
bool.

[msw, 2015-03-26 21:56:03]

lgtm

https://codereview.chromium.org/1021643002/diff/40001/ui/gfx/font_render_para...
File ui/gfx/font_render_params.cc (right):

https://codereview.chromium.org/1021643002/diff/40001/ui/gfx/font_render_para...
ui/gfx/font_render_params.cc:33: default:
On 2015/03/26 21:16:16, scottmg wrote:
> On 2015/03/26 20:41:25, msw wrote:
> > nit: omit default case to get compile errors instead; drop logging.h
> 
> I can never figure out which compiler wants what. If I remove the default:s on
> Windows, I get:
> 
> [4704->175/4912 ~33] CXX obj\ui\gfx\gfx.font_render_params.obj
> FAILED: ninja -t msvc -e environment.x86 --
> "d:\src\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe"
/nologo
> /showIncludes /FC @obj\ui\gfx\gfx.font_render_params.obj.rsp /c
> ..\..\ui\gfx\font_render_params.cc /Foobj\ui\gfx\gfx.font_render_params.obj
> /Fdobj\ui\gfx\gfx.cc.pdb
> d:\src\cr3\src\ui\gfx\font_render_params.cc(32) : error C2220: warning treated
> as error - no 'object' file generated
> d:\src\cr3\src\ui\gfx\font_render_params.cc(47) : error C2220: warning treated
> as error - no 'object' file generated
> d:\src\cr3\src\ui\gfx\font_render_params.cc(32) : warning C4715:
> 'gfx::FontRenderParams::SubpixelRenderingToSkiaLCDOrder' : not all control
paths
> return a value
> d:\src\cr3\src\ui\gfx\font_render_params.cc(47) : warning C4715:
> 'gfx::FontRenderParams::SubpixelRenderingToSkiaLCDOrientation' : not all
control
> paths return
> a value
> 
> But I think if I just have a return after the switch, clang might complain
about
> unreachable code (and then I'd probably want a NOTREACHED() there anyway too).
> Do you prefer this version?

Yeah, I prefer the newer version, but you can go either way, it's a nit.

[scottmg, 2015-03-26 21:57:25]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2015-03-26 21:57:25]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1021643002/#ps60001
(title: "review")

[commit-bot: I haz the power, 2015-03-26 21:57:53]

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

[commit-bot: I haz the power, 2015-03-26 22:56:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-26 22:58:30]

Patchset 4 (id:??) landed as
https://crrev.com/a04a4b3f43eecb49e144cf194577af0192d83b35
Cr-Commit-Position: refs/heads/master@{#322489}