Force ComplexPath if any part of the node requires it

Force ComplexPath if any part of the node requires it

Change our font measurement/rendering logic to force the ComplexPath for
the entire RenderText if any part of it requires it. This ensures
consistency between preferred width and line break calculations.
Also change FontCacheSkiaWin to use the subpixel settings from the
primary font in case of character based fallback.

TEST=fast/text/international/complex-text-with-default-font-spaces.html
BUG=412758
R=leviw@chromium.org

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

[eae, 2014-10-01 18:50:36]

eae@chromium.org changed reviewers:
+ leviw@chromium.org

[eae, 2014-10-01 20:08:01]

On 2014/10/01 19:06:06, eae wrote:

Obviously need a few rebaseline lines on windows. The mac failures are likely
all harfbuzz fallout related. Verifying that now.

[leviw_travelin_and_unemployed, 2014-10-01 20:33:15]

Just a couple questions. I think this is a good change.

https://codereview.chromium.org/622653002/diff/1/Source/platform/fonts/Font.cpp
File Source/platform/fonts/Font.cpp (right):

https://codereview.chromium.org/622653002/diff/1/Source/platform/fonts/Font.c...
Source/platform/fonts/Font.cpp:301: if (run.useComplexCodePath())
Are you sure you want this check here? Could a RenderText that uses the complex
code path have an 8bit run?

https://codereview.chromium.org/622653002/diff/1/Source/platform/fonts/win/Fo...
File Source/platform/fonts/win/FontCacheSkiaWin.cpp (right):

https://codereview.chromium.org/622653002/diff/1/Source/platform/fonts/win/Fo...
Source/platform/fonts/win/FontCacheSkiaWin.cpp:169: if (data &&
originalFontData) {
Isn't this necessary for other platforms?

[eae, 2014-10-01 21:16:43]

https://codereview.chromium.org/622653002/diff/1/Source/platform/fonts/Font.cpp
File Source/platform/fonts/Font.cpp (right):

https://codereview.chromium.org/622653002/diff/1/Source/platform/fonts/Font.c...
Source/platform/fonts/Font.cpp:301: if (run.useComplexCodePath())
On 2014/10/01 20:33:14, leviw wrote:
> Are you sure you want this check here? Could a RenderText that uses the
complex
> code path have an 8bit run?

Yes but then it would be due to a CSS setting in which case the text content is
irrelevant. I could move it up if you prefer though, doesn't hurt in any way.

https://codereview.chromium.org/622653002/diff/1/Source/platform/fonts/win/Fo...
File Source/platform/fonts/win/FontCacheSkiaWin.cpp (right):

https://codereview.chromium.org/622653002/diff/1/Source/platform/fonts/win/Fo...
Source/platform/fonts/win/FontCacheSkiaWin.cpp:169: if (data &&
originalFontData) {
On 2014/10/01 20:33:15, leviw wrote:
> Isn't this necessary for other platforms?

Not at the moment, windows is the only platform where these can change at
runtime. For the other platforms subpixel is determined at startup.

[leviw_travelin_and_unemployed, 2014-10-01 21:37:28]

https://codereview.chromium.org/622653002/diff/1/Source/platform/fonts/Font.cpp
File Source/platform/fonts/Font.cpp (right):

https://codereview.chromium.org/622653002/diff/1/Source/platform/fonts/Font.c...
Source/platform/fonts/Font.cpp:301: if (run.useComplexCodePath())
On 2014/10/01 21:16:43, eae wrote:
> On 2014/10/01 20:33:14, leviw wrote:
> > Are you sure you want this check here? Could a RenderText that uses the
> complex
> > code path have an 8bit run?
> 
> Yes but then it would be due to a CSS setting in which case the text content
is
> irrelevant. I could move it up if you prefer though, doesn't hurt in any way.

I'm not sure I understand. The case I'm thinking is a RenderText with
non-western script that requires complex text, but also breaks up into some 8bit
runs. Isn't the point of this patch that we'll handle all those TextRuns using
the complex path? This as is could still handle 8bit runs as simple in this
case, no?

[eae, 2014-10-01 21:40:47]

On 2014/10/01 21:37:28, leviw wrote:
>
https://codereview.chromium.org/622653002/diff/1/Source/platform/fonts/Font.cpp
> File Source/platform/fonts/Font.cpp (right):
> 
>
https://codereview.chromium.org/622653002/diff/1/Source/platform/fonts/Font.c...
> Source/platform/fonts/Font.cpp:301: if (run.useComplexCodePath())
> On 2014/10/01 21:16:43, eae wrote:
> > On 2014/10/01 20:33:14, leviw wrote:
> > > Are you sure you want this check here? Could a RenderText that uses the
> > complex
> > > code path have an 8bit run?
> > 
> > Yes but then it would be due to a CSS setting in which case the text content
> is
> > irrelevant. I could move it up if you prefer though, doesn't hurt in any
way.
> 
> I'm not sure I understand. The case I'm thinking is a RenderText with
> non-western script that requires complex text, but also breaks up into some
8bit
> runs. Isn't the point of this patch that we'll handle all those TextRuns using
> the complex path? This as is could still handle 8bit runs as simple in this
> case, no?

In that case all runs for that RenderText will be 16bit, we never mix 8 and 16
bit in a single RenderText instance.

You do bring up a good point in that none of this is obvious from reading the
code. I'll move the check to be above to 8bit check for clarity.

[leviw_travelin_and_unemployed, 2014-10-01 21:51:33]

Thanks for explaining. LGTM!

[eae, 2014-10-01 22:43:43]

The CQ bit was checked by eae@chromium.org

[eae, 2014-10-01 22:43:47]

The CQ bit was unchecked by eae@chromium.org

[eae, 2014-10-01 22:43:58]

Patchset #2 (id:20001) has been deleted

[eae, 2014-10-01 22:46:01]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-10-01 22:46:59]

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

[commit-bot: I haz the power, 2014-10-02 00:03:32]

Committed patchset #2 (id:40001) as 183104