Add heuristic to LayoutTextControl::hasValidAvgCharWidth()

third_party/WebKit/Source/core/layout/LayoutTextControl.cpp

 /**
  * Copyright (C) 2006, 2007 Apple Inc. All rights reserved.
  *           (C) 2008 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
@@ skipping 171 matching lines @@
     "#HeadLineA",
     "#PCMyungjo",
     "#PilGi",
 };
 
 // For font families where any of the fonts don't have a valid entry in the OS/2 table
 // for avgCharWidth, fallback to the legacy webkit behavior of getting the avgCharWidth
 // from the width of a '0'. This only seems to apply to a fixed number of Mac fonts,
 // but, in order to get similar rendering across platforms, we do this check for
 // all platforms.
-bool LayoutTextControl::hasValidAvgCharWidth(const AtomicString& family)
+bool LayoutTextControl::hasValidAvgCharWidth(const SimpleFontData* font, const AtomicString& family)

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

Can't this be "const SimpleFontData& font"?
(that saves line 196 DCHECK)

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

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

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

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]

eae@ just added a runtime null check. https://codereview.chromium.org/2149683002

 {
+    // Some fonts match avgCharWidth to CJK full-width characters.
+    // Heuristic check to avoid such fonts.
+    DCHECK(font);
+    const FontMetrics& metrics = font->getFontMetrics();
+    if (metrics.hasZeroWidth() && font->avgCharWidth() > metrics.zeroWidth() * 1.7)
+        return false;
+
     static HashSet<AtomicString>* fontFamiliesWithInvalidCharWidthMap = nullptr;
 
     if (family.isEmpty())
         return false;
 
     if (!fontFamiliesWithInvalidCharWidthMap) {
         fontFamiliesWithInvalidCharWidthMap = new HashSet<AtomicString>;
 
         for (size_t i = 0; i < WTF_ARRAY_LENGTH(fontFamiliesWithInvalidCharWidth); ++i)
             fontFamiliesWithInvalidCharWidthMap->add(AtomicString(fontFamiliesWithInvalidCharWidth[i]));
     }
 
     return !fontFamiliesWithInvalidCharWidthMap->contains(family);
 }
 
 float LayoutTextControl::getAvgCharWidth(const AtomicString& family) const
 {
     const Font& font = style()->font();
-    if (hasValidAvgCharWidth(family)) {
-        ASSERT(font.primaryFont());
-        return roundf(font.primaryFont()->avgCharWidth());
-    }
+    const SimpleFontData* primaryFont = font.primaryFont();
+    if (hasValidAvgCharWidth(primaryFont, family))
+        return roundf(primaryFont->avgCharWidth());
 
     const UChar ch = '0';
     const String str = String(&ch, 1);
     TextRun textRun = constructTextRun(font, str, styleRef(), TextRun::AllowTrailingExpansion);
     return font.width(textRun);
 }
 
 float LayoutTextControl::scaleEmToUnits(int x) const
 {
     // This matches the unitsPerEm value for MS Shell Dlg and Courier New from the "head" font table.
@@ skipping 53 matching lines @@
     HTMLElement* placeholder = toHTMLTextFormControlElement(node())->placeholderElement();
     LayoutObject* placeholderLayoutObject = placeholder ? placeholder->layoutObject() : nullptr;
     if (!placeholderLayoutObject)
         return nullptr;
     if (relayoutChildren)
         layoutScope.setChildNeedsLayout(placeholderLayoutObject);
     return placeholderLayoutObject;
 }
 
 } // namespace blink