Use Skia for font selection on windows

Use Skia for font selection on windows

Changes windows to use Skia (instead of GDI directly) for font selection and fallback handling on windows. Only applies if the GDI_FONTS_ON_WINDOWS flag is unset.

BUG=249099
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156339

[eae, 2013-07-31 21:07:44]

This is working in progress and obviously not ready for review yet.

It currently compiles and runs but text rendering is pretty much completely
broken.

[eae, 2013-08-15 21:52:08]

Ready for review. There are still some issues with the text size for complext
text (if the flag is unset) but other than that it passes most tests.

[bungeman-chromium, 2013-08-15 22:24:44]

https://codereview.chromium.org/21353004/diff/16001/Source/core/platform/grap...
File Source/core/platform/graphics/chromium/FontPlatformDataChromiumWin.cpp
(right):

https://codereview.chromium.org/21353004/diff/16001/Source/core/platform/grap...
Source/core/platform/graphics/chromium/FontPlatformDataChromiumWin.cpp:197:
HFONT hFont = CreateFontIndirect(&logFont);
When Skia hands back the logFont here, I don't think it really knows what size
anything should be, so I think you get back some 'canonical' size (I think this
is currently -2048 or something). Given the way the Blink is currently using
Skia, I think you could get away with setting the logFont.lfHeight to -textSize.

https://codereview.chromium.org/21353004/diff/16001/Source/core/platform/grap...
File Source/core/platform/graphics/skia/FontCacheSkiaWin.cpp (right):

https://codereview.chromium.org/21353004/diff/16001/Source/core/platform/grap...
Source/core/platform/graphics/skia/FontCacheSkiaWin.cpp:158:
actualFamilies->unref();
Hmmm... I think if I put a deref on this you could use a RefPtr to unref it, I
think. It was designed with SkAutoTUnref in mind, but I'm not sure that's
supposed to be publicly usable or not.

Also, all Skia SkStrings are in UTF-8. I'm not familiar with AtomicString, but
it looks like it should have enough overrides of equalIgnoringCase for this to
work.

[eae, 2013-08-15 22:34:49]

https://codereview.chromium.org/21353004/diff/16001/Source/core/platform/grap...
File Source/core/platform/graphics/chromium/FontPlatformDataChromiumWin.cpp
(right):

https://codereview.chromium.org/21353004/diff/16001/Source/core/platform/grap...
Source/core/platform/graphics/chromium/FontPlatformDataChromiumWin.cpp:197:
HFONT hFont = CreateFontIndirect(&logFont);
On 2013/08/15 22:24:44, bungeman2 wrote:
> When Skia hands back the logFont here, I don't think it really knows what size
> anything should be, so I think you get back some 'canonical' size (I think
this
> is currently -2048 or something). Given the way the Blink is currently using
> Skia, I think you could get away with setting the logFont.lfHeight to
-textSize.

Ah, that likely explains my font-sizing problems. Thanks!

https://codereview.chromium.org/21353004/diff/16001/Source/core/platform/grap...
File Source/core/platform/graphics/skia/FontCacheSkiaWin.cpp (right):

https://codereview.chromium.org/21353004/diff/16001/Source/core/platform/grap...
Source/core/platform/graphics/skia/FontCacheSkiaWin.cpp:158:
actualFamilies->unref();
On 2013/08/15 22:24:44, bungeman2 wrote:
> Hmmm... I think if I put a deref on this you could use a RefPtr to unref it, I
> think. It was designed with SkAutoTUnref in mind, but I'm not sure that's
> supposed to be publicly usable or not.

Might make sense long term but for now calling unref manually is fine and
consistent with existing APIs.

> Also, all Skia SkStrings are in UTF-8. I'm not familiar with AtomicString, but
> it looks like it should have enough overrides of equalIgnoringCase for this to
> work.

As long as both are UTF-8 that should not be a problem, I suppose the requested
family could be 8859-1 though...

[eae, 2013-08-19 19:41:45]

Please take another look. As we talked about earlier your suggestion to set the
lfHeight did the trick.

[bungeman-skia, 2013-08-19 19:44:28]

lgtm

[bungeman-chromium, 2013-08-19 19:46:28]

lgtm, but note that my lgtm probably doesn't carry real weight (I think I'm
provisional or something).

[eae, 2013-08-19 19:47:25]

On 2013/08/19 19:46:28, bungeman2 wrote:
> lgtm, but note that my lgtm probably doesn't carry real weight (I think I'm
> provisional or something).

Thanks! I'll get someone else to rubber stamp it as needed but you know this
code better than most.

[commit-bot: I haz the power, 2013-08-19 20:40:05]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[pdr., 2013-08-19 20:40:18]

On 2013/08/19 20:40:05, I haz the power (commit-bot) wrote:
> No LGTM from a valid reviewer yet. Only full committers are accepted.
> Even if an LGTM may have been provided, it was from a non-committer or
> a lowly provisional committer, _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

LGTM

[commit-bot: I haz the power, 2013-08-19 20:42:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eae@chromium.org/21353004/29001

[commit-bot: I haz the power, 2013-08-19 22:04:47]

Change committed as 156339