Diff
Modified: trunk/Source/WebCore/ChangeLog (230451 => 230452)
--- trunk/Source/WebCore/ChangeLog 2018-04-09 21:09:54 UTC (rev 230451)
+++ trunk/Source/WebCore/ChangeLog 2018-04-09 23:02:29 UTC (rev 230452)
@@ -1,3 +1,66 @@
+2018-04-09 Said Abou-Hallawa <sabouhall...@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 <mcatanz...@igalia.com>
Unreviewed, rolling out r230390.
Modified: trunk/Source/WebCore/page/DebugPageOverlays.cpp (230451 => 230452)
--- trunk/Source/WebCore/page/DebugPageOverlays.cpp 2018-04-09 21:09:54 UTC (rev 230451)
+++ trunk/Source/WebCore/page/DebugPageOverlays.cpp 2018-04-09 23:02:29 UTC (rev 230452)
@@ -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);
Modified: trunk/Source/WebCore/platform/graphics/TextRun.cpp (230451 => 230452)
--- trunk/Source/WebCore/platform/graphics/TextRun.cpp 2018-04-09 21:09:54 UTC (rev 230451)
+++ trunk/Source/WebCore/platform/graphics/TextRun.cpp 2018-04-09 23:02:29 UTC (rev 230452)
@@ -29,7 +29,7 @@
namespace WebCore {
struct ExpectedTextRunSize {
- StringView text;
+ String text;
unsigned integer1;
float float1;
float float2;
Modified: trunk/Source/WebCore/platform/graphics/TextRun.h (230451 => 230452)
--- trunk/Source/WebCore/platform/graphics/TextRun.h 2018-04-09 21:09:54 UTC (rev 230451)
+++ trunk/Source/WebCore/platform/graphics/TextRun.h 2018-04-09 23:02:29 UTC (rev 230452)
@@ -2,7 +2,7 @@
* Copyright (C) 2000 Lars Knoll (kn...@kde.org)
* (C) 2000 Antti Koivisto (koivi...@kde.org)
* (C) 2000 Dirk Mueller (muel...@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;
Modified: trunk/Source/WebCore/rendering/InlineTextBox.cpp (230451 => 230452)
--- trunk/Source/WebCore/rendering/InlineTextBox.cpp 2018-04-09 21:09:54 UTC (rev 230451)
+++ trunk/Source/WebCore/rendering/InlineTextBox.cpp 2018-04-09 23:02:29 UTC (rev 230452)
@@ -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;
}
Modified: trunk/Source/WebCore/rendering/InlineTextBox.h (230451 => 230452)
--- trunk/Source/WebCore/rendering/InlineTextBox.h 2018-04-09 21:09:54 UTC (rev 230451)
+++ trunk/Source/WebCore/rendering/InlineTextBox.h 2018-04-09 23:02:29 UTC (rev 230452)
@@ -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;
Modified: trunk/Source/WebCore/rendering/RenderText.cpp (230451 => 230452)
--- trunk/Source/WebCore/rendering/RenderText.cpp 2018-04-09 21:09:54 UTC (rev 230451)
+++ trunk/Source/WebCore/rendering/RenderText.cpp 2018-04-09 23:02:29 UTC (rev 230452)
@@ -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;
Modified: trunk/Source/WebCore/rendering/SimpleLineLayout.cpp (230451 => 230452)
--- trunk/Source/WebCore/rendering/SimpleLineLayout.cpp 2018-04-09 21:09:54 UTC (rev 230451)
+++ trunk/Source/WebCore/rendering/SimpleLineLayout.cpp 2018-04-09 23:02:29 UTC (rev 230452)
@@ -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);
Modified: trunk/Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.cpp (230451 => 230452)
--- trunk/Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.cpp 2018-04-09 21:09:54 UTC (rev 230451)
+++ trunk/Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.cpp 2018-04-09 23:02:29 UTC (rev 230452)
@@ -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())
