Attachment 260874 Details for Bug 146194 – Patch

[patch] Patch

bug-146194-20150909144038.patch (text/plain), 9.50 KB, created by Myles C. Maxfield on 2015-09-09 14:40:44 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Myles C. Maxfield

Created: 2015-09-09 14:40:44 PDT

Size: 9.50 KB

patch

obsolete

>Subversion Revision: 189547
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 3fb4ef0f67b28cba1a607d7ee118ddb03ea13869..81b6c8327f662bb1efa4fb66ace7a95fce4ff187 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,21 @@
>+2015-09-09  Myles C. Maxfield  <mmaxfield@apple.com>
>+
>+        ASSERTION FAILED: typesettingFeatures & (Kerning | Ligatures) in WebCore::applyFontTransforms
>+        https://bugs.webkit.org/show_bug.cgi?id=146194
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        We might trigger shaping even if the author hasn't specified kerning or ligatures.
>+
>+        Test: fast/text/softbank-emoji-no-ligatures-nor-kerning.html
>+
>+        * platform/graphics/WidthIterator.cpp:
>+        (WebCore::isSoftBankEmoji):
>+        (WebCore::WidthIterator::applyFontTransforms):
>+        (WebCore::WidthIterator::advanceInternal):
>+        (WebCore::applyFontTransforms): Deleted.
>+        * platform/graphics/WidthIterator.h:
>+
> 2015-09-09  Commit Queue  <commit-queue@webkit.org>
> 
>         Unreviewed, rolling out r189536 and r189538.
>diff --git a/Source/WebCore/platform/graphics/WidthIterator.cpp b/Source/WebCore/platform/graphics/WidthIterator.cpp
>index ca488d9e6d2f62379ed3b21b11e91f800e21d2ed..faefab2892b3f5a76d07f87096103f8bbf7315fe 100644
>--- a/Source/WebCore/platform/graphics/WidthIterator.cpp
>+++ b/Source/WebCore/platform/graphics/WidthIterator.cpp
>@@ -95,11 +95,23 @@ public:
>     float advanceAtCharacter;
> };
> 
>-typedef Vector<std::pair<int, OriginalAdvancesForCharacterTreatedAsSpace>, 64> CharactersTreatedAsSpace;
>+static inline bool isSoftBankEmoji(UChar32 codepoint)
>+{
>+    return codepoint >= 0xE001 && codepoint <= 0xE537;
>+}
> 
>-static inline float applyFontTransforms(GlyphBuffer* glyphBuffer, bool ltr, int& lastGlyphCount, const Font* font, WidthIterator& iterator, TypesettingFeatures typesettingFeatures, bool force, CharactersTreatedAsSpace& charactersTreatedAsSpace)
>+inline auto WidthIterator::shouldApplyFontTransforms(const GlyphBuffer* glyphBuffer, int lastGlyphCount, UChar32 previousCharacter) const -> TransformsType
> {
>-    ASSERT(typesettingFeatures & (Kerning | Ligatures));
>+    if (glyphBuffer && glyphBuffer->size() == lastGlyphCount + 1 && isSoftBankEmoji(previousCharacter))
>+        return TransformsType::Forced;
>+    if (m_run.length() <= 1 || !(m_typesettingFeatures & (Kerning | Ligatures)))
>+        return TransformsType::None;
>+    return TransformsType::NotForced;
>+}
>+
>+inline float WidthIterator::applyFontTransforms(GlyphBuffer* glyphBuffer, bool ltr, int& lastGlyphCount, const Font* font, TypesettingFeatures typesettingFeatures, UChar32 previousCharacter, bool force, CharactersTreatedAsSpace& charactersTreatedAsSpace)
>+{
>+    ASSERT_UNUSED(previousCharacter, shouldApplyFontTransforms(glyphBuffer, lastGlyphCount, previousCharacter) != WidthIterator::TransformsType::None);
> 
>     if (!glyphBuffer)
>         return 0;
>@@ -118,16 +130,14 @@ static inline float applyFontTransforms(GlyphBuffer* glyphBuffer, bool ltr, int&
>     if (!ltr)
>         glyphBuffer->reverse(lastGlyphCount, glyphBufferSize - lastGlyphCount);
> 
>-#if !ENABLE(SVG_FONTS)
>-    UNUSED_PARAM(iterator);
>-#else
>+#if ENABLE(SVG_FONTS)
>     // We need to handle transforms on SVG fonts internally, since they are rendered internally.
>     if (font->isSVGFont()) {
>         // SVG font ligatures are handled during glyph selection, only kerning remaining.
>-        if (iterator.run().renderingContext() && (typesettingFeatures & Kerning)) {
>+        if (run().renderingContext() && (typesettingFeatures & Kerning)) {
>             // FIXME: We could pass the necessary context down to this level so we can lazily create rendering contexts at this point.
>             // However, a larger refactoring of SVG fonts might necessary to sidestep this problem completely.
>-            iterator.run().renderingContext()->applySVGKerning(font, iterator, glyphBuffer, lastGlyphCount);
>+            run().renderingContext()->applySVGKerning(font, *this, glyphBuffer, lastGlyphCount);
>         }
>     } else
> #endif
>@@ -194,20 +204,6 @@ static inline std::pair<bool, bool> expansionLocation(bool ideograph, bool treat
>     return std::make_pair(expandLeft, expandRight);
> }
> 
>-static inline bool isSoftBankEmoji(UChar32 codepoint)
>-{
>-    return codepoint >= 0xE001 && codepoint <= 0xE537;
>-}
>-
>-inline auto WidthIterator::shouldApplyFontTransforms(const GlyphBuffer* glyphBuffer, int lastGlyphCount, UChar32 previousCharacter) const -> TransformsType
>-{
>-    if (glyphBuffer && glyphBuffer->size() == lastGlyphCount + 1 && isSoftBankEmoji(previousCharacter))
>-        return TransformsType::Forced;
>-    if (m_run.length() <= 1 || !(m_typesettingFeatures & (Kerning | Ligatures)))
>-        return TransformsType::None;
>-    return TransformsType::NotForced;
>-}
>-
> template <typename TextIterator>
> inline unsigned WidthIterator::advanceInternal(TextIterator& textIterator, GlyphBuffer* glyphBuffer)
> {
>@@ -269,7 +265,7 @@ inline unsigned WidthIterator::advanceInternal(TextIterator& textIterator, Glyph
>         if (font != lastFontData && width) {
>             auto transformsType = shouldApplyFontTransforms(glyphBuffer, lastGlyphCount, previousCharacter);
>             if (transformsType != TransformsType::None) {
>-                m_runWidthSoFar += applyFontTransforms(glyphBuffer, m_run.ltr(), lastGlyphCount, lastFontData, *this, m_typesettingFeatures, transformsType == TransformsType::Forced, charactersTreatedAsSpace);
>+                m_runWidthSoFar += applyFontTransforms(glyphBuffer, m_run.ltr(), lastGlyphCount, lastFontData, m_typesettingFeatures, previousCharacter, transformsType == TransformsType::Forced, charactersTreatedAsSpace);
>                 if (glyphBuffer)
>                     glyphBuffer->shrink(lastGlyphCount);
>             }
>@@ -427,7 +423,7 @@ inline unsigned WidthIterator::advanceInternal(TextIterator& textIterator, Glyph
> 
>     auto transformsType = shouldApplyFontTransforms(glyphBuffer, lastGlyphCount, previousCharacter);
>     if (transformsType != TransformsType::None) {
>-        m_runWidthSoFar += applyFontTransforms(glyphBuffer, m_run.ltr(), lastGlyphCount, lastFontData, *this, m_typesettingFeatures, transformsType == TransformsType::Forced, charactersTreatedAsSpace);
>+        m_runWidthSoFar += applyFontTransforms(glyphBuffer, m_run.ltr(), lastGlyphCount, lastFontData, m_typesettingFeatures, previousCharacter, transformsType == TransformsType::Forced, charactersTreatedAsSpace);
>         if (glyphBuffer)
>             glyphBuffer->shrink(lastGlyphCount);
>     }
>diff --git a/Source/WebCore/platform/graphics/WidthIterator.h b/Source/WebCore/platform/graphics/WidthIterator.h
>index d5fe258c9c0905cd2f6e906b0d362b704de56be5..61d30471c69d6900ab0c421d00b99fc6f0a0eb90 100644
>--- a/Source/WebCore/platform/graphics/WidthIterator.h
>+++ b/Source/WebCore/platform/graphics/WidthIterator.h
>@@ -35,6 +35,9 @@ class GlyphBuffer;
> class Font;
> class TextRun;
> struct GlyphData;
>+struct OriginalAdvancesForCharacterTreatedAsSpace;
>+
>+typedef Vector<std::pair<int, OriginalAdvancesForCharacterTreatedAsSpace>, 64> CharactersTreatedAsSpace;
> 
> struct WidthIterator {
>     WTF_MAKE_FAST_ALLOCATED;
>@@ -90,6 +93,7 @@ private:
> 
>     enum class TransformsType { None, Forced, NotForced };
>     TransformsType shouldApplyFontTransforms(const GlyphBuffer*, int lastGlyphCount, UChar32 previousCharacter) const;
>+    float applyFontTransforms(GlyphBuffer*, bool ltr, int& lastGlyphCount, const Font*, TypesettingFeatures, UChar32 previousCharacter, bool force, CharactersTreatedAsSpace&);
> 
>     TypesettingFeatures m_typesettingFeatures;
>     HashSet<const Font*>* m_fallbackFonts;
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 9672cc0a3f21bce818aae7f9f7e70e43ea4eadb0..530290fb938112ac0766ed9950c3c14cb0cc14b4 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,13 @@
>+2015-09-09  Myles C. Maxfield  <mmaxfield@apple.com>
>+
>+        ASSERTION FAILED: typesettingFeatures & (Kerning | Ligatures) in WebCore::applyFontTransforms
>+        https://bugs.webkit.org/show_bug.cgi?id=146194
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * fast/text/softbank-emoji-no-ligatures-nor-kerning-expected.html: Added
>+        * fast/text/softbank-emoji-no-ligatures-nor-kerning.html: Added
>+
> 2015-09-09  Alexey Proskuryakov  <ap@apple.com>
> 
>         fast/dom/rtl-scroll-to-leftmost-and-resize.html is a flaky timeout - IPC drops messages
>diff --git a/LayoutTests/fast/text/softbank-emoji-no-ligatures-nor-kerning-expected.txt b/LayoutTests/fast/text/softbank-emoji-no-ligatures-nor-kerning-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..ae983dbe1b0b661a8e08a676bc2f7101a04f1e37
>--- /dev/null
>+++ b/LayoutTests/fast/text/softbank-emoji-no-ligatures-nor-kerning-expected.txt
>@@ -0,0 +1,2 @@
>+This test makes sure there is no ASSERT when laying out SoftBank emoji symbols without kerning or ligatures. The test passes if there is no ASSERT.
>+î
>diff --git a/LayoutTests/fast/text/softbank-emoji-no-ligatures-nor-kerning.html b/LayoutTests/fast/text/softbank-emoji-no-ligatures-nor-kerning.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..18cf17f37c1bee202bcaee4f93305f1d3d57befe
>--- /dev/null
>+++ b/LayoutTests/fast/text/softbank-emoji-no-ligatures-nor-kerning.html
>@@ -0,0 +1,14 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<script>
>+if (window.testRunner)
>+    testRunner.dumpAsText();
>+</script>
>+</head>
>+<body>
>+This test makes sure there is no ASSERT when laying out SoftBank emoji symbols without kerning or ligatures.
>+The test passes if there is no ASSERT.
>+<div style="text-rendering: optimizeSpeed;">&#xE001;</div>
>+</body>
>+</html>

Flags:

dino: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 146194: 255326 | 260871 | 260874