Make InlineTextBox::createTextRun() take a const lvalue reference String
https://bugs.webkit.org/show_bug.cgi?id=184182
Patch by Said Abou-Hallawa <sabouhallawa@apple.com> on 2018-04-09
Reviewed by Zalan Bujtas.
InlineTextBox::createTextRun() takes a non-const lvalue reference String.
It is tempting to change the signature of this method to take a const lvalue
reference. But this was done intentionally. TextRun is effectively a StringView:
it does not own the passed string. Having the argument a non-const lvalue
reference makes the compiler prevent calls like createTextRun("abc").
To have a better way to express the lifetime of TextRun, this patch does
the following:
-- It makes TextRun::m_text of type String instead of StringView.
-- It adds a new constructor which takes const String&. This constructor
will addRef the underlying StringImpl when assigning it to m_text.
-- It keeps the constructor which takes a StringView. The caller of this
constructor still has to make sure the underlying String outlives the
TextRun. To avoid copying the underlying buffer of the StringView, we
will not use StringView::toString(). Instead we will use
StringView::toStringWithoutCopying() which makes the returned String
accesses the same buffer the StringView uses. In this case, the returned
String is effectively a StringView.
* page/DebugPageOverlays.cpp:
(WebCore::drawRightAlignedText):
* platform/graphics/TextRun.cpp:
* platform/graphics/TextRun.h:
(WebCore::TextRun::TextRun):
(WebCore::TextRun::subRun const):
(WebCore::TextRun::length const):
(WebCore::TextRun::setText):
(WebCore::TextRun::string const): Deleted.
* rendering/InlineTextBox.cpp:
(WebCore::InlineTextBox::localSelectionRect const):
(WebCore::InlineTextBox::paint):
(WebCore::InlineTextBox::paintPlatformDocumentMarker):
(WebCore::InlineTextBox::paintMarkedTextBackground):
(WebCore::InlineTextBox::paintMarkedTextForeground):
(WebCore::InlineTextBox::paintMarkedTextDecoration):
(WebCore::InlineTextBox::offsetForPosition const):
(WebCore::InlineTextBox::positionForOffset const):
(WebCore::InlineTextBox::createTextRun const):
There is no need for this function to take a String argument anymore. The
reason for passing the String was to guarantee its lifetime by keeping
a copy of it in the caller side. Now there is no need for that. The TextRun
itself will keep this copy.
* rendering/InlineTextBox.h:
* rendering/RenderText.cpp:
(WebCore::RenderText::computeCanUseSimplifiedTextMeasuring const):
RenderText::text() returns StringImpl. The compiler wants us to be more
explicit about which constructor of TextRun to call.
* rendering/SimpleLineLayout.cpp:
(WebCore::SimpleLineLayout::canUseForFontAndText):
* rendering/SimpleLineLayoutTextFragmentIterator.cpp:
(WebCore::SimpleLineLayout::TextFragmentIterator::Style::Style):
RenderStyle::hyphenString() returns an AtomicString.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@230452 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 78aad34..5b13541 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,66 @@
+2018-04-09 Said Abou-Hallawa <sabouhallawa@apple.com>
+
+ Make InlineTextBox::createTextRun() take a const lvalue reference String
+ https://bugs.webkit.org/show_bug.cgi?id=184182
+
+ Reviewed by Zalan Bujtas.
+
+ InlineTextBox::createTextRun() takes a non-const lvalue reference String.
+ It is tempting to change the signature of this method to take a const lvalue
+ reference. But this was done intentionally. TextRun is effectively a StringView:
+ it does not own the passed string. Having the argument a non-const lvalue
+ reference makes the compiler prevent calls like createTextRun("abc").
+
+ To have a better way to express the lifetime of TextRun, this patch does
+ the following:
+
+ -- It makes TextRun::m_text of type String instead of StringView.
+ -- It adds a new constructor which takes const String&. This constructor
+ will addRef the underlying StringImpl when assigning it to m_text.
+ -- It keeps the constructor which takes a StringView. The caller of this
+ constructor still has to make sure the underlying String outlives the
+ TextRun. To avoid copying the underlying buffer of the StringView, we
+ will not use StringView::toString(). Instead we will use
+ StringView::toStringWithoutCopying() which makes the returned String
+ accesses the same buffer the StringView uses. In this case, the returned
+ String is effectively a StringView.
+
+ * page/DebugPageOverlays.cpp:
+ (WebCore::drawRightAlignedText):
+ * platform/graphics/TextRun.cpp:
+ * platform/graphics/TextRun.h:
+ (WebCore::TextRun::TextRun):
+ (WebCore::TextRun::subRun const):
+ (WebCore::TextRun::length const):
+ (WebCore::TextRun::setText):
+ (WebCore::TextRun::string const): Deleted.
+ * rendering/InlineTextBox.cpp:
+ (WebCore::InlineTextBox::localSelectionRect const):
+ (WebCore::InlineTextBox::paint):
+ (WebCore::InlineTextBox::paintPlatformDocumentMarker):
+ (WebCore::InlineTextBox::paintMarkedTextBackground):
+ (WebCore::InlineTextBox::paintMarkedTextForeground):
+ (WebCore::InlineTextBox::paintMarkedTextDecoration):
+ (WebCore::InlineTextBox::offsetForPosition const):
+ (WebCore::InlineTextBox::positionForOffset const):
+ (WebCore::InlineTextBox::createTextRun const):
+ There is no need for this function to take a String argument anymore. The
+ reason for passing the String was to guarantee its lifetime by keeping
+ a copy of it in the caller side. Now there is no need for that. The TextRun
+ itself will keep this copy.
+
+ * rendering/InlineTextBox.h:
+ * rendering/RenderText.cpp:
+ (WebCore::RenderText::computeCanUseSimplifiedTextMeasuring const):
+ RenderText::text() returns StringImpl. The compiler wants us to be more
+ explicit about which constructor of TextRun to call.
+
+ * rendering/SimpleLineLayout.cpp:
+ (WebCore::SimpleLineLayout::canUseForFontAndText):
+ * rendering/SimpleLineLayoutTextFragmentIterator.cpp:
+ (WebCore::SimpleLineLayout::TextFragmentIterator::Style::Style):
+ RenderStyle::hyphenString() returns an AtomicString.
+
2018-04-09 Michael Catanzaro <mcatanzaro@igalia.com>
Unreviewed, rolling out r230390.
diff --git a/Source/WebCore/page/DebugPageOverlays.cpp b/Source/WebCore/page/DebugPageOverlays.cpp
index f70e7cc..0255463 100644
--- a/Source/WebCore/page/DebugPageOverlays.cpp
+++ b/Source/WebCore/page/DebugPageOverlays.cpp
@@ -169,7 +169,7 @@
float textGap = 10;
float textBaselineFromTop = 14;
- TextRun textRun = TextRun(StringView(text));
+ TextRun textRun = TextRun(text);
context.setFillColor(Color::transparent);
float textWidth = context.drawText(font, textRun, { });
context.setFillColor(Color::black);
diff --git a/Source/WebCore/platform/graphics/TextRun.cpp b/Source/WebCore/platform/graphics/TextRun.cpp
index 26d400e..f9c6586 100644
--- a/Source/WebCore/platform/graphics/TextRun.cpp
+++ b/Source/WebCore/platform/graphics/TextRun.cpp
@@ -29,7 +29,7 @@
namespace WebCore {
struct ExpectedTextRunSize {
- StringView text;
+ String text;
unsigned integer1;
float float1;
float float2;
diff --git a/Source/WebCore/platform/graphics/TextRun.h b/Source/WebCore/platform/graphics/TextRun.h
index d2b5c1fc2..b51e7a5 100644
--- a/Source/WebCore/platform/graphics/TextRun.h
+++ b/Source/WebCore/platform/graphics/TextRun.h
@@ -2,7 +2,7 @@
* Copyright (C) 2000 Lars Knoll (knoll@kde.org)
* (C) 2000 Antti Koivisto (koivisto@kde.org)
* (C) 2000 Dirk Mueller (mueller@kde.org)
- * Copyright (C) 2003, 2006, 2007, 2011 Apple Inc. All rights reserved.
+ * Copyright (C) 2003-2018 Apple Inc. All rights reserved.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Library General Public
@@ -44,7 +44,7 @@
class TextRun {
WTF_MAKE_FAST_ALLOCATED;
public:
- explicit TextRun(StringView text, float xpos = 0, float expansion = 0, ExpansionBehavior expansionBehavior = DefaultExpansion, TextDirection direction = LTR, bool directionalOverride = false, bool characterScanForCodePath = true)
+ explicit TextRun(const String& text, float xpos = 0, float expansion = 0, ExpansionBehavior expansionBehavior = DefaultExpansion, TextDirection direction = LTR, bool directionalOverride = false, bool characterScanForCodePath = true)
: m_text(text)
, m_tabSize(0)
, m_xpos(xpos)
@@ -59,17 +59,21 @@
{
}
+ explicit TextRun(StringView stringView, float xpos = 0, float expansion = 0, ExpansionBehavior expansionBehavior = DefaultExpansion, TextDirection direction = LTR, bool directionalOverride = false, bool characterScanForCodePath = true)
+ : TextRun(stringView.toStringWithoutCopying(), xpos, expansion, expansionBehavior, direction, directionalOverride, characterScanForCodePath)
+ {
+ }
+
TextRun subRun(unsigned startOffset, unsigned length) const
{
ASSERT_WITH_SECURITY_IMPLICATION(startOffset < m_text.length());
- TextRun result = *this;
+ auto result { *this };
- if (is8Bit()) {
+ if (is8Bit())
result.setText(data8(startOffset), length);
- return result;
- }
- result.setText(data16(startOffset), length);
+ else
+ result.setText(data16(startOffset), length);
return result;
}
@@ -82,11 +86,10 @@
bool is8Bit() const { return m_text.is8Bit(); }
unsigned length() const { return m_text.length(); }
- String string() const { return m_text.toString(); }
- void setText(const LChar* c, unsigned len) { m_text = StringView(c, len); }
- void setText(const UChar* c, unsigned len) { m_text = StringView(c, len); }
- void setText(StringView stringView) { m_text = stringView; }
+ void setText(const LChar* text, unsigned length) { setText({ text, length }); }
+ void setText(const UChar* text, unsigned length) { setText({ text, length }); }
+ void setText(StringView text) { m_text = text.toStringWithoutCopying(); }
float horizontalGlyphStretch() const { return m_horizontalGlyphStretch; }
void setHorizontalGlyphStretch(float scale) { m_horizontalGlyphStretch = scale; }
@@ -113,7 +116,7 @@
StringView text() const { return m_text; }
private:
- StringView m_text;
+ String m_text;
unsigned m_tabSize;
diff --git a/Source/WebCore/rendering/InlineTextBox.cpp b/Source/WebCore/rendering/InlineTextBox.cpp
index 8551e2d..11f58b1 100644
--- a/Source/WebCore/rendering/InlineTextBox.cpp
+++ b/Source/WebCore/rendering/InlineTextBox.cpp
@@ -200,8 +200,7 @@
LayoutUnit selectionTop = this->selectionTop();
LayoutUnit selectionHeight = this->selectionHeight();
- auto text = this->text();
- TextRun textRun = createTextRun(text);
+ TextRun textRun = createTextRun();
LayoutRect selectionRect = LayoutRect(LayoutPoint(logicalLeft(), selectionTop), LayoutSize(m_logicalWidth, selectionHeight));
// Avoid measuring the text when the entire line box is selected as an optimization.
@@ -570,8 +569,7 @@
// Paint decorations
TextDecoration textDecorations = lineStyle.textDecorationsInEffect();
if (textDecorations != TextDecorationNone && paintInfo.phase != PaintPhaseSelection) {
- auto text = this->text();
- TextRun textRun = createTextRun(text);
+ TextRun textRun = createTextRun();
unsigned length = textRun.length();
if (m_truncation != cNoTruncation)
length = m_truncation;
@@ -676,8 +674,7 @@
int deltaY = renderer().style().isFlippedLinesWritingMode() ? selectionBottom() - logicalBottom() : logicalTop() - selectionTop();
int selHeight = selectionHeight();
FloatPoint startPoint(boxOrigin.x(), boxOrigin.y() - deltaY);
- auto text = this->text();
- TextRun run = createTextRun(text);
+ TextRun run = createTextRun();
LayoutRect selectionRect = LayoutRect(startPoint, FloatSize(0, selHeight));
lineFont().adjustSelectionRectForText(run, selectionRect, markedText.startOffset, markedText.endOffset);
@@ -974,8 +971,7 @@
// Note that if the text is truncated, we let the thing being painted in the truncation
// draw its own highlight.
- auto text = this->text();
- TextRun textRun = createTextRun(text);
+ TextRun textRun = createTextRun();
const RootInlineBox& rootBox = root();
LayoutUnit selectionBottom = rootBox.selectionBottom();
@@ -1022,8 +1018,7 @@
context.setAlpha(markedText.style.alpha);
}
// TextPainter wants the box rectangle and text origin of the entire line box.
- auto text = this->text();
- textPainter.paintRange(createTextRun(text), boxRect, textOriginFromBoxRect(boxRect), markedText.startOffset, markedText.endOffset);
+ textPainter.paintRange(createTextRun(), boxRect, textOriginFromBoxRect(boxRect), markedText.startOffset, markedText.endOffset);
}
void InlineTextBox::paintMarkedTextDecoration(GraphicsContext& context, const FloatRect& boxRect, const FloatRect& clipOutRect, const StyledMarkedText& markedText)
@@ -1045,8 +1040,7 @@
// Note that if the text is truncated, we let the thing being painted in the truncation
// draw its own decoration.
- auto text = this->text();
- TextRun textRun = createTextRun(text);
+ TextRun textRun = createTextRun();
// Avoid measuring the text when the entire line box is selected as an optimization.
FloatRect snappedSelectionRect = boxRect;
@@ -1190,8 +1184,7 @@
return isLeftToRightDirection() ? 0 : len();
bool ignoreCombinedText = true;
bool ignoreHyphen = true;
- auto text = this->text(ignoreCombinedText, ignoreHyphen);
- return lineFont().offsetForPosition(createTextRun(text), lineOffset - logicalLeft(), includePartialGlyphs);
+ return lineFont().offsetForPosition(createTextRun(ignoreCombinedText, ignoreHyphen), lineOffset - logicalLeft(), includePartialGlyphs);
}
float InlineTextBox::positionForOffset(unsigned offset) const
@@ -1216,16 +1209,15 @@
LayoutRect selectionRect = LayoutRect(logicalLeft(), 0, 0, 0);
bool ignoreCombinedText = true;
bool ignoreHyphen = true;
- auto text = this->text(ignoreCombinedText, ignoreHyphen);
- TextRun textRun = createTextRun(text);
+ TextRun textRun = createTextRun(ignoreCombinedText, ignoreHyphen);
lineFont().adjustSelectionRectForText(textRun, selectionRect, startOffset, endOffset);
return snapRectToDevicePixelsWithWritingDirection(selectionRect, renderer().document().deviceScaleFactor(), textRun.ltr()).maxX();
}
-TextRun InlineTextBox::createTextRun(String& string) const
+TextRun InlineTextBox::createTextRun(bool ignoreCombinedText, bool ignoreHyphen) const
{
const auto& style = lineStyle();
- TextRun textRun { string, textPos(), expansion(), expansionBehavior(), direction(), dirOverride() || style.rtlOrdering() == VisualOrder, !renderer().canUseSimpleFontCodePath() };
+ TextRun textRun { text(ignoreCombinedText, ignoreHyphen), textPos(), expansion(), expansionBehavior(), direction(), dirOverride() || style.rtlOrdering() == VisualOrder, !renderer().canUseSimpleFontCodePath() };
textRun.setTabSize(!style.collapseWhiteSpace(), style.tabSize());
return textRun;
}
diff --git a/Source/WebCore/rendering/InlineTextBox.h b/Source/WebCore/rendering/InlineTextBox.h
index c2374af..4793ca8 100644
--- a/Source/WebCore/rendering/InlineTextBox.h
+++ b/Source/WebCore/rendering/InlineTextBox.h
@@ -185,7 +185,7 @@
const FontCascade& lineFont() const;
String text(bool ignoreCombinedText = false, bool ignoreHyphen = false) const; // The effective text for the run.
- TextRun createTextRun(String&) const;
+ TextRun createTextRun(bool ignoreCombinedText = false, bool ignoreHyphen = false) const;
ExpansionBehavior expansionBehavior() const;
diff --git a/Source/WebCore/rendering/RenderText.cpp b/Source/WebCore/rendering/RenderText.cpp
index e55be1e..8fa51e2 100644
--- a/Source/WebCore/rendering/RenderText.cpp
+++ b/Source/WebCore/rendering/RenderText.cpp
@@ -1243,7 +1243,7 @@
return false;
// Additional check on the font codepath.
- TextRun run(text());
+ TextRun run(m_text);
run.setCharacterScanForCodePath(false);
if (font.codePath(run) != FontCascade::Simple)
return false;
diff --git a/Source/WebCore/rendering/SimpleLineLayout.cpp b/Source/WebCore/rendering/SimpleLineLayout.cpp
index 8777dc0..7bd5ff6 100644
--- a/Source/WebCore/rendering/SimpleLineLayout.cpp
+++ b/Source/WebCore/rendering/SimpleLineLayout.cpp
@@ -181,7 +181,7 @@
// No need to check the code path at this point. We already know it can't be simple.
SET_REASON_AND_RETURN_IF_NEEDED(FlowHasComplexFontCodePath, reasons, includeReasons);
} else {
- TextRun run(textRenderer.text());
+ TextRun run(String(textRenderer.text()));
run.setCharacterScanForCodePath(false);
if (style.fontCascade().codePath(run) != FontCascade::Simple)
SET_REASON_AND_RETURN_IF_NEEDED(FlowHasComplexFontCodePath, reasons, includeReasons);
diff --git a/Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.cpp b/Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.cpp
index b9d0bb7..329024d 100644
--- a/Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.cpp
+++ b/Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.cpp
@@ -49,7 +49,7 @@
, wordSpacing(font.wordSpacing())
, tabWidth(collapseWhitespace ? 0 : style.tabSize())
, shouldHyphenate(style.hyphens() == HyphensAuto && canHyphenate(style.locale()))
- , hyphenStringWidth(shouldHyphenate ? (useSimplifiedTextMeasuring ? font.widthForSimpleText(style.hyphenString()) : font.width(TextRun(style.hyphenString()))) : 0)
+ , hyphenStringWidth(shouldHyphenate ? (useSimplifiedTextMeasuring ? font.widthForSimpleText(style.hyphenString()) : font.width(TextRun(String(style.hyphenString())))) : 0)
, hyphenLimitBefore(style.hyphenationLimitBefore() < 0 ? 2 : style.hyphenationLimitBefore())
, hyphenLimitAfter(style.hyphenationLimitAfter() < 0 ? 2 : style.hyphenationLimitAfter())
, locale(style.locale())
