Font fallback for UI

Updates UI font fallback to use DirectWrite when available
so that we can fallback to the color emoji font.

Summary of changes:
TextAnalysisSource was moved from content/common to
gfx/ui/win, namespace was changed accordingly, and users
updated.
The public GetUniscribeFallbackFont API was renamed to
GetFallbackFont. This API will now check if it can use
DirectWrite font fallback; otherwise it will call the old
codepath. Using DirectWrite for fallback allows the UI
code to correctly fallback on the color emoji font when
available.

BUG=617376
Committed: https://crrev.com/ce612ebdb8874b563d020338969f20f227b8dd9f
Cr-Commit-Position: refs/heads/master@{#402879}

[Ilya Kulshin, 2016-06-23 18:09:27]

Description was changed from

==========
Font fallback for UI

TODO: rename /win/*_win; formatting; update content/child to use
gfx/win/dwritetextanalysis

BUG=
==========

to

==========
Font fallback for UI

Summary of changes:
TextAnalysisSource was moved from content/common to gfx/ui/win, namespace was
changed accordingly, and users updated.
The public GetUniscribeFallbackFont API was renamed to GetFallbackFont. This API
will now check if it can use DirectWrite font fallback; otherwise it will call
the old codepath. Using DirectWrite for fallback allows the UI code to correctly
fallback on the color emoji font when available.

BUG=617376
==========

[Ilya Kulshin, 2016-06-23 19:37:49]

Description was changed from

==========
Font fallback for UI

Summary of changes:
TextAnalysisSource was moved from content/common to gfx/ui/win, namespace was
changed accordingly, and users updated.
The public GetUniscribeFallbackFont API was renamed to GetFallbackFont. This API
will now check if it can use DirectWrite font fallback; otherwise it will call
the old codepath. Using DirectWrite for fallback allows the UI code to correctly
fallback on the color emoji font when available.

BUG=617376
==========

to

==========
Font fallback for UI

Summary of changes:
TextAnalysisSource was moved from content/common to
gfx/ui/win, namespace was changed accordingly, and users
updated.
The public GetUniscribeFallbackFont API was renamed to
GetFallbackFont. This API will now check if it can use
DirectWrite font fallback; otherwise it will call the old
codepath. Using DirectWrite for fallback allows the UI
code to correctly fallback on the color emoji font when
available.

BUG=617376
==========

[Ilya Kulshin, 2016-06-23 19:39:05]

Description was changed from

==========
Font fallback for UI

Summary of changes:
TextAnalysisSource was moved from content/common to
gfx/ui/win, namespace was changed accordingly, and users
updated.
The public GetUniscribeFallbackFont API was renamed to
GetFallbackFont. This API will now check if it can use
DirectWrite font fallback; otherwise it will call the old
codepath. Using DirectWrite for fallback allows the UI
code to correctly fallback on the color emoji font when
available.

BUG=617376
==========

to

==========
Updates UI font fallback to use DirectWrite when available
so that we can fallback to the color emoji font.

Summary of changes:
TextAnalysisSource was moved from content/common to
gfx/ui/win, namespace was changed accordingly, and users
updated.
The public GetUniscribeFallbackFont API was renamed to
GetFallbackFont. This API will now check if it can use
DirectWrite font fallback; otherwise it will call the old
codepath. Using DirectWrite for fallback allows the UI
code to correctly fallback on the color emoji font when
available.

BUG=617376
==========

[Ilya Kulshin, 2016-06-23 19:43:29]

kulshin@chromium.org changed reviewers:
+ ananta@chromium.org, asvitkine@google.com, msw@chromium.org

[Ilya Kulshin, 2016-06-23 19:43:30]

ptal

msw@ - ui/gfx/*
ananta@ - content/*

[Ilya Kulshin, 2016-06-23 19:43:53]

kulshin@chromium.org changed reviewers:
+ asvitkine@chromium.org
- asvitkine@google.com

[msw, 2016-06-23 20:59:38]

I don't know direct write code well, but I hope my review helps.

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
File ui/gfx/font_fallback_win.cc (right):

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:23: #include "ui/gfx/platform_font_win.h"
q: what is this used for?

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:351:
gfx::win::CreateDWriteFactory(factory.Receive());
Is it unreasonably expensive for every fallback attempt to call LoadLibraryW?
Can we instead get the instance cached with PlatformFontWin?

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:364: DWRITE_NUMBER_SUBSTITUTION_METHOD_NONE, L"" /*
locale */,
Can/should we use the actual locale from base::18n or icu?

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:371:
Microsoft::WRL::MakeAndInitialize<gfx::win::TextAnalysisSource>(
This can fail; try uniscribe (or bail?) if it doesn't succeed.

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:372: text_analysis.Receive(), text, L"" /* locale
*/,
ditto locale q

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:373: number_substitution.get(),
DWRITE_READING_DIRECTION_LEFT_TO_RIGHT);
Can/should we use the actual text direction here?

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:376: original_name.c_str(),
DWRITE_FONT_WEIGHT_NORMAL,
Can/should we use the weight and style of the input font?

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:382:
GetFamilyNameFromDirectWriteFont(mapped_font.get(), &family_name);
Check the result here (or remove the result and just return the name?)

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:386: return false;
Fall back to uniscribe here?

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/platform_font_w...
File ui/gfx/platform_font_win.h (right):

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/platform_font_w...
ui/gfx/platform_font_win.h:23: // The family name is returned in the
|family_name_ret| parameter.
nit: rename the param |family_name| or |family_name_out|?

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/platform_font_w...
ui/gfx/platform_font_win.h:25: HRESULT
GetFamilyNameFromDirectWriteFont(IDWriteFont* dwrite_font,
nit: move below class decl

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/render_text_har...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/render_text_har...
ui/gfx/render_text_harfbuzz.cc:1396: Font uniscribe_font(primary_font);
nit: rename |fallback_font| or similar

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/win/dwrite_text...
File ui/gfx/win/dwrite_text_analysis_source.h (right):

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/win/dwrite_text...
ui/gfx/win/dwrite_text_analysis_source.h:5: #ifndef
UI_GFX_WIN_DWRITE_TEXT_ANALYSIS_SOURCE_H_
optional nit: rename files direct_write* to match ui/gfx/win/direct_write.*
(or rename to simply text_analysis_source.* to match the class name)

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/win/dwrite_text...
ui/gfx/win/dwrite_text_analysis_source.h:21: class GFX_EXPORT TextAnalysisSource
Can you move the applicable unit tests with this file?

https://codereview.chromium.org/2054273002/diff/120001/ui/gfx/render_text_uni...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/2054273002/diff/120001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:310: // Needed to bypass DCHECK in
GetFallbackFont
nit: trailing period.

https://codereview.chromium.org/2054273002/diff/120001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:311: base::MessageLoopForUI message_loop_;
q: can you move this to just the needed fixtures? (if there aren't many)

https://codereview.chromium.org/2054273002/diff/120001/ui/gfx/text_elider_uni...
File ui/gfx/text_elider_unittest.cc (right):

https://codereview.chromium.org/2054273002/diff/120001/ui/gfx/text_elider_uni...
ui/gfx/text_elider_unittest.cc:335: // Needed to bypass DCHECK in
GetFallbackFont
ditto nit: trailing period

[Ilya Kulshin, 2016-06-24 20:48:25]

ptal

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
File ui/gfx/font_fallback_win.cc (right):

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:23: #include "ui/gfx/platform_font_win.h"
On 2016/06/23 20:59:37, msw wrote:
> q: what is this used for?

This is needed for GetFamilyNameFromDirectWriteFont().

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:351:
gfx::win::CreateDWriteFactory(factory.Receive());
On 2016/06/23 20:59:38, msw wrote:
> Is it unreasonably expensive for every fallback attempt to call LoadLibraryW?
> Can we instead get the instance cached with PlatformFontWin?

Now that we no longer need to support XP and Vista, I'm working on a change to
link to dwrite.dll directly, without having to call LoadLibrary. Not in this
change, but should happen soon.

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:364: DWRITE_NUMBER_SUBSTITUTION_METHOD_NONE, L"" /*
locale */,
On 2016/06/23 20:59:37, msw wrote:
> Can/should we use the actual locale from base::18n or icu?

Done.

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:371:
Microsoft::WRL::MakeAndInitialize<gfx::win::TextAnalysisSource>(
On 2016/06/23 20:59:38, msw wrote:
> This can fail; try uniscribe (or bail?) if it doesn't succeed.

This would only fail if we fail to alloc memory. I'll add checks so this bails,
but I don't think it's worth trying to fallback to uniscribe.

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:372: text_analysis.Receive(), text, L"" /* locale
*/,
On 2016/06/23 20:59:37, msw wrote:
> ditto locale q

Done.

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:373: number_substitution.get(),
DWRITE_READING_DIRECTION_LEFT_TO_RIGHT);
On 2016/06/23 20:59:38, msw wrote:
> Can/should we use the actual text direction here?

Done.

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:376: original_name.c_str(),
DWRITE_FONT_WEIGHT_NORMAL,
On 2016/06/23 20:59:37, msw wrote:
> Can/should we use the weight and style of the input font?

Weight was fairly simple to pipe through. Style was manageable, although
DirectWrite apparently doesn't care about potential underline for purposes of
fallback, so I'm ignoring underline style. As far as I can tell |Font| has no
concept of font stretch, so I'm hardcoding font_stretch_normal. Since this API
only needs to be get the right font family that should be sufficient.

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:382:
GetFamilyNameFromDirectWriteFont(mapped_font.get(), &family_name);
On 2016/06/23 20:59:38, msw wrote:
> Check the result here (or remove the result and just return the name?)

Done.

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:386: return false;
On 2016/06/23 20:59:38, msw wrote:
> Fall back to uniscribe here?

DirectWrite returns a null mapped_font if it cannot find any font to render the
requested characters. If that happens, it is highly unlikely that uniscribe
would find anything.

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/platform_font_w...
File ui/gfx/platform_font_win.h (right):

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/platform_font_w...
ui/gfx/platform_font_win.h:23: // The family name is returned in the
|family_name_ret| parameter.
On 2016/06/23 20:59:38, msw wrote:
> nit: rename the param |family_name| or |family_name_out|?

Done.

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/platform_font_w...
ui/gfx/platform_font_win.h:25: HRESULT
GetFamilyNameFromDirectWriteFont(IDWriteFont* dwrite_font,
On 2016/06/23 20:59:38, msw wrote:
> nit: move below class decl

Done.

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/render_text_har...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/render_text_har...
ui/gfx/render_text_harfbuzz.cc:1396: Font uniscribe_font(primary_font);
On 2016/06/23 20:59:38, msw wrote:
> nit: rename |fallback_font| or similar

Done.

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/win/dwrite_text...
File ui/gfx/win/dwrite_text_analysis_source.h (right):

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/win/dwrite_text...
ui/gfx/win/dwrite_text_analysis_source.h:5: #ifndef
UI_GFX_WIN_DWRITE_TEXT_ANALYSIS_SOURCE_H_
On 2016/06/23 20:59:38, msw wrote:
> optional nit: rename files direct_write* to match ui/gfx/win/direct_write.*
> (or rename to simply text_analysis_source.* to match the class name)

Done. Renamed to text_analysis_source.*

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/win/dwrite_text...
ui/gfx/win/dwrite_text_analysis_source.h:21: class GFX_EXPORT TextAnalysisSource
On 2016/06/23 20:59:38, msw wrote:
> Can you move the applicable unit tests with this file?

This class doesn't have dedicated unit tests, and instead relies on being
exercised by other tests. Although now that it's no longer in content (where
those other tests live), perhaps it deserves tests of its own.

https://codereview.chromium.org/2054273002/diff/120001/ui/gfx/render_text_uni...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/2054273002/diff/120001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:310: // Needed to bypass DCHECK in
GetFallbackFont
On 2016/06/23 20:59:38, msw wrote:
> nit: trailing period.

Done.

https://codereview.chromium.org/2054273002/diff/120001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:311: base::MessageLoopForUI message_loop_;
On 2016/06/23 20:59:38, msw wrote:
> q: can you move this to just the needed fixtures? (if there aren't many)

There's about two dozen tests that need this. I'll leave it as-is for now.

https://codereview.chromium.org/2054273002/diff/120001/ui/gfx/text_elider_uni...
File ui/gfx/text_elider_unittest.cc (right):

https://codereview.chromium.org/2054273002/diff/120001/ui/gfx/text_elider_uni...
ui/gfx/text_elider_unittest.cc:335: // Needed to bypass DCHECK in
GetFallbackFont
On 2016/06/23 20:59:38, msw wrote:
> ditto nit: trailing period

Done.

[msw, 2016-06-27 20:45:37]

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
File ui/gfx/font_fallback_win.cc (right):

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:23: #include "ui/gfx/platform_font_win.h"
On 2016/06/24 20:48:24, Ilya Kulshin wrote:
> On 2016/06/23 20:59:37, msw wrote:
> > q: what is this used for?
> 
> This is needed for GetFamilyNameFromDirectWriteFont().

Acknowledged.

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:351:
gfx::win::CreateDWriteFactory(factory.Receive());
On 2016/06/24 20:48:24, Ilya Kulshin wrote:
> On 2016/06/23 20:59:38, msw wrote:
> > Is it unreasonably expensive for every fallback attempt to call
LoadLibraryW?
> > Can we instead get the instance cached with PlatformFontWin?
> 
> Now that we no longer need to support XP and Vista, I'm working on a change to
> link to dwrite.dll directly, without having to call LoadLibrary. Not in this
> change, but should happen soon.

Is it not easily feasible to cache an instance in the mean time? If we need to
LoadLibrary every time, I'd like to know roughly how much it will regress
performance, especially for users encountering many strings that require
fallback. Can you try caching or run some quick numbers on the perf impact?

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:371:
Microsoft::WRL::MakeAndInitialize<gfx::win::TextAnalysisSource>(
On 2016/06/24 20:48:24, Ilya Kulshin wrote:
> On 2016/06/23 20:59:38, msw wrote:
> > This can fail; try uniscribe (or bail?) if it doesn't succeed.
> 
> This would only fail if we fail to alloc memory. I'll add checks so this
bails,
> but I don't think it's worth trying to fallback to uniscribe.

Acknowledged.

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:376: original_name.c_str(),
DWRITE_FONT_WEIGHT_NORMAL,
On 2016/06/24 20:48:24, Ilya Kulshin wrote:
> On 2016/06/23 20:59:37, msw wrote:
> > Can/should we use the weight and style of the input font?
> 
> Weight was fairly simple to pipe through. Style was manageable, although
> DirectWrite apparently doesn't care about potential underline for purposes of
> fallback, so I'm ignoring underline style. As far as I can tell |Font| has no
> concept of font stretch, so I'm hardcoding font_stretch_normal. Since this API
> only needs to be get the right font family that should be sufficient.

Acknowledged.

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:386: return false;
On 2016/06/24 20:48:24, Ilya Kulshin wrote:
> On 2016/06/23 20:59:38, msw wrote:
> > Fall back to uniscribe here?
> 
> DirectWrite returns a null mapped_font if it cannot find any font to render
the
> requested characters. If that happens, it is highly unlikely that uniscribe
> would find anything.

Acknowledged.

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/win/dwrite_text...
File ui/gfx/win/dwrite_text_analysis_source.h (right):

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/win/dwrite_text...
ui/gfx/win/dwrite_text_analysis_source.h:21: class GFX_EXPORT TextAnalysisSource
On 2016/06/24 20:48:24, Ilya Kulshin wrote:
> On 2016/06/23 20:59:38, msw wrote:
> > Can you move the applicable unit tests with this file?
> 
> This class doesn't have dedicated unit tests, and instead relies on being
> exercised by other tests. Although now that it's no longer in content (where
> those other tests live), perhaps it deserves tests of its own.

Agreed; thanks for adding some!

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/font_fallback_w...
File ui/gfx/font_fallback_win.cc (right):

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:378: base::i18n::IsRTL() ?
DWRITE_READING_DIRECTION_RIGHT_TO_LEFT
I think this should actually use GetFirstStrongCharacterDirection (the reading
direction of the text), not that of the application locale, since the value is
used for TextAnalysisSource::GetParagraphReadingDirection.

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/win/text_analys...
File ui/gfx/win/text_analysis_source_unittest.cc (right):

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/win/text_analys...
ui/gfx/win/text_analysis_source_unittest.cc:21: }
nit: blank line above and add a comment: "}  // namespace".

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/win/text_analys...
ui/gfx/win/text_analysis_source_unittest.cc:25: TextAnalysisSourceTest() {
nit: define a dtor or use "= default;"?

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/win/text_analys...
ui/gfx/win/text_analysis_source_unittest.cc:38: void
CreateDWriteFactory(IUnknown** factory) {
Can you use gfx::win::CreateDWriteFactory instead?

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/win/text_analys...
ui/gfx/win/text_analysis_source_unittest.cc:54: mswr::ComPtr<TextAnalysisSource>
source_;
nit: make private; add a protected/public accessor.

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/win/text_analys...
ui/gfx/win/text_analysis_source_unittest.cc:55: };
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/win/text_analys...
ui/gfx/win/text_analysis_source_unittest.cc:60: HRESULT hr;
nit: init to E_FAIL or E_UNEXPECTED; ditto elsewhere.

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/win/text_analys...
ui/gfx/win/text_analysis_source_unittest.cc:67: hr =
source_->GetLocaleName(kTextLength, &length, &locale_name);
nit: add a comment explaining why this fails? (is it because it's attempting to
determine the locale for an out-of-bounds position?)

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/win/text_analys...
ui/gfx/win/text_analysis_source_unittest.cc:82: hr =
source_->GetNumberSubstitution(kTextLength, &length,
ditto nit: add a simple comment on why this fails.

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/win/text_analys...
ui/gfx/win/text_analysis_source_unittest.cc:110: EXPECT_TRUE(SUCCEEDED(hr));
nit: odd that this passes... should you check that it fails with kTextLength + 1
or is that simply attempting to access invalid memory?

[Ilya Kulshin, 2016-06-28 18:08:01]

ptal.

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/font_fallback_w...
File ui/gfx/font_fallback_win.cc (right):

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:378: base::i18n::IsRTL() ?
DWRITE_READING_DIRECTION_RIGHT_TO_LEFT
On 2016/06/27 20:45:36, msw wrote:
> I think this should actually use GetFirstStrongCharacterDirection (the reading
> direction of the text), not that of the application locale, since the value is
> used for TextAnalysisSource::GetParagraphReadingDirection.

Using GetFirstStrongCharacterDirection might give incorrect results if a chunk
of text begins with a bidi-embedded sequence (such as "<arabic><english>" in
English UI, or "<english><arabic>" in Arabic UI). Since this code is concerned
with rendering text in UI, I think using the UI reading direction as the
paragraph direction is more likely to be correct.

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/win/text_analys...
File ui/gfx/win/text_analysis_source_unittest.cc (right):

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/win/text_analys...
ui/gfx/win/text_analysis_source_unittest.cc:21: }
On 2016/06/27 20:45:37, msw wrote:
> nit: blank line above and add a comment: "}  // namespace".

Done.

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/win/text_analys...
ui/gfx/win/text_analysis_source_unittest.cc:25: TextAnalysisSourceTest() {
On 2016/06/27 20:45:37, msw wrote:
> nit: define a dtor or use "= default;"?

This class does not require custom destruction logic. I'm not sure what would be
the benefit of defining a default destructor.

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/win/text_analys...
ui/gfx/win/text_analysis_source_unittest.cc:38: void
CreateDWriteFactory(IUnknown** factory) {
On 2016/06/27 20:45:37, msw wrote:
> Can you use gfx::win::CreateDWriteFactory instead?

Done.

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/win/text_analys...
ui/gfx/win/text_analysis_source_unittest.cc:54: mswr::ComPtr<TextAnalysisSource>
source_;
On 2016/06/27 20:45:36, msw wrote:
> nit: make private; add a protected/public accessor.

Made protected.

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/win/text_analys...
ui/gfx/win/text_analysis_source_unittest.cc:55: };
On 2016/06/27 20:45:37, msw wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/win/text_analys...
ui/gfx/win/text_analysis_source_unittest.cc:60: HRESULT hr;
On 2016/06/27 20:45:36, msw wrote:
> nit: init to E_FAIL or E_UNEXPECTED; ditto elsewhere.

Done.

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/win/text_analys...
ui/gfx/win/text_analysis_source_unittest.cc:67: hr =
source_->GetLocaleName(kTextLength, &length, &locale_name);
On 2016/06/27 20:45:37, msw wrote:
> nit: add a comment explaining why this fails? (is it because it's attempting
to
> determine the locale for an out-of-bounds position?)

Done.

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/win/text_analys...
ui/gfx/win/text_analysis_source_unittest.cc:82: hr =
source_->GetNumberSubstitution(kTextLength, &length,
On 2016/06/27 20:45:36, msw wrote:
> ditto nit: add a simple comment on why this fails.

Done.

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/win/text_analys...
ui/gfx/win/text_analysis_source_unittest.cc:110: EXPECT_TRUE(SUCCEEDED(hr));
On 2016/06/27 20:45:37, msw wrote:
> nit: odd that this passes... should you check that it fails with kTextLength +
1
> or is that simply attempting to access invalid memory?

My reading of
https://msdn.microsoft.com/en-us/library/windows/desktop/dd371330(v=vs.85).aspx
is that this function should succeed and return null when called with an out of
range index.

[msw, 2016-06-28 18:37:23]

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
File ui/gfx/font_fallback_win.cc (right):

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:351:
gfx::win::CreateDWriteFactory(factory.Receive());
On 2016/06/27 20:45:36, msw wrote:
> On 2016/06/24 20:48:24, Ilya Kulshin wrote:
> > On 2016/06/23 20:59:38, msw wrote:
> > > Is it unreasonably expensive for every fallback attempt to call
> LoadLibraryW?
> > > Can we instead get the instance cached with PlatformFontWin?
> > 
> > Now that we no longer need to support XP and Vista, I'm working on a change
to
> > link to dwrite.dll directly, without having to call LoadLibrary. Not in this
> > change, but should happen soon.
> 
> Is it not easily feasible to cache an instance in the mean time? If we need to
> LoadLibrary every time, I'd like to know roughly how much it will regress
> performance, especially for users encountering many strings that require
> fallback. Can you try caching or run some quick numbers on the perf impact?

Ping?

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/font_fallback_w...
File ui/gfx/font_fallback_win.cc (right):

https://codereview.chromium.org/2054273002/diff/200001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:378: base::i18n::IsRTL() ?
DWRITE_READING_DIRECTION_RIGHT_TO_LEFT
On 2016/06/28 18:08:01, Ilya Kulshin wrote:
> On 2016/06/27 20:45:36, msw wrote:
> > I think this should actually use GetFirstStrongCharacterDirection (the
reading
> > direction of the text), not that of the application locale, since the value
is
> > used for TextAnalysisSource::GetParagraphReadingDirection.
> 
> Using GetFirstStrongCharacterDirection might give incorrect results if a chunk
> of text begins with a bidi-embedded sequence (such as "<arabic><english>" in
> English UI, or "<english><arabic>" in Arabic UI). Since this code is concerned
> with rendering text in UI, I think using the UI reading direction as the
> paragraph direction is more likely to be correct.

Hmm, perhaps; we've had bugs in the past about tab titles, textfields etc.
showing with the wrong base directionality. Hopefully this doesn't regress that.

[Ilya Kulshin, 2016-06-28 20:32:52]

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
File ui/gfx/font_fallback_win.cc (right):

https://codereview.chromium.org/2054273002/diff/100001/ui/gfx/font_fallback_w...
ui/gfx/font_fallback_win.cc:351:
gfx::win::CreateDWriteFactory(factory.Receive());
On 2016/06/28 18:37:23, msw wrote:
> On 2016/06/27 20:45:36, msw wrote:
> > On 2016/06/24 20:48:24, Ilya Kulshin wrote:
> > > On 2016/06/23 20:59:38, msw wrote:
> > > > Is it unreasonably expensive for every fallback attempt to call
> > LoadLibraryW?
> > > > Can we instead get the instance cached with PlatformFontWin?
> > > 
> > > Now that we no longer need to support XP and Vista, I'm working on a
change
> to
> > > link to dwrite.dll directly, without having to call LoadLibrary. Not in
this
> > > change, but should happen soon.
> > 
> > Is it not easily feasible to cache an instance in the mean time? If we need
to
> > LoadLibrary every time, I'd like to know roughly how much it will regress
> > performance, especially for users encountering many strings that require
> > fallback. Can you try caching or run some quick numbers on the perf impact?
> 
> Ping?

Sorry, didn't notice this comment. I did some measurements, and it looks like
the calls to LoadLibrary add about 10-15 microseconds (direct write is usually
loaded already anyway). Not sure if that would really matter, but added a cache
anyway.

[msw, 2016-06-28 20:41:47]

lgtm

[Ilya Kulshin, 2016-06-29 16:12:30]

kulshin@chromium.org changed reviewers:
+ jam@chromium.org

[Ilya Kulshin, 2016-06-29 16:12:31]

+jam@ - could I get an owners approval for
components/url_formatter/elide_url_unittest.cc and content/*?

[jam, 2016-06-29 18:14:55]

lgtm

[Ilya Kulshin, 2016-06-29 18:21:24]

The CQ bit was checked by kulshin@chromium.org

[Ilya Kulshin, 2016-06-29 18:21:24]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2054273002/#ps260001
(title: "Rebase")

[commit-bot: I haz the power, 2016-06-29 18:22:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 18:47:07]

Description was changed from

==========
Updates UI font fallback to use DirectWrite when available
so that we can fallback to the color emoji font.

Summary of changes:
TextAnalysisSource was moved from content/common to
gfx/ui/win, namespace was changed accordingly, and users
updated.
The public GetUniscribeFallbackFont API was renamed to
GetFallbackFont. This API will now check if it can use
DirectWrite font fallback; otherwise it will call the old
codepath. Using DirectWrite for fallback allows the UI
code to correctly fallback on the color emoji font when
available.

BUG=617376
==========

to

==========
Updates UI font fallback to use DirectWrite when available
so that we can fallback to the color emoji font.

Summary of changes:
TextAnalysisSource was moved from content/common to
gfx/ui/win, namespace was changed accordingly, and users
updated.
The public GetUniscribeFallbackFont API was renamed to
GetFallbackFont. This API will now check if it can use
DirectWrite font fallback; otherwise it will call the old
codepath. Using DirectWrite for fallback allows the UI
code to correctly fallback on the color emoji font when
available.

BUG=617376
==========

[commit-bot: I haz the power, 2016-06-29 18:47:08]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-06-29 18:47:30]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-29 18:49:17]

Description was changed from

==========
Updates UI font fallback to use DirectWrite when available
so that we can fallback to the color emoji font.

Summary of changes:
TextAnalysisSource was moved from content/common to
gfx/ui/win, namespace was changed accordingly, and users
updated.
The public GetUniscribeFallbackFont API was renamed to
GetFallbackFont. This API will now check if it can use
DirectWrite font fallback; otherwise it will call the old
codepath. Using DirectWrite for fallback allows the UI
code to correctly fallback on the color emoji font when
available.

BUG=617376
==========

to

==========
Updates UI font fallback to use DirectWrite when available
so that we can fallback to the color emoji font.

Summary of changes:
TextAnalysisSource was moved from content/common to
gfx/ui/win, namespace was changed accordingly, and users
updated.
The public GetUniscribeFallbackFont API was renamed to
GetFallbackFont. This API will now check if it can use
DirectWrite font fallback; otherwise it will call the old
codepath. Using DirectWrite for fallback allows the UI
code to correctly fallback on the color emoji font when
available.

BUG=617376

Committed: https://crrev.com/ce612ebdb8874b563d020338969f20f227b8dd9f
Cr-Commit-Position: refs/heads/master@{#402879}
==========

[commit-bot: I haz the power, 2016-06-29 18:49:19]

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