Changeset 31324

Timestamp:

03/26/08 12:22:01 (20 months ago)

Author:

mjs@apple.com

Message:

WebCore:

2008-03-26 Maciej Stachowiak < mjs@apple.com>

Reviewed by Darin.

• fixed "SVG multichar glyph matching matches longest instead of first (affects Acid3 test 79)"  http://bugs.webkit.org/show_bug.cgi?id=18118

• svg/SVGFont.cpp: (WebCore::SVGTextRunWalker::walk):
• svg/SVGFontElement.cpp: (WebCore::SVGFontElement::SVGFontElement): (WebCore::SVGFontElement::addGlyphToCache): (WebCore::SVGFontElement::removeGlyphFromCache): (WebCore::SVGFontElement::ensureGlyphCache): (WebCore::SVGFontElement::getGlyphIdentifiersForString):
• svg/SVGFontElement.h:
• svg/SVGGlyphElement.h: (WebCore::SVGGlyphIdentifier::SVGGlyphIdentifier):
• svg/SVGGlyphMap.h: Added. New radix tree based glyph map. (WebCore::GlyphMapNode::GlyphMapNode): (WebCore::SVGGlyphMap::SVGGlyphMap): (WebCore::SVGGlyphMap::add): (WebCore::SVGGlyphMap::compareGlyphPriority): (WebCore::SVGGlyphMap::get): (WebCore::SVGGlyphMap::clear):

LayoutTests:

2008-03-26 Maciej Stachowiak < mjs@apple.com>

Reviewed by Darin.

• test updates for "SVG multichar glyph matching matches longest instead of first (affects Acid3 test 79)"  http://bugs.webkit.org/show_bug.cgi?id=18118

These test cases were already checking for this exact bug and now
render correctly.

• platform/mac/svg/W3C-SVG-1.1/fonts-glyph-02-t-expected.txt:
• platform/mac/svg/W3C-SVG-1.1/fonts-glyph-04-t-expected.txt:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/mac/svg/W3C-SVG-1.1/fonts-glyph-02-t-expected.txt (modified) (1 diff)
• LayoutTests/platform/mac/svg/W3C-SVG-1.1/fonts-glyph-04-t-expected.txt (modified) (1 diff)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/svg/SVGFont.cpp (modified) (3 diffs)
• WebCore/svg/SVGFontElement.cpp (modified) (3 diffs)
• WebCore/svg/SVGFontElement.h (modified) (2 diffs)
• WebCore/svg/SVGGlyphElement.h (modified) (2 diffs)
• WebCore/svg/SVGGlyphMap.h (added)

trunk/LayoutTests/ChangeLog

@@ -1 @@
-2008-03-26  Eric Seidel  <eric@webkit.org>
+2008-03-26  Maciej Stachowiak  <mjs@apple.com>
+
+        Reviewed by Darin.
+
+        - test updates for "SVG multichar glyph matching matches longest instead of first (affects Acid3 test 79)"
+        http://bugs.webkit.org/show_bug.cgi?id=18118
+
+        These test cases were already checking for this exact bug and now
+        render correctly.
+        
+        * platform/mac/svg/W3C-SVG-1.1/fonts-glyph-02-t-expected.txt:
+        * platform/mac/svg/W3C-SVG-1.1/fonts-glyph-04-t-expected.txt:
+
+o2008-03-26  Eric Seidel  <eric@webkit.org>
 
         Reviewed by darin.

trunk/LayoutTests/platform/mac/svg/W3C-SVG-1.1/fonts-glyph-02-t-expected.txt

@@ -3 +3 @@
 layer at (0,0) size 480x360
   RenderSVGRoot {svg} at (0.50,0.50) size 479x359
-    RenderSVGContainer {g} at (100,36) size 209x180
+    RenderSVGContainer {g} at (100,36) size 210x180
       RenderSVGHiddenContainer {defs} at (0,0) size 0x0
