Consolidate Font::codePath kerning & ligatures exceptions

Consolidate Font::codePath kerning & ligatures exceptions

Currently, we tweak the computed code path in several places to force
kerning & ligatures handling via the complex path. These checks
are identical and can be handled in codePath() instead.

The refactoring preserves existing logic.

R=eae@chromium.org,jbroman@chromium.org,dominik.rottsches@intel.com

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

[Dominik Röttsches, 2014-10-10 13:37:54]

Good idea. A few comments.

https://codereview.chromium.org/642863005/diff/100001/Source/platform/fonts/F...
File Source/platform/fonts/Font.cpp (right):

https://codereview.chromium.org/642863005/diff/100001/Source/platform/fonts/F...
Source/platform/fonts/Font.cpp:305: CodePath Font::codePath(const TextRun& run,
int from, int to) const
I would make the argument a TextRunPaintInfo - which has the from, to parameters
and then just pass the existing runInfo that exists in the first 3 of the 4 call
sites, and create one for the 4th.

https://codereview.chromium.org/642863005/diff/100001/Source/platform/fonts/F...
Source/platform/fonts/Font.cpp:307: // FIXME: Do we really want to override
s_codePath?
Can you elaborate on the question? What do we need s_codePath for?

https://codereview.chromium.org/642863005/diff/100001/Source/platform/fonts/F...
Source/platform/fonts/Font.cpp:308: // FIXME: Use the fast code path once it
handles partial runs with kerning and ligatures.
The second FIXME is obsolete. We plan to remove the fast/simple code path.

[f(malita), 2014-10-10 14:09:12]

https://codereview.chromium.org/642863005/diff/100001/Source/platform/fonts/F...
File Source/platform/fonts/Font.cpp (right):

https://codereview.chromium.org/642863005/diff/100001/Source/platform/fonts/F...
Source/platform/fonts/Font.cpp:305: CodePath Font::codePath(const TextRun& run,
int from, int to) const
On 2014/10/10 13:37:53, Dominik Röttsches wrote:
> I would make the argument a TextRunPaintInfo - which has the from, to
parameters
> and then just pass the existing runInfo that exists in the first 3 of the 4
call
> sites, and create one for the 4th.

Done.

https://codereview.chromium.org/642863005/diff/100001/Source/platform/fonts/F...
Source/platform/fonts/Font.cpp:307: // FIXME: Do we really want to override
s_codePath?
On 2014/10/10 13:37:54, Dominik Röttsches wrote:
> Can you elaborate on the question? What do we need s_codePath for?

Per comments, I assumed it is a way to force a particular code path for
debugging/layout test purposes - in which case it seems it should take
precedence over kerning/ligatures heuristics.

But a quick search reveals that it's not actually being used/set at all!
Removed.

https://codereview.chromium.org/642863005/diff/100001/Source/platform/fonts/F...
Source/platform/fonts/Font.cpp:308: // FIXME: Use the fast code path once it
handles partial runs with kerning and ligatures.
On 2014/10/10 13:37:54, Dominik Röttsches wrote:
> The second FIXME is obsolete. We plan to remove the fast/simple code path.

Do you mean SimpleShaper & friends are going away?

[Dominik Röttsches, 2014-10-10 14:14:59]

> But a quick search reveals that it's not actually being used/set at all!
> Removed.

Good.

>
https://codereview.chromium.org/642863005/diff/100001/Source/platform/fonts/F...
> Source/platform/fonts/Font.cpp:308: // FIXME: Use the fast code path once it
> handles partial runs with kerning and ligatures.
> On 2014/10/10 13:37:54, Dominik Röttsches wrote:
> > The second FIXME is obsolete. We plan to remove the fast/simple code path.
> 
> Do you mean SimpleShaper & friends are going away?

We aim for that, yes, while not regressing in performance. We should enable
kerning by default, like FF does, and we avoid problems with rendering
inconsistencies with a change in code path when we converge on one. The complex
path' performance needs to be brought to a point where we don't regress when we
remove SimpleShaper, but that should be possible.

LGTM, thanks for the quick update.

[f(malita), 2014-10-10 20:02:49]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-10-10 20:03:47]

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

[commit-bot: I haz the power, 2014-10-10 21:54:00]

Committed patchset #3 (id:200001) as 183562