SVG TextRuns do not always get RenderingContexts

https://bugs.webkit.org/show_bug.cgi?id=133198

Reviewed by Simon Fraser.

Source/WebCore:
There were a couple places in RenderListMarker and RenderMenuList that were
implicitly creating TextRuns by passing a String to a function which
expected a TextRun. Because TextRun has a constructor which takes a single
String and isn't marked explicit, TextRuns were being created without
any of the associated code that initializes the TextRun (such as creating
a RenderingContext if necessary).

This patch marks the aforementioned constructor as "explicit" to discourage
such behavior in the future.

Test: svg/custom/list-items-with-svg-font-family.html

* platform/graphics/TextRun.h:
(WebCore::TextRun::TextRun): Mark constructors as explicit.
* rendering/RenderListMarker.cpp:
(WebCore::RenderListMarker::computePreferredLogicalWidths): Call
RenderBlock::constructTextRun.
(WebCore::RenderListMarker::getRelativeMarkerRect): Ditto.
* rendering/RenderMenuList.cpp:
(RenderMenuList::updateOptionsWidth): Ditto.

LayoutTests:
See per-file comments.

* svg/custom/list-items-with-svg-font-family-expected.txt: Added.
* svg/custom/list-items-with-svg-font-family.html: Added. Make sure there is
no crash when styling list elements with SVG fonts.

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

diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 0495ee2..838d509 100644 (file)
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,16 @@
+2014-05-27  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        SVG TextRuns do not always get RenderingContexts
+        https://bugs.webkit.org/show_bug.cgi?id=133198
+
+        Reviewed by Simon Fraser.
+
+        See per-file comments.
+
+        * svg/custom/list-items-with-svg-font-family-expected.txt: Added.
+        * svg/custom/list-items-with-svg-font-family.html: Added. Make sure there is
+        no crash when styling list elements with SVG fonts.
+
 2014-05-23  Myles C. Maxfield  <mmaxfield@apple.com>
 
         Caret's screen position does not update during an overflow scroll

diff --git a/LayoutTests/svg/custom/list-items-with-svg-font-family-expected.txt b/LayoutTests/svg/custom/list-items-with-svg-font-family-expected.txt
new file mode 100644 (file)
index 0000000..a7a5683
--- /dev/null
+++ b/LayoutTests/svg/custom/list-items-with-svg-font-family-expected.txt
@@ -0,0 +1,5 @@
+This test makes sure that styling list elements with an SVG font does not cause a crash. 
+This test is successful if there is no crash.
+element
+element
+element

diff --git a/LayoutTests/svg/custom/list-items-with-svg-font-family.html b/LayoutTests/svg/custom/list-items-with-svg-font-family.html
new file mode 100644 (file)
index 0000000..b4af0eb
--- /dev/null
+++ b/LayoutTests/svg/custom/list-items-with-svg-font-family.html
@@ -0,0 +1,28 @@
+<style>
+@font-face {
+    font-family: 'SVGraffiti';
+    src: url("resources/graffiti.svg#SVGraffiti") format(svg)
+}
+li.a {
+    font-size: 30px;
+    font-family: SVGraffiti;
+    list-style-type: lower-roman;
+}
+li.b {
+    font-size: 30px;
+    font-family: SVGraffiti;
+    list-style-type: asterisks;
+}
+</style>
+This test makes sure that styling list elements with an SVG font does not cause a crash.
+<br>
+This test is successful if there is no crash.
+<ol>
+<li class="a">element</li>
+<li class="a">element</li>
+<li class="b">element</li>
+</ol>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+</script>

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 8c5efd4..c89c1eb 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,31 @@
+2014-05-27  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        SVG TextRuns do not always get RenderingContexts
+        https://bugs.webkit.org/show_bug.cgi?id=133198
+
+        Reviewed by Simon Fraser.
+
+        There were a couple places in RenderListMarker and RenderMenuList that were
+        implicitly creating TextRuns by passing a String to a function which
+        expected a TextRun. Because TextRun has a constructor which takes a single
+        String and isn't marked explicit, TextRuns were being created without
+        any of the associated code that initializes the TextRun (such as creating
+        a RenderingContext if necessary).
+
+        This patch marks the aforementioned constructor as "explicit" to discourage
+        such behavior in the future.
+
+        Test: svg/custom/list-items-with-svg-font-family.html
+
+        * platform/graphics/TextRun.h:
+        (WebCore::TextRun::TextRun): Mark constructors as explicit.
+        * rendering/RenderListMarker.cpp:
+        (WebCore::RenderListMarker::computePreferredLogicalWidths): Call
+        RenderBlock::constructTextRun.
+        (WebCore::RenderListMarker::getRelativeMarkerRect): Ditto.
+        * rendering/RenderMenuList.cpp:
+        (RenderMenuList::updateOptionsWidth): Ditto.
+
 2014-05-23  Myles C. Maxfield  <mmaxfield@apple.com>
 
         Caret's screen position does not update during an overflow scroll

