Use DirectWrite to populate list of avalible fonts

Use DirectWrite to populate list of avalible fonts

Use the DirectWrite API to populate the list of avalible fonts in
settings when DirectWrite is enabled. False back on GDI on failure.
As GDI and DirectWrite handle font family variants and alternate names
differently this ensures that all fonts presented in the list are valid
options that can be rendered.

R=scottmg@chromium.org, cevans@chromium.org
BUG=367702, 373494
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280719

[eae, 2014-06-26 19:46:39]

This is an early draft of a patch to use DW to populate the font list in chrome
settings. While not ready for detailed review I'd like to get high-level
comments on whether this is the right approach or not.

[scottmg, 2014-06-26 20:13:39]

Maybe it could live in skia/ext and potentially use some of the Sk objects then?

Otherwise, generally seems fine.

+cpu fyi since changing stuff nearby.

https://codereview.chromium.org/354023007/diff/1/content/common/font_list_win.cc
File content/common/font_list_win.cc (right):

https://codereview.chromium.org/354023007/diff/1/content/common/font_list_win...
content/common/font_list_win.cc:59: static inline void AddLocalizedFontFamily(
nit; no 'inline's necessary

https://codereview.chromium.org/354023007/diff/1/content/common/font_list_win...
content/common/font_list_win.cc:111: AddLocalizedFontFamily(font_list,
font_family, user_locale);
I'm not sure how this changes the actual names returned -- will we need to
update various .grd translations of font names along with this?

https://codereview.chromium.org/354023007/diff/1/content/common/font_list_win...
content/common/font_list_win.cc:126: scoped_ptr<base::ListValue>
GetFontList_SlowBlocking() {
I'm a little unclear on where this happens. If it's in the browser (I assume?),
then we shouldn't be using the content/public/renderer code.

https://codereview.chromium.org/354023007/diff/1/content/public/renderer/rend...
File content/public/renderer/render_font_warmup_win.cc (right):

https://codereview.chromium.org/354023007/diff/1/content/public/renderer/rend...
content/public/renderer/render_font_warmup_win.cc:18: SkFontMgr*
g_warmup_fontmgr = NULL;
I don't think it's a great idea to make another of these. If you can't use the
SkFontMgr, I would instead save off the created factory below and have a getter
for that.

[eae, 2014-06-26 20:58:24]

Thanks for your input Scott, really helpful.

https://codereview.chromium.org/354023007/diff/1/content/common/font_list_win.cc
File content/common/font_list_win.cc (right):

https://codereview.chromium.org/354023007/diff/1/content/common/font_list_win...
content/common/font_list_win.cc:111: AddLocalizedFontFamily(font_list,
font_family, user_locale);
On 2014/06/26 20:13:38, scottmg wrote:
> I'm not sure how this changes the actual names returned -- will we need to
> update various .grd translations of font names along with this?

For fonts with multiple names (e.x. SimSun which is either "宋体" or "SimSun")
this will pick show the name that matches to current locale, falling back on
English or the first name listed.

Given that the names are interchangeable from a programmatic standpoint I don't
think we need to change the translations but I must confess that I don't know
much about the chrome translations code.

https://codereview.chromium.org/354023007/diff/1/content/common/font_list_win...
content/common/font_list_win.cc:126: scoped_ptr<base::ListValue>
GetFontList_SlowBlocking() {
On 2014/06/26 20:13:38, scottmg wrote:
> I'm a little unclear on where this happens. If it's in the browser (I
assume?),
> then we shouldn't be using the content/public/renderer code.

Yeah, the way I get the factory from the sandbox code is a complete hack right
now. Not quite sure what the right way to get the DW factory is though.

https://codereview.chromium.org/354023007/diff/1/content/public/renderer/rend...
File content/public/renderer/render_font_warmup_win.cc (right):

https://codereview.chromium.org/354023007/diff/1/content/public/renderer/rend...
content/public/renderer/render_font_warmup_win.cc:18: SkFontMgr*
g_warmup_fontmgr = NULL;
On 2014/06/26 20:13:38, scottmg wrote:
> I don't think it's a great idea to make another of these. If you can't use the
> SkFontMgr, I would instead save off the created factory below and have a
getter
> for that.

That makes sense. Any idea where this code should live then? Probably not in
public/renderer...

[scottmg, 2014-06-26 21:05:17]

https://codereview.chromium.org/354023007/diff/1/content/common/font_list_win.cc
File content/common/font_list_win.cc (right):

https://codereview.chromium.org/354023007/diff/1/content/common/font_list_win...
content/common/font_list_win.cc:111: AddLocalizedFontFamily(font_list,
font_family, user_locale);
On 2014/06/26 20:58:24, eae wrote:
> On 2014/06/26 20:13:38, scottmg wrote:
> > I'm not sure how this changes the actual names returned -- will we need to
> > update various .grd translations of font names along with this?
> 
> For fonts with multiple names (e.x. SimSun which is either "宋体" or "SimSun")
> this will pick show the name that matches to current locale, falling back on
> English or the first name listed.
> 
> Given that the names are interchangeable from a programmatic standpoint I
don't
> think we need to change the translations but I must confess that I don't know
> much about the chrome translations code.

OK, I just noticed e.g. in
https://src.chromium.org/viewvc/chrome?revision=279449&view=revision the .grd
includes the name? Maybe cpu can comment here.

https://codereview.chromium.org/354023007/diff/1/content/public/renderer/rend...
File content/public/renderer/render_font_warmup_win.cc (right):

https://codereview.chromium.org/354023007/diff/1/content/public/renderer/rend...
content/public/renderer/render_font_warmup_win.cc:18: SkFontMgr*
g_warmup_fontmgr = NULL;
On 2014/06/26 20:58:24, eae wrote:
> On 2014/06/26 20:13:38, scottmg wrote:
> > I don't think it's a great idea to make another of these. If you can't use
the
> > SkFontMgr, I would instead save off the created factory below and have a
> getter
> > for that.
> 
> That makes sense. Any idea where this code should live then? Probably not in
> public/renderer...

Yeah, I think I wouldn't bother sharing with this code now.

There's factory creation in Skia, since it's happening in browser, we don't do
any of that there yet (UI is still GDI rendered).

I _think_ (though am not certain) that code in skia/ext can use
third_party/skia/src/utils/win/SkDWrite.cpp, so sk_get_dwrite_factory() there
might be the easiest, if you just have the content/common code call a utility
function there.

[eae, 2014-06-26 22:01:40]

On 2014/06/26 21:05:17, scottmg wrote:
>
https://codereview.chromium.org/354023007/diff/1/content/common/font_list_win.cc
> File content/common/font_list_win.cc (right):
> 
>
https://codereview.chromium.org/354023007/diff/1/content/common/font_list_win...
> content/common/font_list_win.cc:111: AddLocalizedFontFamily(font_list,
> font_family, user_locale);
> On 2014/06/26 20:58:24, eae wrote:
> > On 2014/06/26 20:13:38, scottmg wrote:
> > > I'm not sure how this changes the actual names returned -- will we need to
> > > update various .grd translations of font names along with this?
> > 
> > For fonts with multiple names (e.x. SimSun which is either "宋体" or "SimSun")
> > this will pick show the name that matches to current locale, falling back on
> > English or the first name listed.
> > 
> > Given that the names are interchangeable from a programmatic standpoint I
> don't
> > think we need to change the translations but I must confess that I don't
know
> > much about the chrome translations code.
> 
> OK, I just noticed e.g. in
> https://src.chromium.org/viewvc/chrome?revision=279449&view=revision the .grd
> includes the name? Maybe cpu can comment here.
> 
>
https://codereview.chromium.org/354023007/diff/1/content/public/renderer/rend...
> File content/public/renderer/render_font_warmup_win.cc (right):
> 
>
https://codereview.chromium.org/354023007/diff/1/content/public/renderer/rend...
> content/public/renderer/render_font_warmup_win.cc:18: SkFontMgr*
> g_warmup_fontmgr = NULL;
> On 2014/06/26 20:58:24, eae wrote:
> > On 2014/06/26 20:13:38, scottmg wrote:
> > > I don't think it's a great idea to make another of these. If you can't use
> the
> > > SkFontMgr, I would instead save off the created factory below and have a
> > getter
> > > for that.
> > 
> > That makes sense. Any idea where this code should live then? Probably not in
> > public/renderer...
> 
> Yeah, I think I wouldn't bother sharing with this code now.
> 
> There's factory creation in Skia, since it's happening in browser, we don't do
> any of that there yet (UI is still GDI rendered).
> 
> I _think_ (though am not certain) that code in skia/ext can use
> third_party/skia/src/utils/win/SkDWrite.cpp, so sk_get_dwrite_factory() there
> might be the easiest, if you just have the content/common code call a utility
> function there.

We don't use SkDwrite in the browser process yet though so I'm not sure how that
would work...

[eae, 2014-06-26 22:32:07]

PTAL

[scottmg, 2014-06-26 22:45:27]

https://codereview.chromium.org/354023007/diff/40001/content/common/font_list...
File content/common/font_list_win.cc (right):

https://codereview.chromium.org/354023007/diff/40001/content/common/font_list...
content/common/font_list_win.cc:34: static void
GetFontListInternal_GDI(scoped_ptr<base::ListValue>& font_list) {
no & here

https://codereview.chromium.org/354023007/diff/40001/content/common/font_list...
content/common/font_list_win.cc:55: static void
AddLocalizedFontFamily(scoped_ptr<base::ListValue>& font_list,
no & here

https://codereview.chromium.org/354023007/diff/40001/content/common/font_list...
content/common/font_list_win.cc:59: if
(SUCCEEDED(font_family->GetFamilyNames(&family_names))) {
gdi skips fonts starting with @ (vertical orientation or something?), do we need
to do that here?

https://codereview.chromium.org/354023007/diff/40001/content/common/font_list...
content/common/font_list_win.cc:77: font_item->Append(new
base::StringValue(name));
i assume 2x is correct since gdi does too, maybe a comment as to why we pass the
same thing twice

https://codereview.chromium.org/354023007/diff/40001/content/common/font_list...
content/common/font_list_win.cc:85: if (family_names)
could this just not go inside the if (SUCCEEDED(GetFamilyNames)) block?

https://codereview.chromium.org/354023007/diff/40001/content/common/font_list...
content/common/font_list_win.cc:105: scoped_ptr<base::ListValue>& font_list) {
no & here

https://codereview.chromium.org/354023007/diff/40001/content/common/font_list...
content/common/font_list_win.cc:113: BOOL checkForUpdates = false;
checkForUpdates -> check_for_updates

https://codereview.chromium.org/354023007/diff/40001/content/common/font_list...
content/common/font_list_win.cc:122: font_family->Release();
move inside if (SUCCEEDED())?

https://codereview.chromium.org/354023007/diff/40001/content/common/font_list...
content/common/font_list_win.cc:126: if (font_collection)
can this move inside the GetSystemFontCollection block and be unconditional?

https://codereview.chromium.org/354023007/diff/40001/content/common/font_list...
content/common/font_list_win.cc:140: if (font_list->GetSize() < 1)
nit (optional); == 0 seems clearer than < 1 to me

[eae, 2014-06-26 23:14:58]

https://codereview.chromium.org/354023007/diff/40001/content/common/font_list...
File content/common/font_list_win.cc (right):

https://codereview.chromium.org/354023007/diff/40001/content/common/font_list...
content/common/font_list_win.cc:59: if
(SUCCEEDED(font_family->GetFamilyNames(&family_names))) {
On 2014/06/26 22:45:27, scottmg wrote:
> gdi skips fonts starting with @ (vertical orientation or something?), do we
need
> to do that here?

We go out of our way to check for the @ prefix in blink code regardless of
rendering path so yeah we should probably do the same here. Good call.

https://codereview.chromium.org/354023007/diff/40001/content/common/font_list...
content/common/font_list_win.cc:85: if (family_names)
On 2014/06/26 22:45:27, scottmg wrote:
> could this just not go inside the if (SUCCEEDED(GetFamilyNames)) block?

The documentation for GetFamilyNames is unclear on whether a failure  can result
in the output parameter being constructed. I'd rather err or the side of
caution.

https://codereview.chromium.org/354023007/diff/40001/content/common/font_list...
content/common/font_list_win.cc:113: BOOL checkForUpdates = false;
On 2014/06/26 22:45:27, scottmg wrote:
> checkForUpdates -> check_for_updates

Thanks, haven't written chromium style code in awhile.

https://codereview.chromium.org/354023007/diff/40001/content/common/font_list...
content/common/font_list_win.cc:122: font_family->Release();
On 2014/06/26 22:45:27, scottmg wrote:
> move inside if (SUCCEEDED())?

Again, it is unclear whether it is guaranteed not to be constructor on failure.

https://codereview.chromium.org/354023007/diff/40001/content/common/font_list...
content/common/font_list_win.cc:140: if (font_list->GetSize() < 1)
On 2014/06/26 22:45:27, scottmg wrote:
> nit (optional); == 0 seems clearer than < 1 to me

I agree.

[scottmg, 2014-06-26 23:48:23]

lgtm

[eae, 2014-06-26 23:58:39]

+cevans for OWNERS

[cpu_(ooo_6.6-7.5), 2014-06-27 01:11:28]

https://codereview.chromium.org/354023007/diff/60001/content/common/font_list...
File content/common/font_list_win.cc (right):

https://codereview.chromium.org/354023007/diff/60001/content/common/font_list...
content/common/font_list_win.cc:112: IDWriteFactory* factory = NULL;
factory and font_collection should be ScopedComPtr here. Also consider simply
returning the ScopedComPtr from the factory in line 93.

Well, in general this code would be cleaner if 
"if (<interface>) <interface>->Release() 

was not there. Any reason to avoid ScopedComPtr?

[Chris Evans, 2014-06-27 01:59:07]

On 2014/06/26 23:58:39, eae wrote:
> +cevans for OWNERS

I'm only OWNER here for the Linux sandboxing files, sorry :-/

[eae, 2014-06-27 02:14:59]

-cevans, +jam for OWNERS

https://codereview.chromium.org/354023007/diff/60001/content/common/font_list...
File content/common/font_list_win.cc (right):

https://codereview.chromium.org/354023007/diff/60001/content/common/font_list...
content/common/font_list_win.cc:112: IDWriteFactory* factory = NULL;
On 2014/06/27 01:11:28, cpu wrote:
> factory and font_collection should be ScopedComPtr here. Also consider simply
> returning the ScopedComPtr from the factory in line 93.
> 
> Well, in general this code would be cleaner if 
> "if (<interface>) <interface>->Release() 
> 
> was not there. Any reason to avoid ScopedComPtr?

Didn't know we had ScopedComPtr. Seems like a perfect fit. Updated it to use it,
thanks!

[cpu_(ooo_6.6-7.5), 2014-06-27 18:02:42]

lgtm

[jam, 2014-06-30 22:08:35]

lgtm

[eae, 2014-06-30 22:09:24]

Thanks for the review everyone.

[eae, 2014-06-30 22:09:28]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-06-30 22:09:45]

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

[commit-bot: I haz the power, 2014-07-01 00:52:12]

Change committed as 280719

[jochen (gone - plz use gerrit), 2014-07-01 07:05:31]

A revert of this CL has been created in
https://codereview.chromium.org/368523002/ by jochen@chromium.org.

The reason for reverting is: Uses a Vista+ only API.