Add heuristic to LayoutTextControl::hasValidAvgCharWidth()

Add heuristic to LayoutTextControl::hasValidAvgCharWidth()

This patch adds a heuristic check to see if the font has invalid
avgCharWidth.

The spec is silent about how avgCharWidth is computed for CJK fonts,
and some fonts selected to match to CJK full-width characters.

Since in most fonts, avgCharWidth is close to the width of "0", when
it's too big compared to the width of "0" can be considered as invalid.

BUG=625058
Committed: https://crrev.com/77183f3386fcb2362ca6140ab5fb15958d7cfa2d
Cr-Commit-Position: refs/heads/master@{#405086}

[kojii, 2016-07-13 06:17:57]

Description was changed from

==========
avgCharWidth

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

to

==========
Add heuristic to LayoutTextControl::hasValidAvgCharWidth()

This patch adds a heuristic check to see if the font has invalid
avgCharWidth.

The spec is silent about how avgCharWidth is computed for CJK fonts,
and some fonts selected to match to CJK full-width characters.

Since in most fonts, avgCharWidth is close to the width of "0", when
it's too big compared to the width of "0" can be considered as invalid.

BUG=625058
==========

[kojii, 2016-07-13 06:18:56]

kojii@chromium.org changed reviewers:
+ tkent@chromium.org

[kojii, 2016-07-13 06:18:57]

WDYT?

[tkent, 2016-07-13 06:31:19]

On 2016/07/13 at 06:18:57, kojii wrote:
> WDYT?

Sounds a good idea.  How popular are fonts with hasZeroWidth()?  Does Noto Sans
CJK have it?

[kojii, 2016-07-13 07:17:23]

On 2016/07/13 at 06:31:19, tkent wrote:
> Sounds a good idea.  How popular are fonts with hasZeroWidth()?  Does Noto
Sans CJK have it?

Most fonts do, including Noto Sans CJK. It's false only if the font doesn't have
a glyph for "0"; e.g., symbol, emoji, etc.

[tkent, 2016-07-13 07:24:01]

lgtm

[kojii, 2016-07-13 07:27:58]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2016-07-13 07:28:20]

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-07-13 08:43:07]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-07-13 08:45:04]

Description was changed from

==========
Add heuristic to LayoutTextControl::hasValidAvgCharWidth()

This patch adds a heuristic check to see if the font has invalid
avgCharWidth.

The spec is silent about how avgCharWidth is computed for CJK fonts,
and some fonts selected to match to CJK full-width characters.

Since in most fonts, avgCharWidth is close to the width of "0", when
it's too big compared to the width of "0" can be considered as invalid.

BUG=625058
==========

to

==========
Add heuristic to LayoutTextControl::hasValidAvgCharWidth()

This patch adds a heuristic check to see if the font has invalid
avgCharWidth.

The spec is silent about how avgCharWidth is computed for CJK fonts,
and some fonts selected to match to CJK full-width characters.

Since in most fonts, avgCharWidth is close to the width of "0", when
it's too big compared to the width of "0" can be considered as invalid.

BUG=625058

Committed: https://crrev.com/77183f3386fcb2362ca6140ab5fb15958d7cfa2d
Cr-Commit-Position: refs/heads/master@{#405086}
==========

[commit-bot: I haz the power, 2016-07-13 08:45:06]

Patchset 1 (id:??) landed as
https://crrev.com/77183f3386fcb2362ca6140ab5fb15958d7cfa2d
Cr-Commit-Position: refs/heads/master@{#405086}

[kochi, 2016-07-13 09:36:12]

kochi@chromium.org changed reviewers:
+ kochi@chromium.org

[kochi, 2016-07-13 09:36:13]

https://codereview.chromium.org/2145903002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutTextControl.cpp (right):

https://codereview.chromium.org/2145903002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTextControl.cpp:192: bool
LayoutTextControl::hasValidAvgCharWidth(const SimpleFontData* font, const
AtomicString& family)
Can't this be "const SimpleFontData& font"?
(that saves line 196 DCHECK)

[kojii, 2016-07-13 11:20:35]

https://codereview.chromium.org/2145903002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutTextControl.cpp (right):

https://codereview.chromium.org/2145903002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTextControl.cpp:192: bool
LayoutTextControl::hasValidAvgCharWidth(const SimpleFontData* font, const
AtomicString& family)
On 2016/07/13 at 09:36:12, kochi wrote:
> Can't this be "const SimpleFontData& font"?
> (that saves line 196 DCHECK)

Making "*" to "&" doesn't assure non-null, so it changes to "DCHECK(&font)",
which looks strange, no?

[tkent, 2016-07-13 23:14:35]

https://codereview.chromium.org/2145903002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutTextControl.cpp (right):

https://codereview.chromium.org/2145903002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTextControl.cpp:192: bool
LayoutTextControl::hasValidAvgCharWidth(const SimpleFontData* font, const
AtomicString& family)
On 2016/07/13 at 11:20:35, kojii wrote:
> On 2016/07/13 at 09:36:12, kochi wrote:
> > Can't this be "const SimpleFontData& font"?
> > (that saves line 196 DCHECK)
> 
> Making "*" to "&" doesn't assure non-null, so it changes to "DCHECK(&font)",
which looks strange, no?

In this case,
 * making the argument type |const SimpleFontData&|,
 * remove DCHECK(font) here, and
 * add DCHECK(primaryFont) in getAvgCharWidth()

would be reasonable.

[tkent, 2016-07-14 00:07:09]

https://codereview.chromium.org/2145903002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutTextControl.cpp (right):

https://codereview.chromium.org/2145903002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTextControl.cpp:192: bool
LayoutTextControl::hasValidAvgCharWidth(const SimpleFontData* font, const
AtomicString& family)
eae@ just added a runtime null check. https://codereview.chromium.org/2149683002