Makes sure that debug-only layout test failures are not to the ZERO WIDTH SPACE mapping to SPACE glyph complaints (http://b/1317563), fixes a layout test (fast/text/zero-width-characters.html), and provides an updated patch for WebKit.org bug 20237 (https://bugs.webkit.org/show_bug.cgi?id=20237).

This change brings handling of the ZWS and CJK character widths down to the level of SimpleFontData by creating special (sub-classed) SimpleFontData objects that are used in GlyphData. These instances are created when the glyph cache is being filled (GlyphPage::fill). More better things are possible, but at the moment I thought it might be good to just get the basics right.

Also, a couple of the layout tests are brought back to pre-font-metric-hacks removal versions.


git-svn-id: svn://svn.chromium.org/chrome/trunk/src@1557 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 168 insertions, 86 deletions

diff --git a/webkit/data/layout_test_results/v8/LayoutTests/fast/repaint/continuation-after-outline-expected.txt b/webkit/data/layout_test_results/v8/LayoutTests/fast/repaint/continuation-after-outline-expected.txt
index b6ac7ce..74aa009 100644
--- a/webkit/data/layout_test_results/v8/LayoutTests/fast/repaint/continuation-after-outline-expected.txt
+++ b/webkit/data/layout_test_results/v8/LayoutTests/fast/repaint/continuation-after-outline-expected.txt
@@ -14,7 +14,7 @@ layer at (0,0) size 800x96
               text run at (21,0) width 7: "x"
           RenderText {#text} at (0,0) size 0x0
       RenderBlock (anonymous) at (0,60) size 784x20
-        RenderText {#text} at (0,0) size 4x19
-          text run at (0,0) width 4: "\x{19}"
+        RenderText {#text} at (0,0) size 0x19
+          text run at (0,0) width 0: "\x{19}"
         RenderText {#text} at (0,0) size 0x0
         RenderText {#text} at (0,0) size 0x0
diff --git a/webkit/data/layout_test_results/v8/LayoutTests/tables/mozilla/images/adforce_imgis_com-expected.checksum b/webkit/data/layout_test_results/v8/LayoutTests/tables/mozilla/images/adforce_imgis_com-expected.checksum
index d0e83b2..08b93d0 100644
--- a/webkit/data/layout_test_results/v8/LayoutTests/tables/mozilla/images/adforce_imgis_com-expected.checksum
+++ b/webkit/data/layout_test_results/v8/LayoutTests/tables/mozilla/images/adforce_imgis_com-expected.checksum
@@ -1 +1 @@
-844d5b7ea45d2c834778fd16f56fd5e1
\ No newline at end of file
+341ad6b6a7d2d9f1d13a56fec8b17743
\ No newline at end of file
diff --git a/webkit/data/layout_test_results/v8/LayoutTests/tables/mozilla/images/adforce_imgis_com-expected.png b/webkit/data/layout_test_results/v8/LayoutTests/tables/mozilla/images/adforce_imgis_com-expected.png
index dfe8727..da3fc637 100644
--- a/webkit/data/layout_test_results/v8/LayoutTests/tables/mozilla/images/adforce_imgis_com-expected.png
+++ b/webkit/data/layout_test_results/v8/LayoutTests/tables/mozilla/images/adforce_imgis_com-expected.png
diff --git a/webkit/data/layout_test_results/v8/LayoutTests/tables/mozilla/images/adforce_imgis_com-expected.txt b/webkit/data/layout_test_results/v8/LayoutTests/tables/mozilla/images/adforce_imgis_com-expected.txt
index 1aef145..40f1195 100644
--- a/webkit/data/layout_test_results/v8/LayoutTests/tables/mozilla/images/adforce_imgis_com-expected.txt
+++ b/webkit/data/layout_test_results/v8/LayoutTests/tables/mozilla/images/adforce_imgis_com-expected.txt
@@ -3,5 +3,5 @@ layer at (0,0) size 800x600
 layer at (0,0) size 800x600
   RenderBlock {HTML} at (0,0) size 800x600
     RenderBody {BODY} at (8,8) size 784x584
-      RenderText {#text} at (0,0) size 664x19
-        text run at (0,0) width 664: "GIF89a\x{D4}\x{1}<\x{20AC}\x{7F}!\x{FF}\x{B}NETSCAPE2.0\x{3}\x{1}!\x{F9}\x{4}\x{4}<,\x{D4}\x{1}<\x{201A}\x{7F}\x{FF}\x{FF}\x{FF}\x{FF}\x{FF}\x{DF}\x{DF}\x{DF}\x{BF}\x{BF}\x{BF}\x{7F}\x{7F}\x{7F}\x{3}\x{FF}8\x{BA}\x{DC}\x{FE}0\x{CA}I\x{AB}\x{BD}8\x{EB}\x{CD}\x{BB}\x{FF}`(\x{17D}di*\x{C6} \x{AC}l\x{EB}\x{BE}p,\x{CF}tm\x{DF}x\x{AE}\x{EF}|\x{EF}\x{FF}\x{C0} pH,"
+      RenderText {#text} at (0,0) size 632x19
+        text run at (0,0) width 632: "GIF89a\x{D4}\x{1}<\x{20AC}\x{7F}!\x{FF}\x{B}NETSCAPE2.0\x{3}\x{1}!\x{F9}\x{4}\x{4}<,\x{D4}\x{1}<\x{201A}\x{7F}\x{FF}\x{FF}\x{FF}\x{FF}\x{FF}\x{DF}\x{DF}\x{DF}\x{BF}\x{BF}\x{BF}\x{7F}\x{7F}\x{7F}\x{3}\x{FF}8\x{BA}\x{DC}\x{FE}0\x{CA}I\x{AB}\x{BD}8\x{EB}\x{CD}\x{BB}\x{FF}`(\x{17D}di*\x{C6} \x{AC}l\x{EB}\x{BE}p,\x{CF}tm\x{DF}x\x{AE}\x{EF}|\x{EF}\x{FF}\x{C0} pH,"
diff --git a/webkit/pending/Font.cpp b/webkit/pending/Font.cpp
index 3f9b6b9..ef3091d 100644
--- a/webkit/pending/Font.cpp
+++ b/webkit/pending/Font.cpp
@@ -163,7 +163,7 @@ void WidthIterator::advance(int offset, GlyphBuffer* glyphBuffer)
             float tabWidth = m_font->tabWidth();
             width = tabWidth - fmodf(m_run.xPos() + runWidthSoFar, tabWidth);
         } else {
-            width = fontData->widthForGlyph(c, glyph);
+            width = fontData->widthForGlyph(glyph);
             // We special case spaces in two ways when applying word rounding.
             // First, we round spaces to an adjusted width in all fonts.
             // Second, in fixed-pitch fonts we ensure that all characters that
@@ -809,4 +809,17 @@ FontSelector* Font::fontSelector() const
     return m_fontList ? m_fontList->fontSelector() : 0;
 }
 
+// static
+bool Font::isCJKCodePoint(UChar32 c)
+{
+    // AC00..D7AF; Hangul Syllables
+    if ((0xAC00 <= c) && (c <= 0xD7AF))
+        return true;
+
+    // CJK ideographs
+    UErrorCode errorCode;
+    return uscript_getScript(c, &errorCode) == USCRIPT_HAN &&
+        U_SUCCESS(errorCode);
+}
+
 }
diff --git a/webkit/pending/SimpleFontData.cpp b/webkit/pending/SimpleFontData.cpp
index fa0e2b1..54abade 100644
--- a/webkit/pending/SimpleFontData.cpp
+++ b/webkit/pending/SimpleFontData.cpp
@@ -50,7 +50,8 @@ SimpleFontData::SimpleFontData(const FontPlatformData& f, bool customFont, bool
     , m_isCustomFont(customFont)
     , m_isLoading(loading)
     , m_smallCapsFontData(0)
-    , m_cjkGlyphWidth(cGlyphWidthUnknown)
+    , m_zeroWidthFontData(new ZeroWidthFontData())
+    , m_cjkWidthFontData(new CJKWidthFontData())
 {
 #if ENABLE(SVG_FONTS) && !PLATFORM(QT)
     if (SVGFontFaceElement* svgFontFaceElement = svgFontData ? svgFontData->svgFontFaceElement() : 0) {
@@ -77,6 +78,8 @@ SimpleFontData::SimpleFontData(const FontPlatformData& f, bool customFont, bool
         determinePitch();
         m_missingGlyphData.fontData = this;
         m_missingGlyphData.glyph = 0;
+        m_zeroWidthFontData->init(this);
+        m_cjkWidthFontData->init(this);
         return;
     }
 #endif
@@ -92,6 +95,8 @@ SimpleFontData::SimpleFontData(const FontPlatformData& f, bool customFont, bool
         determinePitch();
         m_missingGlyphData.fontData = this;
         m_missingGlyphData.glyph = 0;
+        m_zeroWidthFontData->init(this);
+        m_cjkWidthFontData->init(this);
         return;
     }
 
@@ -100,11 +105,18 @@ SimpleFontData::SimpleFontData(const FontPlatformData& f, bool customFont, bool
     // every character and the space are the same width.  Otherwise we round.
     static const UChar32 space_char = ' ';
     m_spaceGlyph = glyphPageZero->glyphDataForCharacter(space_char).glyph;
-    float width = widthForGlyph(space_char, m_spaceGlyph);
+    float width = widthForGlyph(m_spaceGlyph);
     m_spaceWidth = width;
     determinePitch();
     m_adjustedSpaceWidth = m_treatAsFixedPitch ? ceilf(width) : roundf(width);
 
+    // TODO(dglazkov): Investigate and implement across platforms, if needed
+#if PLATFORM(WIN)
+    // ZERO WIDTH SPACES are explicitly mapped to share the glyph
+    // with SPACE (with width adjusted to 0) during GlyphPage::fill
+    // This is currently only implemented for Windows port. The FontData
+    // remapping may very well be needed for other platforms.
+#else
     // Force the glyph for ZERO WIDTH SPACE to have zero width, unless it is shared with SPACE.
     // Helvetica is an example of a non-zero width ZERO WIDTH SPACE glyph.
     // See <http://bugs.webkit.org/show_bug.cgi?id=13178>
@@ -117,9 +129,23 @@ SimpleFontData::SimpleFontData(const FontPlatformData& f, bool customFont, bool
         else
             LOG_ERROR("Font maps SPACE and ZERO WIDTH SPACE to the same glyph. Glyph width not overridden.");
     }
+#endif
 
     m_missingGlyphData.fontData = this;
     m_missingGlyphData.glyph = 0;
+    m_zeroWidthFontData->init(this);
+    m_cjkWidthFontData->init(this);
+}
+
+SimpleFontData::SimpleFontData()
+    : m_treatAsFixedPitch(false)
+#if ENABLE(SVG_FONTS)
+    , m_svgFontData(0)
+#endif
+    , m_isCustomFont(0)
+    , m_isLoading(0)
+    , m_smallCapsFontData(0)
+{
 }
 
 SimpleFontData::~SimpleFontData()
@@ -133,48 +159,15 @@ SimpleFontData::~SimpleFontData()
     // it will be deleted then, so we don't need to do anything here.
 }
 
-// Use the character corresponding the glyph to determine if the glyph
-// is a fixed width CJK glyph. This will allow us to save on storage in
-// GlyphWidthMap for CJK glyph entries having the same width value.
-float SimpleFontData::widthForGlyph(UChar32 c, Glyph glyph) const
+float SimpleFontData::widthForGlyph(Glyph glyph) const
 {
-    bool is_CJK = IsCJKCodePoint(c);
-    float width = is_CJK ? m_cjkGlyphWidth : m_glyphToWidthMap.widthForGlyph(glyph);
-
-#ifndef NDEBUG
-    // Test our optimization that assuming all CGK glyphs have the same width
-    if (is_CJK) {
-        const float actual_width = platformWidthForGlyph(glyph);
-        ASSERT((cGlyphWidthUnknown == width) || (actual_width == width));
-    }
-#endif
-
-    // Some characters should be zero width and we want to ignore whatever
-    // crazy stuff the font may have (or not defined). If the font doesn't
-    // define it, we don't want to measure the width of the "invalid character"
-    // box, for example.
-    //
-    // Note that we have to exempt control characters, which
-    // treatAsZeroWidthSpace would normally return true for. This is primarily
-    // for \n since it will be rendered as a regular space in HTML.
-    //
-    // TODO(brettw): we should have Font::treatAsZeroWidthSpace return true for
-    // zero width spaces (U+200B) just like Font::treatAsSpace will return true
-    // for spaces. Then the additional OR is not necessary.
-    if (c > ' ' && (Font::treatAsZeroWidthSpace(c) || c == 0x200b))
-      return 0.0f;
-
+    float width = m_glyphToWidthMap.widthForGlyph(glyph);
     if (width != cGlyphWidthUnknown)
         return width;
 
     width = platformWidthForGlyph(glyph);
+    m_glyphToWidthMap.setWidthForGlyph(glyph, width);
 
-    if (is_CJK) {
-        m_cjkGlyphWidth = width;
-    } else {
-        m_glyphToWidthMap.setWidthForGlyph(glyph, width);
-    }
-    
     return width;
 }
 
@@ -188,21 +181,57 @@ bool SimpleFontData::isSegmented() const
     return false;
 }
 
-bool SimpleFontData::IsCJKCodePoint(UChar32 c) const
+const SimpleFontData* SimpleFontData::zeroWidthFontData() const
 {
-    // 3400..4DBF; CJK Unified Ideographs Extension A
-    // 4DC0..4DFF; Yijing Hexagram Symbols
-    // 4E00..9FFF; CJK Unified Ideographs
-    if ((0x3400 <= c) && (c <= 0x9FFF))
-        return true;
-    // AC00..D7AF; Hangul Syllables
-    if ((0xAC00 <= c) && (c <= 0xD7AF))
-        return true;
-    // F900..FAFF; CJK Compatibility Ideographs
-    if ((0xF900 <= c) && (c <= 0xFAFF))
-        return true;
+    return m_zeroWidthFontData.get();
+}
 
-    return false;
+const SimpleFontData* SimpleFontData::cjkWidthFontData() const
+{
+    return m_cjkWidthFontData.get();
+}
+
+void ZeroWidthFontData::init(SimpleFontData* fontData)
+{
+    m_font = fontData->m_font;
+    m_smallCapsFontData = fontData->m_smallCapsFontData;
+    m_ascent = fontData->m_ascent;
+    m_descent = fontData->m_descent;
+    m_lineSpacing = fontData->m_lineSpacing;
+    m_lineGap = fontData->m_lineGap;
+    m_maxCharWidth = 0;
+    m_avgCharWidth = 0;
+    m_xHeight = fontData->m_xHeight;
+    m_unitsPerEm = fontData->m_unitsPerEm;
+    m_spaceWidth = 0;
+    m_spaceGlyph = 0;
+    m_adjustedSpaceWidth = fontData->m_adjustedSpaceWidth;
+#if PLATFORM(WIN)
+    m_scriptCache = 0;
+    m_scriptFontProperties = 0;
+#endif
+}
+
+CJKWidthFontData::CJKWidthFontData()
+    : m_cjkGlyphWidth(cGlyphWidthUnknown)
+{
+}
+
+float CJKWidthFontData::widthForGlyph(Glyph glyph) const
+{
+    if (m_cjkGlyphWidth != cGlyphWidthUnknown)
+        return m_cjkGlyphWidth;
+
+    float width = platformWidthForGlyph(glyph);
+    m_cjkGlyphWidth = width;
+
+#ifndef NDEBUG
+    // Test our optimization that assuming all CGK glyphs have the same width
+    const float actual_width = platformWidthForGlyph(glyph);
+    ASSERT((cGlyphWidthUnknown == width) || (actual_width == width));
+#endif
+
+    return width;
 }
 
 } // namespace WebCore
diff --git a/webkit/pending/SimpleFontData.h b/webkit/pending/SimpleFontData.h
index c1a84aa..1ebdeaa 100644
--- a/webkit/pending/SimpleFontData.h
+++ b/webkit/pending/SimpleFontData.h
@@ -44,6 +44,8 @@ class FontPlatformData;
 class SharedBuffer;
 class SVGFontData;
 class WidthMap;
+class ZeroWidthFontData;
+class CJKWidthFontData;
 
 enum Pitch { UnknownPitch, FixedPitch, VariablePitch };
 
@@ -52,6 +54,10 @@ public:
     SimpleFontData(const FontPlatformData&, bool customFont = false, bool loading = false, SVGFontData* data = 0);
     virtual ~SimpleFontData();
 
+protected:
+    // sub-class constructor
+    SimpleFontData();
+
 public:
     const FontPlatformData& platformData() const { return m_font; }
     SimpleFontData* smallCapsFontData(const FontDescription& fontDescription) const;
@@ -67,7 +73,7 @@ public:
     float xHeight() const { return m_xHeight; }
     unsigned unitsPerEm() const { return m_unitsPerEm; }
 
-    float widthForGlyph(UChar32, Glyph) const;
+    virtual float widthForGlyph(Glyph) const;
     float platformWidthForGlyph(Glyph) const;
 
     virtual const SimpleFontData* fontDataForCharacter(UChar32) const;
@@ -117,12 +123,14 @@ public:
     wxFont getWxFont() const { return m_font.font(); }
 #endif
 
+    const SimpleFontData* zeroWidthFontData() const;
+    const SimpleFontData* cjkWidthFontData() const;
+
 private:
     void platformInit();
     void platformDestroy();
     
     void commonInit();
-    bool IsCJKCodePoint(UChar32) const;
 
 public:
     int m_ascent;
@@ -155,9 +163,6 @@ public:
 
     mutable SimpleFontData* m_smallCapsFontData;
 
-    // Optimization for CJK glyphs
-    mutable float m_cjkGlyphWidth;
-
 #if PLATFORM(CG)
     float m_syntheticBoldOffset;
 #endif
@@ -176,6 +181,34 @@ public:
     mutable SCRIPT_CACHE m_scriptCache;
     mutable SCRIPT_FONTPROPERTIES* m_scriptFontProperties;
 #endif
+
+private:
+    OwnPtr<ZeroWidthFontData> m_zeroWidthFontData;
+    OwnPtr<CJKWidthFontData> m_cjkWidthFontData;
+};
+
+// SimpleFontData sub-classes:
+
+// Has a single zero-width glyph, used for ZWS and 
+// UCHAR_DEFAULT_IGNORABLE_CODE_POINT characters
+class ZeroWidthFontData : public SimpleFontData {
+public:
+    void init(SimpleFontData*);
+    virtual float widthForGlyph(Glyph) const { return 0.0f; }
+};
+
+// Monospaced, single glyph and width, used for CJK characters
+// The assumption made here can break for some high-quality CJK fonts with
+// proportional CJK glyphs.
+class CJKWidthFontData : public ZeroWidthFontData {
+public:
+    CJKWidthFontData();
+
+    virtual float widthForGlyph(Glyph) const;
+
+private:
+    // Optimization for CJK glyphs
+    mutable float m_cjkGlyphWidth;
 };
 
 } // namespace WebCore
diff --git a/webkit/port/platform/graphics/GlyphPageTreeNodeWin.cpp b/webkit/port/platform/graphics/GlyphPageTreeNodeWin.cpp
index e32d1ac..185aa44 100644
--- a/webkit/port/platform/graphics/GlyphPageTreeNodeWin.cpp
+++ b/webkit/port/platform/graphics/GlyphPageTreeNodeWin.cpp
@@ -49,6 +49,16 @@ static void FillEmptyGlyphs(GlyphPage* page) {
     page->setGlyphDataForIndex(i, NULL, NULL);
 }
 
+// Lazily initializes space glyph
+static Glyph InitSpaceGlyph(HDC dc, Glyph* space_glyph) {
+    if (*space_glyph)
+        return *space_glyph;
+    static wchar_t space = ' ';
+    GetGlyphIndices(dc, &space, 1, space_glyph, 0);
+    return *space_glyph;
+}
+
+
 // Fills a page of glyphs in the Basic Multilingual Plane (<= U+FFFF). We
 // can use the standard Windows GDI functions here. The input buffer size is
 // assumed to be GlyphPage::size. Returns true if any glyphs were found.
@@ -126,36 +136,37 @@ static bool FillBMPGlyphs(UChar* buffer,
         !(tm.tmPitchAndFamily & TMPF_TRUETYPE))
       invalid_glyph = 0x1F;
 
-    WORD space_glyph = 0;  // Glyph for a space. Lazily filled, see below.
+    Glyph space_glyph = 0;  // Glyph for a space. Lazily filled.
 
     for (unsigned i = 0; i < GlyphPage::size; i++) {
+        UChar c = buffer[i];
+        Glyph glyph = localGlyphBuffer[i];
+        const SimpleFontData* glyphFontData = fontData;
         // When this character should be a space, we ignore whatever the font
         // says and use a space. Otherwise, if fonts don't map one of these
         // space or zero width glyphs, we will get a box.
-        //
-        // TODO(brettw): we should have Font::treatAsZeroWidthSpace return true
-        // for zero width spaces (U+200B) just like Font::treatAsSpace will
-        // return true for spaces. Then the additional OR is not necessary.
-        if (buffer[i] > ' ' &&
-            (Font::treatAsSpace(buffer[i]) ||
-             Font::treatAsZeroWidthSpace(buffer[i]) ||
-             buffer[i] == 0x200B)) {
-            // Hard code the glyph indices for characters that should be treated
-            // like spaces.
-            if (!space_glyph) {
-                // Get the glyph index for space.
-                wchar_t space = ' ';
-                GetGlyphIndices(dc, &space, 1, &space_glyph, 0);
-            }
-            page->setGlyphDataForIndex(i, space_glyph, fontData);
-        } else if (localGlyphBuffer[i] == invalid_glyph) {
+        if (Font::treatAsSpace(c)) {
+            // Hard code the glyph indices for characters that should be
+            // treated like spaces.
+            glyph = InitSpaceGlyph(dc, &space_glyph);
+        // TODO(dglazkov): change Font::treatAsZeroWidthSpace to use
+        // u_hasBinaryProperty, per jungshik's comment here:
+        // https://bugs.webkit.org/show_bug.cgi?id=20237#c6.
+        // Then the additional OR won't be necessary.
+        } else if (Font::treatAsZeroWidthSpace(c) || c == 0x200B) {
+            glyph = InitSpaceGlyph(dc, &space_glyph);
+            glyphFontData = fontData->zeroWidthFontData();
+        } else if (glyph == invalid_glyph) {
             // WebKit expects both the glyph index and FontData
             // pointer to be NULL if the glyph is not present
-            page->setGlyphDataForIndex(i, 0, 0);
+            glyph = 0;
+            glyphFontData = 0;
         } else {
+            if (Font::isCJKCodePoint(c))
+                glyphFontData = fontData->cjkWidthFontData();
             have_glyphs = true;
-            page->setGlyphDataForIndex(i, localGlyphBuffer[i], fontData);
         }
+        page->setGlyphDataForCharacter(i, glyph, glyphFontData);
     }
 
     SelectObject(dc, old_font);
diff --git a/webkit/tools/layout_tests/test_lists/tests_fixable.txt b/webkit/tools/layout_tests/test_lists/tests_fixable.txt
index abdc4b6..2463103 100644
--- a/webkit/tools/layout_tests/test_lists/tests_fixable.txt
+++ b/webkit/tools/layout_tests/test_lists/tests_fixable.txt
@@ -120,10 +120,6 @@ V8 | KJS # LayoutTests/svg/dom/animated-tearoff-lifespan.xhtml = FAIL | PASS
 V8 | KJS # LayoutTests/css2.1/t1202-counter-04-b.html = FAIL
 V8 | KJS # LayoutTests/css2.1/t1202-counters-04-b.html = FAIL
 
-// Bug 1316382: fails now that we use the same font code path in test_shell
-// as in Chrome: some characters that should have zero width don't
-V8 | KJS # LayoutTests/fast/text/zero-width-characters.html = FAIL
-
 // Bug 1124513: the max length is being applied correctly, but the over- and
 // under-lines aren't placed properly over the "x".
 // The under-lines are a cosmetic error which is not necessary to fix for Beta. (eseidel)