REGRESSION (r104060): Web font is not loaded if specified by link element dynamically...
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 23 Feb 2012 00:42:47 +0000 (00:42 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 23 Feb 2012 00:42:47 +0000 (00:42 +0000)
https://bugs.webkit.org/show_bug.cgi?id=79186

Reviewed by Andreas Kling.

Source/WebCore:

Test: fast/css/font-face-insert-link.html

If a dynamically inserted stylesheet contains @font-face rules, we may fail to update the rendering.

Before r104060 the style selector was destroyed on every style change, and the font selector along with it.
This is no longer the case and we can't rely on comparing font selector pointers when comparing Fonts
for equality. This patch adds version number to the font selector and checks it in Font::operator==.
The version number is incremented when new font-face rules are added to the font selector.

FontFallbackList is an object shared between Fonts so the extra field shouldn't matter much in terms
of memory.

* css/CSSFontSelector.cpp:
(WebCore::CSSFontSelector::CSSFontSelector):
(WebCore::CSSFontSelector::addFontFaceRule):
* css/CSSFontSelector.h:
(CSSFontSelector):
* platform/graphics/Font.cpp:
(WebCore::Font::operator==):
* platform/graphics/FontFallbackList.cpp:
(WebCore::FontFallbackList::FontFallbackList):
(WebCore::FontFallbackList::invalidate):
* platform/graphics/FontFallbackList.h:
(FontFallbackList):
(WebCore::FontFallbackList::fontSelectorVersion):
* platform/graphics/FontSelector.h:
(FontSelector):

LayoutTests:

* fast/css/font-face-insert-link-expected.txt: Added.
* fast/css/font-face-insert-link.html: Added.
* fast/css/resources/ahem.css: Added.
(@font-face):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@108574 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/css/font-face-insert-link-expected.txt [new file with mode: 0644]
LayoutTests/fast/css/font-face-insert-link.html [new file with mode: 0644]
LayoutTests/fast/css/resources/ahem.css [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/css/CSSFontSelector.cpp
Source/WebCore/css/CSSFontSelector.h
Source/WebCore/platform/graphics/Font.cpp
Source/WebCore/platform/graphics/FontFallbackList.cpp
Source/WebCore/platform/graphics/FontFallbackList.h
Source/WebCore/platform/graphics/FontSelector.h

index b509db3..5e2c53b 100644 (file)
@@ -1,3 +1,15 @@
+2012-02-22  Antti Koivisto  <antti@apple.com>
+
+        REGRESSION (r104060): Web font is not loaded if specified by link element dynamically inserted
+        https://bugs.webkit.org/show_bug.cgi?id=79186
+
+        Reviewed by Andreas Kling.
+
+        * fast/css/font-face-insert-link-expected.txt: Added.
+        * fast/css/font-face-insert-link.html: Added.
+        * fast/css/resources/ahem.css: Added.
+        (@font-face):
+
 2012-02-22  Nate Chapin  <japhet@chromium.org>
 
         inspector/debugger/script-formatter-console.html should
diff --git a/LayoutTests/fast/css/font-face-insert-link-expected.txt b/LayoutTests/fast/css/font-face-insert-link-expected.txt
new file mode 100644 (file)
index 0000000..a60ac90
--- /dev/null
@@ -0,0 +1,5 @@
+PASS window.getComputedStyle(a).width == window.getComputedStyle(b).width is false
+PASS successfullyParsed is true
+
+TEST COMPLETE
+TextText
diff --git a/LayoutTests/fast/css/font-face-insert-link.html b/LayoutTests/fast/css/font-face-insert-link.html
new file mode 100644 (file)
index 0000000..ef770b3
--- /dev/null
@@ -0,0 +1,39 @@
+<head>
+<script src="../js/resources/js-test-pre.js"></script>
+<style>
+.test {
+  font-family: 'myahem';
+}
+#a, #b, #container { position:absolute; }
+#b { top: 20px }
+</style>
+</head>
+
+<div id=container>
+<div id=a>Text</div>
+<div id=b class="test">Text</div>
+</div>
+
+<script>
+jsTestIsAsync = true;
+
+window.setTimeout(
+  function() {
+    var link = document.createElement("link");
+    link.setAttribute("href", "resources/ahem.css");
+    link.setAttribute("rel", "stylesheet");
+    link.setAttribute("type", "text/css");
+    document.head.appendChild(link);
+    window.setTimeout(
+      function() {
+        var a = document.getElementById('a');
+        var b = document.getElementById('b');
+        shouldBeFalse('window.getComputedStyle(a).width == window.getComputedStyle(b).width');
+        finishJSTest();
+      },
+     500
+    );
+  },
+  1);
+</script>
+<script src="../js/resources/js-test-post.js"></script>
diff --git a/LayoutTests/fast/css/resources/ahem.css b/LayoutTests/fast/css/resources/ahem.css
new file mode 100644 (file)
index 0000000..640efe5
--- /dev/null
@@ -0,0 +1,4 @@
+@font-face {
+    font-family: 'myahem';
+    src: url(../../../resources/Ahem.ttf);
+}
index 6ec0f0a..1d1535e 100644 (file)
@@ -1,3 +1,38 @@
+2012-02-22  Antti Koivisto  <antti@apple.com>
+
+        REGRESSION (r104060): Web font is not loaded if specified by link element dynamically inserted
+        https://bugs.webkit.org/show_bug.cgi?id=79186
+
+        Reviewed by Andreas Kling.
+
+        Test: fast/css/font-face-insert-link.html
+        
+        If a dynamically inserted stylesheet contains @font-face rules, we may fail to update the rendering.
+        
+        Before r104060 the style selector was destroyed on every style change, and the font selector along with it.
+        This is no longer the case and we can't rely on comparing font selector pointers when comparing Fonts
+        for equality. This patch adds version number to the font selector and checks it in Font::operator==.
+        The version number is incremented when new font-face rules are added to the font selector.
+        
+        FontFallbackList is an object shared between Fonts so the extra field shouldn't matter much in terms
+        of memory.
+
+        * css/CSSFontSelector.cpp:
+        (WebCore::CSSFontSelector::CSSFontSelector):
+        (WebCore::CSSFontSelector::addFontFaceRule):
+        * css/CSSFontSelector.h:
+        (CSSFontSelector):
+        * platform/graphics/Font.cpp:
+        (WebCore::Font::operator==):
+        * platform/graphics/FontFallbackList.cpp:
+        (WebCore::FontFallbackList::FontFallbackList):
+        (WebCore::FontFallbackList::invalidate):
+        * platform/graphics/FontFallbackList.h:
+        (FontFallbackList):
+        (WebCore::FontFallbackList::fontSelectorVersion):
+        * platform/graphics/FontSelector.h:
+        (FontSelector):
+
 2012-02-22  Nate Chapin  <japhet@chromium.org>
 
         Prevent CachedRawResource from sending the same data
index 08650b0..6d22fdc 100644 (file)
@@ -62,6 +62,7 @@ namespace WebCore {
 CSSFontSelector::CSSFontSelector(Document* document)
     : m_document(document)
     , m_beginLoadingTimer(this, &CSSFontSelector::beginLoadTimerFired)
+    , m_version(0)
 {
     // FIXME: An old comment used to say there was no need to hold a reference to m_document
     // because "we are guaranteed to be destroyed before the document". But there does not
@@ -307,6 +308,8 @@ void CSSFontSelector::addFontFaceRule(const CSSFontFaceRule* fontFaceRule)
         }
 
         familyFontFaces->append(fontFace);
+        
+        ++m_version;
     }
 }
 
index 3e992cc..a5f8db5 100644 (file)
@@ -51,6 +51,8 @@ public:
         return adoptRef(new CSSFontSelector(document));
     }
     virtual ~CSSFontSelector();