diff --git a/Source/WebCore/platform/graphics/TextRun.h b/Source/WebCore/platform/graphics/TextRun.h
index 5d0c87d..4f3bf3b 100644 (file)
--- a/Source/WebCore/platform/graphics/TextRun.h
+++ b/Source/WebCore/platform/graphics/TextRun.h
@@ -101,7 +101,7 @@ public:
         m_data.characters16 = c;
     }
     
-    TextRun(const String& s, float xpos = 0, float expansion = 0, ExpansionBehavior expansionBehavior = AllowTrailingExpansion | ForbidLeadingExpansion, TextDirection direction = LTR, bool directionalOverride = false, bool characterScanForCodePath = true, RoundingHacks roundingHacks = RunRounding | WordRounding)
+    explicit TextRun(const String& s, float xpos = 0, float expansion = 0, ExpansionBehavior expansionBehavior = AllowTrailingExpansion | ForbidLeadingExpansion, TextDirection direction = LTR, bool directionalOverride = false, bool characterScanForCodePath = true, RoundingHacks roundingHacks = RunRounding | WordRounding)
         : m_charactersLength(s.length())
         , m_len(s.length())
         , m_xpos(xpos)
@@ -126,7 +126,7 @@ public:
         }
     }
 
-    TextRun(StringView s, float xpos = 0, float expansion = 0, ExpansionBehavior expansionBehavior = AllowTrailingExpansion | ForbidLeadingExpansion, TextDirection direction = LTR, bool directionalOverride = false, bool characterScanForCodePath = true, RoundingHacks roundingHacks = RunRounding | WordRounding)
+    explicit TextRun(StringView s, float xpos = 0, float expansion = 0, ExpansionBehavior expansionBehavior = AllowTrailingExpansion | ForbidLeadingExpansion, TextDirection direction = LTR, bool directionalOverride = false, bool characterScanForCodePath = true, RoundingHacks roundingHacks = RunRounding | WordRounding)
         : m_charactersLength(s.length())
         , m_len(s.length())
         , m_xpos(xpos)

diff --git a/Source/WebCore/rendering/RenderListMarker.cpp b/Source/WebCore/rendering/RenderListMarker.cpp
index b5b73c6..672f390 100644 (file)
--- a/Source/WebCore/rendering/RenderListMarker.cpp
+++ b/Source/WebCore/rendering/RenderListMarker.cpp
@@ -1536,8 +1536,10 @@ void RenderListMarker::computePreferredLogicalWidths()
         case NoneListStyle:
             break;
         case Asterisks:
-        case Footnotes:
-            logicalWidth = font.width(m_text); // no suffix for these types
+        case Footnotes: {
+            TextRun run = RenderBlock::constructTextRun(this, font, m_text, style(), TextRun::AllowTrailingExpansion | TextRun::ForbidLeadingExpansion, DefaultTextRunFlags);
+            logicalWidth = font.width(run); // no suffix for these types
+        }
             break;
         case Circle:
         case Disc:
@@ -1622,7 +1624,8 @@ void RenderListMarker::computePreferredLogicalWidths()
             if (m_text.isEmpty())
                 logicalWidth = 0;
             else {
-                LayoutUnit itemWidth = font.width(m_text);
+                TextRun run = RenderBlock::constructTextRun(this, font, m_text, style(), TextRun::AllowTrailingExpansion | TextRun::ForbidLeadingExpansion, DefaultTextRunFlags);
+                LayoutUnit itemWidth = font.width(run);
                 UChar suffixSpace[2] = { listMarkerSuffix(type, m_listItem.value()), ' ' };
                 LayoutUnit suffixSpaceWidth = font.width(RenderBlock::constructTextRun(this, font, suffixSpace, 2, style()));
                 logicalWidth = itemWidth + suffixSpaceWidth;
@@ -1754,7 +1757,8 @@ IntRect RenderListMarker::getRelativeMarkerRect()
         case Asterisks:
         case Footnotes: {
             const Font& font = style().font();
-            relativeRect = IntRect(0, 0, font.width(m_text), font.fontMetrics().height());
+            TextRun run = RenderBlock::constructTextRun(this, font, m_text, style(), TextRun::AllowTrailingExpansion | TextRun::ForbidLeadingExpansion, DefaultTextRunFlags);
+            relativeRect = IntRect(0, 0, font.width(run), font.fontMetrics().height());
             break;
         }
         case Disc:
@@ -1847,7 +1851,8 @@ IntRect RenderListMarker::getRelativeMarkerRect()
             if (m_text.isEmpty())
                 return IntRect();
             const Font& font = style().font();
-            int itemWidth = font.width(m_text);
+            TextRun run = RenderBlock::constructTextRun(this, font, m_text, style(), TextRun::AllowTrailingExpansion | TextRun::ForbidLeadingExpansion, DefaultTextRunFlags);
+            int itemWidth = font.width(run);
             UChar suffixSpace[2] = { listMarkerSuffix(type, m_listItem.value()), ' ' };
             int suffixSpaceWidth = font.width(RenderBlock::constructTextRun(this, font, suffixSpace, 2, style()));
             relativeRect = IntRect(0, 0, itemWidth + suffixSpaceWidth, font.fontMetrics().height());

diff --git a/Source/WebCore/rendering/RenderMenuList.cpp b/Source/WebCore/rendering/RenderMenuList.cpp
index 11c47e9..e6a8506 100644 (file)
--- a/Source/WebCore/rendering/RenderMenuList.cpp
+++ b/Source/WebCore/rendering/RenderMenuList.cpp
@@ -224,11 +224,17 @@ void RenderMenuList::updateOptionsWidth()
             float optionWidth = 0;
             if (RenderStyle* optionStyle = element->renderStyle())
                 optionWidth += minimumValueForLength(optionStyle->textIndent(), 0);
-            if (!text.isEmpty())
-                optionWidth += style().font().width(text);
+            if (!text.isEmpty()) {
+                const Font& font = style().font();
+                TextRun run = RenderBlock::constructTextRun(this, font, text, style(), TextRun::AllowTrailingExpansion | TextRun::ForbidLeadingExpansion, DefaultTextRunFlags);
+                optionWidth += font.width(run);
+            }
             maxOptionWidth = std::max(maxOptionWidth, optionWidth);
-        } else if (!text.isEmpty())
-            maxOptionWidth = std::max(maxOptionWidth, style().font().width(text));
+        } else if (!text.isEmpty()) {
+            const Font& font = style().font();
+            TextRun run = RenderBlock::constructTextRun(this, font, text, style(), TextRun::AllowTrailingExpansion | TextRun::ForbidLeadingExpansion, DefaultTextRunFlags);
+            maxOptionWidth = std::max(maxOptionWidth, font.width(run));
+        }
     }
 
     int width = static_cast<int>(ceilf(maxOptionWidth));