-      RenderSVGContainer {g} at (100,36) size 184x80
-        RenderSVGText {text} at (100,100) size 184x80 contains 1 chunk(s)
-          RenderSVGInlineText {#text} at (0,-64) size 184x80
-            chunk 1 text run 1 at (100.00,100.00) startOffset 0 endOffset 5 width 184.00 RTL: "\x{69A} \x{69A}\x{69A}\x{69A}"
-      RenderSVGContainer {g} at (100,136) size 209x80
-        RenderSVGText {text} at (100,200) size 209x80 contains 1 chunk(s)
-          RenderSVGInlineText {#text} at (0,-64) size 209x80
-            chunk 1 text run 1 at (100.00,200.00) startOffset 0 endOffset 5 width 223.92 RTL: "\x{62E} \x{62E}\x{62E}\x{62E}"
+      RenderSVGContainer {g} at (100,36) size 200x80
+        RenderSVGText {text} at (100,100) size 200x80 contains 1 chunk(s)
+          RenderSVGInlineText {#text} at (0,-64) size 200x80
+            chunk 1 text run 1 at (100.00,100.00) startOffset 0 endOffset 5 width 200.00 RTL: "\x{69A} \x{69A}\x{69A}\x{69A}"
+      RenderSVGContainer {g} at (100,136) size 210x80
+        RenderSVGText {text} at (100,200) size 210x80 contains 1 chunk(s)
+          RenderSVGInlineText {#text} at (0,-64) size 210x80
+            chunk 1 text run 1 at (100.00,200.00) startOffset 0 endOffset 5 width 209.92 RTL: "\x{62E} \x{62E}\x{62E}\x{62E}"
     RenderSVGText {text} at (10,340) size 264x46 contains 1 chunk(s)
       RenderSVGInlineText {#text} at (0,-36) size 264x46

trunk/LayoutTests/platform/mac/svg/W3C-SVG-1.1/fonts-glyph-04-t-expected.txt

@@ -3 +3 @@
 layer at (0,0) size 480x360
   RenderSVGRoot {svg} at (0.50,0.50) size 479x359
-    RenderSVGContainer {g} at (100,60) size 25x150
+    RenderSVGContainer {g} at (100,60) size 64x150
       RenderSVGHiddenContainer {defs} at (0,0) size 0x0
-      RenderSVGText {text} at (100,100) size 25x50 contains 1 chunk(s)
-        RenderSVGInlineText {#text} at (0,-40) size 25x50
-          chunk 1 text run 1 at (100.00,100.00) startOffset 0 endOffset 3 width 25.00: "ffl"
+      RenderSVGText {text} at (100,100) size 64x50 contains 1 chunk(s)
+        RenderSVGInlineText {#text} at (0,-40) size 64x50
+          chunk 1 text run 1 at (100.00,100.00) startOffset 0 endOffset 3 width 64.00: "ffl"
       RenderSVGText {text} at (100,200) size 25x50 contains 1 chunk(s)
         RenderSVGInlineText {#text} at (0,-40) size 25x50

trunk/WebCore/ChangeLog

@@ +1 @@
+2008-03-26  Maciej Stachowiak  <mjs@apple.com>
+
+        Reviewed by Darin.
+
+        - fixed "SVG multichar glyph matching matches longest instead of first (affects Acid3 test 79)"
+        http://bugs.webkit.org/show_bug.cgi?id=18118
+
+        * svg/SVGFont.cpp:
+        (WebCore::SVGTextRunWalker::walk):
+        * svg/SVGFontElement.cpp:
+        (WebCore::SVGFontElement::SVGFontElement):
+        (WebCore::SVGFontElement::addGlyphToCache):
+        (WebCore::SVGFontElement::removeGlyphFromCache):
+        (WebCore::SVGFontElement::ensureGlyphCache):
+        (WebCore::SVGFontElement::getGlyphIdentifiersForString):
+        * svg/SVGFontElement.h:
+        * svg/SVGGlyphElement.h:
+        (WebCore::SVGGlyphIdentifier::SVGGlyphIdentifier):
+        * svg/SVGGlyphMap.h: Added. New radix tree based glyph map.
+        (WebCore::GlyphMapNode::GlyphMapNode):
+        (WebCore::SVGGlyphMap::SVGGlyphMap):
+        (WebCore::SVGGlyphMap::add):
+        (WebCore::SVGGlyphMap::compareGlyphPriority):
+        (WebCore::SVGGlyphMap::get):
+        (WebCore::SVGGlyphMap::clear):
+
 2008-03-26  David Hyatt  <hyatt@apple.com>
 

trunk/WebCore/svg/SVGFont.cpp

@@ -30 +30 @@
 #include "SVGFontData.h"
 #include "SVGGlyphElement.h"
+#include "SVGGlyphMap.h"
 #include "SVGFontElement.h"
 #include "SVGFontFaceElement.h"
@@ -242 +243 @@
         ASSERT(to - from == run.length());
 
-        int maximumHashKeyLength = m_fontElement->maximumHashKeyLength();
-        ASSERT(maximumHashKeyLength >= 0);
-
         Vector<SVGGlyphIdentifier::ArabicForm> chars(charactersWithArabicForm(String(run.data(from), run.length()), run.rtl()));
 
@@ -256 +254 @@
             // We have to check wheter the current character & the next character define a ligature. This needs to be
             // extended to the n-th next character (where n is 'characterLookupRange'), to check for any possible ligature.
-            characterLookupRange = maximumHashKeyLength + i >= endOfScanRange ? endOfScanRange - i : maximumHashKeyLength;
-
-            // FIXME: instead of checking from longest string to shortest, this should really scan in order
-            // of the glyphs and pick the first match
-            while (characterLookupRange > 0 && !foundGlyph) {
-                String lookupString(run.data(run.rtl() ? run.length() - (i + characterLookupRange) : i), characterLookupRange);
-
-                Vector<SVGGlyphIdentifier> glyphs = m_fontElement->glyphIdentifiersForString(lookupString);
-                Vector<SVGGlyphIdentifier>::iterator it = glyphs.begin();
-                Vector<SVGGlyphIdentifier>::iterator end = glyphs.end();
-
-                for (; it != end; ++it) {
-                    identifier = *it;
-
-                    unsigned int startPosition = run.rtl() ? run.length() - (i + lookupString.length()) : i;
-                    unsigned int endPosition = startPosition + lookupString.length();
-
-                    if (identifier.isValid && isCompatibleGlyph(identifier, isVerticalText, language, chars, startPosition, endPosition)) {
-                        ASSERT(characterLookupRange > 0);
-                        i += characterLookupRange - 1;
-                        m_walkerData.charsConsumed += characterLookupRange;
-
-                        foundGlyph = true;
-                        SVGGlyphElement::inheritUnspecifiedAttributes(identifier, m_fontData);
-                        break;
-                    }
+            characterLookupRange = endOfScanRange - i;
+
+            String lookupString(run.data(run.rtl() ? run.length() - (i + characterLookupRange) : i), characterLookupRange);
+
+            Vector<SVGGlyphIdentifier> glyphs;
+            m_fontElement->getGlyphIdentifiersForString(lookupString, glyphs);
+            Vector<SVGGlyphIdentifier>::iterator it = glyphs.begin();
+            Vector<SVGGlyphIdentifier>::iterator end = glyphs.end();
+            
+            for (; it != end; ++it) {
+                identifier = *it;
+                
+                unsigned int startPosition = run.rtl() ? run.length() - (i + lookupString.length()) : i;
+                unsigned int endPosition = startPosition + lookupString.length();
+                
+                if (identifier.isValid && isCompatibleGlyph(identifier, isVerticalText, language, chars, startPosition, endPosition)) {
+                    ASSERT(characterLookupRange > 0);
+                    i += identifier.nameLength - 1;
+                    m_walkerData.charsConsumed += identifier.nameLength;
+                    
+                    foundGlyph = true;
+                    SVGGlyphElement::inheritUnspecifiedAttributes(identifier, m_fontData);
+                    break;
                 }
-
-                characterLookupRange--;
             }
 
+            
             if (!foundGlyph) {
                 ++m_walkerData.charsConsumed;

trunk/WebCore/svg/SVGFontElement.cpp

@@ -37 +37 @@
 SVGFontElement::SVGFontElement(const QualifiedName& tagName, Document* doc)
     : SVGStyledElement(tagName, doc)
-    , m_maximumHashKeyLength(0)
+    , m_isGlyphCacheValid(false)
 {
 }
@@ -47 +47 @@
 void SVGFontElement::addGlyphToCache(SVGGlyphElement* glyphElement)
 {
-    ASSERT(glyphElement);
-
-    String glyphString = glyphElement->getAttribute(unicodeAttr);
-    if (glyphString.isEmpty()) // No unicode property, means that glyph will be used in <altGlyph> situations!
-        return;
-
-    SVGGlyphIdentifier identifier = glyphElement->buildGlyphIdentifier();
-    identifier.isValid = true;
-
-    if (glyphString.length() > m_maximumHashKeyLength)
-        m_maximumHashKeyLength = glyphString.length();
-
-    GlyphHashMap::iterator glyphsIt = m_glyphMap.find(glyphString);
-    if (glyphsIt == m_glyphMap.end()) {
-        Vector<SVGGlyphIdentifier> glyphs;
-        glyphs.append(identifier);
-
-        m_glyphMap.add(glyphString, glyphs);
-    } else {
-        Vector<SVGGlyphIdentifier>& glyphs = (*glyphsIt).second;
-        glyphs.append(identifier);
-    }
+    if (m_isGlyphCacheValid)
+        m_glyphMap.clear();
+    m_isGlyphCacheValid = false;
 }
 
 void SVGFontElement::removeGlyphFromCache(SVGGlyphElement* glyphElement)
 {
-    ASSERT(glyphElement);
-
-    String glyphString = glyphElement->getAttribute(unicodeAttr);
-    if (glyphString.isEmpty()) // No unicode property, means that glyph will be used in <altGlyph> situations!
-        return;
-
-    GlyphHashMap::iterator glyphsIt = m_glyphMap.find(glyphString);
-    ASSERT(glyphsIt != m_glyphMap.end());
-
-    Vector<SVGGlyphIdentifier>& glyphs = (*glyphsIt).second;
-
-    if (glyphs.size() == 1)
-        m_glyphMap.remove(glyphString);
-    else {
-        SVGGlyphIdentifier identifier = glyphElement->buildGlyphIdentifier();
-        identifier.isValid = true;
-
-        Vector<SVGGlyphIdentifier>::iterator it = glyphs.begin();
-        Vector<SVGGlyphIdentifier>::iterator end = glyphs.end();
-
-        unsigned int position = 0;
-        for (; it != end; ++it) {
-            if ((*it) == identifier)
-                break;
-
-            position++;
-        }
-
-        ASSERT(position < glyphs.size());
-        glyphs.remove(position);
-    }
-
-    // If we remove a glyph from cache, whose unicode property length is equal to
-    // m_maximumHashKeyLength then we need to recalculate the hash key length, because there
-    // is either no more glyph with that length, or there are still more glyphs with the maximum length.
-    if (glyphString.length() == m_maximumHashKeyLength) {
-        m_maximumHashKeyLength = 0;
-
-        GlyphHashMap::iterator it = m_glyphMap.begin();
-        GlyphHashMap::iterator end = m_glyphMap.end();
-
-        for (; it != end; ++it) {
-            if ((*it).first.length() > m_maximumHashKeyLength)
-                m_maximumHashKeyLength = (*it).first.length();
-        }
-    }
+    if (m_isGlyphCacheValid)
+        m_glyphMap.clear();
+    m_isGlyphCacheValid = false;
 }
 
@@ -131 +69 @@
 }
 
-const Vector<SVGGlyphIdentifier>& SVGFontElement::glyphIdentifiersForString(const String& string) const
+void SVGFontElement::ensureGlyphCache() const
 {
-    GlyphHashMap::const_iterator it = m_glyphMap.find(string);
-    if (it == m_glyphMap.end()) {
-        static Vector<SVGGlyphIdentifier> s_emptyGlyphList;
-        return s_emptyGlyphList;
+    if (m_isGlyphCacheValid)
+        return;
+
+    for (Node* child = firstChild(); child; child = child->nextSibling()) {
+        if (child->hasTagName(glyphTag)) {
+            SVGGlyphElement* glyph = static_cast<SVGGlyphElement*>(child);
+            String unicode = glyph->getAttribute(unicodeAttr);
+            if (unicode.length())
+                m_glyphMap.add(unicode, glyph->buildGlyphIdentifier());
+        }
     }
+        
+    m_isGlyphCacheValid = true;
+}
 
-    return (*it).second;
+void SVGFontElement::getGlyphIdentifiersForString(const String& string, Vector<SVGGlyphIdentifier>& glyphs) const
+{
+    ensureGlyphCache();
+    m_glyphMap.get(string, glyphs);
 }
 

trunk/WebCore/svg/SVGFontElement.h

@@ -25 +25 @@
 #include "SVGExternalResourcesRequired.h"
 #include "SVGGlyphElement.h"
+#include "SVGGlyphMap.h"
 #include "SVGStyledElement.h"
 
@@ -42 +43 @@
         void removeGlyphFromCache(SVGGlyphElement*);
 
-        const Vector<SVGGlyphIdentifier>& glyphIdentifiersForString(const String&) const;
-
-        // Returns the longest hash key length (the 'unicode' property value with the
-        // highest amount of characters) - ie. for <glyph unicode="ffl"/> it will return 3.
-        unsigned int maximumHashKeyLength() const { return m_maximumHashKeyLength; }
+        void getGlyphIdentifiersForString(const String&, Vector<SVGGlyphIdentifier>&) const;
 
         SVGMissingGlyphElement* firstMissingGlyphElement() const;
 
     private:
-        typedef HashMap<String, Vector<SVGGlyphIdentifier> > GlyphHashMap;
-        GlyphHashMap m_glyphMap;
+        void ensureGlyphCache() const;
 
-        unsigned int m_maximumHashKeyLength;
+        mutable SVGGlyphMap m_glyphMap;
+        mutable bool m_isGlyphCacheValid;
     };
 

trunk/WebCore/svg/SVGGlyphElement.h

@@ -52 +52 @@
         SVGGlyphIdentifier()
             : isValid(false)
-            , priority(0)
             , orientation(Both)
             , arabicForm(None)
+            , priority(0)
+            , nameLength(0)
             , horizontalAdvanceX(0.0f)
             , verticalOriginX(0.0f)
@@ -84 +85 @@
 
         bool isValid : 1;
-        int priority;
 
         Orientation orientation : 2;
         ArabicForm arabicForm : 3;
+        int priority;
+        size_t nameLength;
         String glyphName;