+    
+    virtual unsigned version() const OVERRIDE { return m_version; }
 
     virtual FontData* getFontData(const FontDescription& fontDescription, const AtomicString& familyName);
 
@@ -85,6 +87,8 @@ private:
 
     Vector<CachedResourceHandle<CachedFont> > m_fontsToBeginLoading;
     Timer<CSSFontSelector> m_beginLoadingTimer;
+    
+    unsigned m_version;
 };
 
 } // namespace WebCore
index c2d9c58..50eee53 100644 (file)
@@ -120,11 +120,12 @@ bool Font::operator==(const Font& other) const
     
     FontSelector* first = m_fontList ? m_fontList->fontSelector() : 0;
     FontSelector* second = other.m_fontList ? other.m_fontList->fontSelector() : 0;
-    
+
     return first == second
            && m_fontDescription == other.m_fontDescription
            && m_letterSpacing == other.m_letterSpacing
            && m_wordSpacing == other.m_wordSpacing
+           && (m_fontList ? m_fontList->fontSelectorVersion() : 0) == (other.m_fontList ? other.m_fontList->fontSelectorVersion() : 0)
            && (m_fontList ? m_fontList->generation() : 0) == (other.m_fontList ? other.m_fontList->generation() : 0);
 }
 
index 2f1b68f..6d11c09 100644 (file)
@@ -39,6 +39,7 @@ FontFallbackList::FontFallbackList()
     : m_pageZero(0)
     , m_cachedPrimarySimpleFontData(0)
     , m_fontSelector(0)
+    , m_fontSelectorVersion(0)
     , m_familyIndex(0)
     , m_generation(fontCache()->generation())
     , m_pitch(UnknownPitch)
@@ -57,6 +58,7 @@ void FontFallbackList::invalidate(PassRefPtr<FontSelector> fontSelector)
     m_pitch = UnknownPitch;
     m_loadingCustomFonts = false;
     m_fontSelector = fontSelector;
+    m_fontSelectorVersion = m_fontSelector ? m_fontSelector->version() : 0;
     m_generation = fontCache()->generation();
 }
 
index 10251a4..12479b2 100644 (file)
@@ -51,6 +51,8 @@ public:
     bool loadingCustomFonts() const { return m_loadingCustomFonts; }
 
     FontSelector* fontSelector() const { return m_fontSelector.get(); }
+    // FIXME: It should be possible to combine fontSelectorVersion and generation.
+    unsigned fontSelectorVersion() const { return m_fontSelectorVersion; }
     unsigned generation() const { return m_generation; }
 
     struct GlyphPagesHashTraits : HashTraits<int> {
@@ -87,6 +89,7 @@ private:
     mutable GlyphPageTreeNode* m_pageZero;
     mutable const SimpleFontData* m_cachedPrimarySimpleFontData;
     RefPtr<FontSelector> m_fontSelector;
+    unsigned m_fontSelectorVersion;
     mutable int m_familyIndex;
     unsigned short m_generation;
     mutable unsigned m_pitch : 3; // Pitch
index e18161b..2c7d149 100644 (file)
@@ -44,6 +44,8 @@ public:
 
     virtual void registerForInvalidationCallbacks(FontSelectorClient*) = 0;
     virtual void unregisterForInvalidationCallbacks(FontSelectorClient*) = 0;
+    
+    virtual unsigned version() const = 0;
 };
 
 class FontSelectorClient {