Diff
Modified: trunk/LayoutTests/ChangeLog (265412 => 265413)
--- trunk/LayoutTests/ChangeLog 2020-08-09 04:27:09 UTC (rev 265412)
+++ trunk/LayoutTests/ChangeLog 2020-08-09 05:14:40 UTC (rev 265413)
@@ -1,3 +1,12 @@
+2020-08-08 Myles C. Maxfield <[email protected]>
+
+ Make GlyphBuffers required in the fast text codepath
+ https://bugs.webkit.org/show_bug.cgi?id=215052
+
+ Reviewed by Darin Adler.
+
+ * platform/win/fast/events/selectstart-by-drag-expected.txt: Test progressed on Windows.
+
2020-08-08 Wenson Hsieh <[email protected]>
[ iOS wk2 ] editing/pasteboard/paste-without-nesting.html is a flaky failure
Deleted: trunk/LayoutTests/platform/win/fast/events/selectstart-by-drag-expected.txt (265412 => 265413)
--- trunk/LayoutTests/platform/win/fast/events/selectstart-by-drag-expected.txt 2020-08-09 04:27:09 UTC (rev 265412)
+++ trunk/LayoutTests/platform/win/fast/events/selectstart-by-drag-expected.txt 2020-08-09 05:14:40 UTC (rev 265413)
@@ -1,14 +0,0 @@
-This test ensures selectstart is fired exactly once when selecting text by a mouse drag.
-
-Initial state: PASS
-Mouse down: FAIL - expected selection to be caret but was range
-Moving slightly to the right: FAIL - expected selection to be caret but was range
-Moving slightly to the left: FAIL - expected selection to be caret but was range
-Moving to the right: PASS
-Moving further to the right: PASS
-Moving back to the left: FAIL - expected selection to be caret but was range
-Moving to the right again: PASS
-Mouse down on the right: PASS
-Moving to the left: PASS
-Done.
-
Modified: trunk/Source/WebCore/ChangeLog (265412 => 265413)
--- trunk/Source/WebCore/ChangeLog 2020-08-09 04:27:09 UTC (rev 265412)
+++ trunk/Source/WebCore/ChangeLog 2020-08-09 05:14:40 UTC (rev 265413)
@@ -1,5 +1,41 @@
2020-08-08 Myles C. Maxfield <[email protected]>
+ Make GlyphBuffers required in the fast text codepath
+ https://bugs.webkit.org/show_bug.cgi?id=215052
+
+ Reviewed by Darin Adler.
+
+ This is in preparation for https://bugs.webkit.org/show_bug.cgi?id=214769
+ and https://bugs.webkit.org/show_bug.cgi?id=206208.
+
+ Performing shaping affects the width of strings; indeed, that is one of
+ its purposes for existence. Shaping can only happen when we have a GlyphBuffer
+ to shape. We can't just arbitrarily decide to disable shaping for various
+ functions just because those functions don't ever inspect the exact glyphs.
+
+ No new tests. This is a preparation step toward
+ https://bugs.webkit.org/show_bug.cgi?id=214769 and
+ https://bugs.webkit.org/show_bug.cgi?id=206208, and I couldn't come up with a
+ test case that was broken here.
+
+ * platform/graphics/FontCascade.cpp:
+ (WebCore::FontCascade::widthOfTextRange const):
+ (WebCore::FontCascade::widthForSimpleText const):
+ (WebCore::FontCascade::layoutSimpleText const):
+ (WebCore::FontCascade::floatWidthForSimpleText const):
+ (WebCore::FontCascade::adjustSelectionRectForSimpleText const):
+ * platform/graphics/WidthIterator.cpp:
+ (WebCore::WidthIterator::shouldApplyFontTransforms const):
+ (WebCore::WidthIterator::applyFontTransforms):
+ (WebCore::WidthIterator::advanceInternal):
+ (WebCore::WidthIterator::advance):
+ (WebCore::WidthIterator::advanceOneCharacter):
+ * platform/graphics/WidthIterator.h:
+ * rendering/svg/SVGTextMetricsBuilder.cpp:
+ (WebCore::SVGTextMetricsBuilder::advanceSimpleText):
+
+2020-08-08 Myles C. Maxfield <[email protected]>
+
WidthIterator::m_finalRoundingWidth is always 0
https://bugs.webkit.org/show_bug.cgi?id=215307
Modified: trunk/Source/WebCore/platform/graphics/FontCascade.cpp (265412 => 265413)
--- trunk/Source/WebCore/platform/graphics/FontCascade.cpp 2020-08-09 04:27:09 UTC (rev 265412)
+++ trunk/Source/WebCore/platform/graphics/FontCascade.cpp 2020-08-09 05:14:40 UTC (rev 265413)
@@ -373,11 +373,12 @@
totalWidth = complexIterator.runWidthSoFar();
} else {
WidthIterator simpleIterator(*this, run, fallbackFonts);
- simpleIterator.advance(from, nullptr);
+ GlyphBuffer glyphBuffer;
+ simpleIterator.advance(from, glyphBuffer);
offsetBeforeRange = simpleIterator.runWidthSoFar();
- simpleIterator.advance(to, nullptr);
+ simpleIterator.advance(to, glyphBuffer);
offsetAfterRange = simpleIterator.runWidthSoFar();
- simpleIterator.advance(run.length(), nullptr);
+ simpleIterator.advance(run.length(), glyphBuffer);
totalWidth = simpleIterator.runWidthSoFar();
}
@@ -435,7 +436,6 @@
return *cacheEntry;
GlyphBuffer glyphBuffer;
- bool hasKerningOrLigatures = enableKerning() || requiresShaping();
float runWidth = 0;
auto& font = primaryFont();
for (unsigned i = 0; i < text.length(); ++i) {
@@ -442,20 +442,17 @@
auto glyph = glyphDataForCharacter(text[i], false).glyph;
auto glyphWidth = font.widthForGlyph(glyph);
runWidth += glyphWidth;
- if (!hasKerningOrLigatures)
- continue;
glyphBuffer.add(glyph, font, glyphWidth);
}
- if (hasKerningOrLigatures) {
- font.applyTransforms(glyphBuffer, 0, enableKerning(), requiresShaping(), fontDescription().computedLocale());
- // This is needed only to match the result of the slow path. Same glyph widths but different floating point arithmentics can
- // produce different run width.
- float runWidthDifferenceWithTransformApplied = -runWidth;
- for (size_t i = 0; i < glyphBuffer.size(); ++i)
- runWidthDifferenceWithTransformApplied += glyphBuffer.advanceAt(i).width();
- runWidth += runWidthDifferenceWithTransformApplied;
- }
+ font.applyTransforms(glyphBuffer, 0, enableKerning(), requiresShaping(), fontDescription().computedLocale());
+ // This is needed only to match the result of the slow path.
+ // Same glyph widths but different floating point arithmetic can produce different run width.
+ float runWidthDifferenceWithTransformApplied = -runWidth;
+ for (size_t i = 0; i < glyphBuffer.size(); ++i)
+ runWidthDifferenceWithTransformApplied += glyphBuffer.advanceAt(i).width();
+ runWidth += runWidthDifferenceWithTransformApplied;
+
if (cacheEntry)
*cacheEntry = runWidth;
return runWidth;
@@ -1376,9 +1373,9 @@
// FIXME: Using separate glyph buffers for the prefix and the suffix is incorrect when kerning or
// ligatures are enabled.
GlyphBuffer localGlyphBuffer;
- it.advance(from, &localGlyphBuffer);
+ it.advance(from, localGlyphBuffer);
float beforeWidth = it.runWidthSoFar();
- it.advance(to, &glyphBuffer);
+ it.advance(to, glyphBuffer);
if (glyphBuffer.isEmpty())
return glyphBuffer;
@@ -1387,7 +1384,7 @@
float initialAdvance = 0;
if (run.rtl()) {
- it.advance(run.length(), &localGlyphBuffer);
+ it.advance(run.length(), localGlyphBuffer);
initialAdvance = it.runWidthSoFar() - afterWidth;
} else
initialAdvance = beforeWidth;
@@ -1522,7 +1519,7 @@
{
WidthIterator it(*this, run, fallbackFonts, glyphOverflow);
GlyphBuffer glyphBuffer;
- it.advance(run.length(), (enableKerning() || requiresShaping()) ? &glyphBuffer : nullptr);
+ it.advance(run.length(), glyphBuffer);
if (glyphOverflow) {
glyphOverflow->top = std::max<int>(glyphOverflow->top, ceilf(-it.minGlyphBoundingBoxY()) - (glyphOverflow->computeBounds ? 0 : fontMetrics().ascent()));
@@ -1550,13 +1547,13 @@
{
GlyphBuffer glyphBuffer;
WidthIterator it(*this, run);
- it.advance(from, &glyphBuffer);
+ it.advance(from, glyphBuffer);
float beforeWidth = it.runWidthSoFar();
- it.advance(to, &glyphBuffer);
+ it.advance(to, glyphBuffer);
float afterWidth = it.runWidthSoFar();
if (run.rtl()) {
- it.advance(run.length(), &glyphBuffer);
+ it.advance(run.length(), glyphBuffer);
float totalWidth = it.runWidthSoFar();
selectionRect.move(totalWidth - afterWidth, 0);
} else
Modified: trunk/Source/WebCore/platform/graphics/WidthIterator.cpp (265412 => 265413)
--- trunk/Source/WebCore/platform/graphics/WidthIterator.cpp 2020-08-09 04:27:09 UTC (rev 265412)
+++ trunk/Source/WebCore/platform/graphics/WidthIterator.cpp 2020-08-09 05:14:40 UTC (rev 265413)
@@ -76,29 +76,26 @@
return codepoint >= 0xE001 && codepoint <= 0xE537;
}
-inline auto WidthIterator::shouldApplyFontTransforms(const GlyphBuffer* glyphBuffer, unsigned lastGlyphCount, UChar32 previousCharacter) const -> TransformsType
+inline auto WidthIterator::shouldApplyFontTransforms(const GlyphBuffer& glyphBuffer, unsigned lastGlyphCount, UChar32 previousCharacter) const -> TransformsType
{
- if (glyphBuffer && glyphBuffer->size() == (lastGlyphCount + 1) && isSoftBankEmoji(previousCharacter))
+ if (glyphBuffer.size() == (lastGlyphCount + 1) && isSoftBankEmoji(previousCharacter))
return TransformsType::Forced;
- if (m_run.length() <= 1 || !(m_enableKerning || m_requiresShaping))
+ if (m_run.length() <= 1)
return TransformsType::None;
return TransformsType::NotForced;
}
-inline float WidthIterator::applyFontTransforms(GlyphBuffer* glyphBuffer, bool ltr, unsigned& lastGlyphCount, const Font& font, UChar32 previousCharacter, bool force, CharactersTreatedAsSpace& charactersTreatedAsSpace)
+inline float WidthIterator::applyFontTransforms(GlyphBuffer& glyphBuffer, bool ltr, unsigned& lastGlyphCount, const Font& font, UChar32 previousCharacter, bool force, CharactersTreatedAsSpace& charactersTreatedAsSpace)
{
ASSERT_UNUSED(previousCharacter, shouldApplyFontTransforms(glyphBuffer, lastGlyphCount, previousCharacter) != WidthIterator::TransformsType::None);
- if (!glyphBuffer)
- return 0;
-
- auto glyphBufferSize = glyphBuffer->size();
+ auto glyphBufferSize = glyphBuffer.size();
if (!force && glyphBufferSize <= lastGlyphCount + 1) {
lastGlyphCount = glyphBufferSize;
return 0;
}
- GlyphBufferAdvance* advances = glyphBuffer->advances(0);
+ GlyphBufferAdvance* advances = glyphBuffer.advances(0);
float beforeWidth = 0;
for (unsigned i = lastGlyphCount; i < glyphBufferSize; ++i)
beforeWidth += advances[i].width();
@@ -105,16 +102,16 @@
ASSERT(lastGlyphCount <= glyphBufferSize);
if (!ltr)
- glyphBuffer->reverse(lastGlyphCount, glyphBufferSize - lastGlyphCount);
+ glyphBuffer.reverse(lastGlyphCount, glyphBufferSize - lastGlyphCount);
- font.applyTransforms(*glyphBuffer, lastGlyphCount, m_enableKerning, m_requiresShaping, m_font.fontDescription().computedLocale());
- glyphBufferSize = glyphBuffer->size();
+ font.applyTransforms(glyphBuffer, lastGlyphCount, m_enableKerning, m_requiresShaping, m_font.fontDescription().computedLocale());
+ glyphBufferSize = glyphBuffer.size();
for (unsigned i = lastGlyphCount; i < glyphBufferSize; ++i)
advances[i].setHeight(-advances[i].height());
if (!ltr)
- glyphBuffer->reverse(lastGlyphCount, glyphBufferSize - lastGlyphCount);
+ glyphBuffer.reverse(lastGlyphCount, glyphBufferSize - lastGlyphCount);
// https://bugs.webkit.org/show_bug.cgi?id=206208: This is totally, 100%, furiously, utterly, frustratingly bogus.
// There is absolutely no guarantee that glyph indices before shaping have any relation at all with glyph indices after shaping.
@@ -126,8 +123,8 @@
continue;
const OriginalAdvancesForCharacterTreatedAsSpace& originalAdvances = charactersTreatedAsSpace[i].second;
if (spaceOffset && !originalAdvances.characterIsSpace)
- glyphBuffer->advances(spaceOffset - 1)->setWidth(originalAdvances.advanceBeforeCharacter);
- glyphBuffer->advances(spaceOffset)->setWidth(originalAdvances.advanceAtCharacter);
+ glyphBuffer.advances(spaceOffset - 1)->setWidth(originalAdvances.advanceBeforeCharacter);
+ glyphBuffer.advances(spaceOffset)->setWidth(originalAdvances.advanceAtCharacter);
}
charactersTreatedAsSpace.clear();
@@ -169,7 +166,7 @@
}
template <typename TextIterator>
-inline void WidthIterator::advanceInternal(TextIterator& textIterator, GlyphBuffer* glyphBuffer)
+inline void WidthIterator::advanceInternal(TextIterator& textIterator, GlyphBuffer& glyphBuffer)
{
// The core logic here needs to match SimpleLineLayout::widthForSimpleText()
bool rtl = m_run.rtl();
@@ -185,7 +182,7 @@
const Font& primaryFont = m_font.primaryFont();
const Font* lastFontData = &primaryFont;
- unsigned lastGlyphCount = glyphBuffer ? glyphBuffer->size() : 0;
+ unsigned lastGlyphCount = glyphBuffer.size();
UChar32 character = 0;
UChar32 previousCharacter = 0;
@@ -229,8 +226,7 @@
auto transformsType = shouldApplyFontTransforms(glyphBuffer, lastGlyphCount, previousCharacter);
if (transformsType != TransformsType::None) {
m_runWidthSoFar += applyFontTransforms(glyphBuffer, m_run.ltr(), lastGlyphCount, *lastFontData, previousCharacter, transformsType == TransformsType::Forced, charactersTreatedAsSpace);
- if (glyphBuffer)
- glyphBuffer->shrink(lastGlyphCount);
+ glyphBuffer.shrink(lastGlyphCount);
}
lastFontData = font;
@@ -280,15 +276,13 @@
// Increase previous width
m_expansion -= m_expansionPerOpportunity;
m_runWidthSoFar += m_expansionPerOpportunity;
- if (glyphBuffer) {
- if (glyphBuffer->isEmpty()) {
- if (m_forTextEmphasis)
- glyphBuffer->add(font->zeroWidthSpaceGlyph(), *font, m_expansionPerOpportunity, currentCharacterIndex);
- else
- glyphBuffer->add(font->spaceGlyph(), *font, m_expansionPerOpportunity, currentCharacterIndex);
- } else
- glyphBuffer->expandLastAdvance(m_expansionPerOpportunity);
- }
+ if (glyphBuffer.isEmpty()) {
+ if (m_forTextEmphasis)
+ glyphBuffer.add(font->zeroWidthSpaceGlyph(), *font, m_expansionPerOpportunity, currentCharacterIndex);
+ else
+ glyphBuffer.add(font->spaceGlyph(), *font, m_expansionPerOpportunity, currentCharacterIndex);
+ } else
+ glyphBuffer.expandLastAdvance(m_expansionPerOpportunity);
} else {
// Increase next width
leftoverJustificationWidth += m_expansionPerOpportunity;
@@ -313,9 +307,9 @@
}
auto transformsType = shouldApplyFontTransforms(glyphBuffer, lastGlyphCount, previousCharacter);
- if (transformsType != TransformsType::None && glyphBuffer && FontCascade::treatAsSpace(character)) {
- charactersTreatedAsSpace.append(std::make_pair(glyphBuffer->size(),
- OriginalAdvancesForCharacterTreatedAsSpace(character == ' ', glyphBuffer->size() ? glyphBuffer->advanceAt(glyphBuffer->size() - 1).width() : 0, width)));
+ if (transformsType != TransformsType::None && FontCascade::treatAsSpace(character)) {
+ charactersTreatedAsSpace.append(std::make_pair(glyphBuffer.size(),
+ OriginalAdvancesForCharacterTreatedAsSpace(character == ' ', glyphBuffer.size() ? glyphBuffer.advanceAt(glyphBuffer.size() - 1).width() : 0, width)));
}
if (m_accountForGlyphBounds) {
@@ -332,8 +326,7 @@
m_runWidthSoFar += width;
- if (glyphBuffer)
- glyphBuffer->add(glyph, *font, width, currentCharacterIndex);
+ glyphBuffer.add(glyph, *font, width, currentCharacterIndex);
if (m_accountForGlyphBounds) {
m_maxGlyphBoundingBoxY = std::max(m_maxGlyphBoundingBoxY, bounds.maxY());
@@ -343,24 +336,23 @@
previousCharacter = character;
}
- if (glyphBuffer && leftoverJustificationWidth) {
+ if (leftoverJustificationWidth) {
if (m_forTextEmphasis)
- glyphBuffer->add(lastFontData->zeroWidthSpaceGlyph(), *lastFontData, leftoverJustificationWidth, m_run.length() - 1);
+ glyphBuffer.add(lastFontData->zeroWidthSpaceGlyph(), *lastFontData, leftoverJustificationWidth, m_run.length() - 1);
else
- glyphBuffer->add(lastFontData->spaceGlyph(), *lastFontData, leftoverJustificationWidth, m_run.length() - 1);
+ glyphBuffer.add(lastFontData->spaceGlyph(), *lastFontData, leftoverJustificationWidth, m_run.length() - 1);
}
auto transformsType = shouldApplyFontTransforms(glyphBuffer, lastGlyphCount, previousCharacter);
if (transformsType != TransformsType::None) {
m_runWidthSoFar += applyFontTransforms(glyphBuffer, m_run.ltr(), lastGlyphCount, *lastFontData, previousCharacter, transformsType == TransformsType::Forced, charactersTreatedAsSpace);
- if (glyphBuffer)
- glyphBuffer->shrink(lastGlyphCount);
+ glyphBuffer.shrink(lastGlyphCount);
}
m_currentCharacterIndex = textIterator.currentIndex();
}
-void WidthIterator::advance(unsigned offset, GlyphBuffer* glyphBuffer)
+void WidthIterator::advance(unsigned offset, GlyphBuffer& glyphBuffer)
{
unsigned length = m_run.length();
@@ -383,7 +375,7 @@
bool WidthIterator::advanceOneCharacter(float& width, GlyphBuffer& glyphBuffer)
{
unsigned oldSize = glyphBuffer.size();
- advance(m_currentCharacterIndex + 1, &glyphBuffer);
+ advance(m_currentCharacterIndex + 1, glyphBuffer);
float w = 0;
for (unsigned i = oldSize; i < glyphBuffer.size(); ++i)
w += glyphBuffer.advanceAt(i).width();
Modified: trunk/Source/WebCore/platform/graphics/WidthIterator.h (265412 => 265413)
--- trunk/Source/WebCore/platform/graphics/WidthIterator.h 2020-08-09 04:27:09 UTC (rev 265412)
+++ trunk/Source/WebCore/platform/graphics/WidthIterator.h 2020-08-09 05:14:40 UTC (rev 265413)
@@ -42,7 +42,7 @@
public:
WidthIterator(const FontCascade&, const TextRun&, HashSet<const Font*>* fallbackFonts = 0, bool accountForGlyphBounds = false, bool forTextEmphasis = false);
- void advance(unsigned to, GlyphBuffer*);
+ void advance(unsigned to, GlyphBuffer&);
bool advanceOneCharacter(float& width, GlyphBuffer&);
float maxGlyphBoundingBoxY() const { ASSERT(m_accountForGlyphBounds); return m_maxGlyphBoundingBoxY; }
@@ -57,11 +57,11 @@
private:
GlyphData glyphDataForCharacter(UChar32, bool mirror);
template <typename TextIterator>
- inline void advanceInternal(TextIterator&, GlyphBuffer*);
+ inline void advanceInternal(TextIterator&, GlyphBuffer&);
enum class TransformsType { None, Forced, NotForced };
- TransformsType shouldApplyFontTransforms(const GlyphBuffer*, unsigned lastGlyphCount, UChar32 previousCharacter) const;
- float applyFontTransforms(GlyphBuffer*, bool ltr, unsigned& lastGlyphCount, const Font&, UChar32 previousCharacter, bool force, CharactersTreatedAsSpace&);
+ TransformsType shouldApplyFontTransforms(const GlyphBuffer&, unsigned lastGlyphCount, UChar32 previousCharacter) const;
+ float applyFontTransforms(GlyphBuffer&, bool ltr, unsigned& lastGlyphCount, const Font&, UChar32 previousCharacter, bool force, CharactersTreatedAsSpace&);
const FontCascade& m_font;
const TextRun& m_run;
Modified: trunk/Source/WebCore/platform/graphics/cocoa/FontCocoa.mm (265412 => 265413)
--- trunk/Source/WebCore/platform/graphics/cocoa/FontCocoa.mm 2020-08-09 04:27:09 UTC (rev 265412)
+++ trunk/Source/WebCore/platform/graphics/cocoa/FontCocoa.mm 2020-08-09 05:14:40 UTC (rev 265413)
@@ -547,7 +547,8 @@
void Font::applyTransforms(GlyphBuffer& glyphBuffer, unsigned beginningIndex, bool enableKerning, bool requiresShaping, const AtomString& locale) const
{
// FIXME: Implement GlyphBuffer initial advance.
- CTFontTransformOptions options = (enableKerning ? kCTFontTransformApplyPositioning : 0) | (requiresShaping ? kCTFontTransformApplyShaping : 0);
+ UNUSED_PARAM(requiresShaping);
+ CTFontTransformOptions options = (enableKerning ? kCTFontTransformApplyPositioning : 0) | kCTFontTransformApplyShaping;
#if USE(CTFONTTRANSFORMGLYPHSWITHLANGUAGE)
auto handler = ^(CFRange range, CGGlyph** newGlyphsPointer, CGSize** newAdvancesPointer) {
range.location = std::min(std::max(range.location, static_cast<CFIndex>(0)), static_cast<CFIndex>(glyphBuffer.size()));
Modified: trunk/Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp (265412 => 265413)
--- trunk/Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp 2020-08-09 04:27:09 UTC (rev 265412)
+++ trunk/Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp 2020-08-09 05:14:40 UTC (rev 265413)
@@ -59,7 +59,7 @@
{
GlyphBuffer glyphBuffer;
auto before = m_simpleWidthIterator->currentCharacterIndex();
- m_simpleWidthIterator->advance(m_textPosition + 1, &glyphBuffer);
+ m_simpleWidthIterator->advance(m_textPosition + 1, glyphBuffer);
auto after = m_simpleWidthIterator->currentCharacterIndex();
if (before == after) {
m_currentMetrics = SVGTextMetrics();
