<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[212072] branches/safari-603-branch</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/212072">212072</a></dd>
<dt>Author</dt> <dd>matthew_hanson@apple.com</dd>
<dt>Date</dt> <dd>2017-02-09 22:37:27 -0800 (Thu, 09 Feb 2017)</dd>
</dl>
<h3>Log Message</h3>
<pre>Merge <a href="http://trac.webkit.org/projects/webkit/changeset/211382">r211382</a>. rdar://problem/29738514</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#branchessafari603branchSourceWebCoreChangeLog">branches/safari-603-branch/Source/WebCore/ChangeLog</a></li>
<li><a href="#branchessafari603branchSourceWebCoreplatformgraphicsGlyphBufferh">branches/safari-603-branch/Source/WebCore/platform/graphics/GlyphBuffer.h</a></li>
<li><a href="#branchessafari603branchSourceWebCoreplatformgraphicscocoaFontCascadeCocoamm">branches/safari-603-branch/Source/WebCore/platform/graphics/cocoa/FontCascadeCocoa.mm</a></li>
<li><a href="#branchessafari603branchSourceWebCoreplatformgraphicsmacComplexTextControllercpp">branches/safari-603-branch/Source/WebCore/platform/graphics/mac/ComplexTextController.cpp</a></li>
<li><a href="#branchessafari603branchSourceWebCoreplatformgraphicsmacComplexTextControllerh">branches/safari-603-branch/Source/WebCore/platform/graphics/mac/ComplexTextController.h</a></li>
<li><a href="#branchessafari603branchToolsChangeLog">branches/safari-603-branch/Tools/ChangeLog</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="branchessafari603branchSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: branches/safari-603-branch/Source/WebCore/ChangeLog (212071 => 212072)</h4>
<pre class="diff"><span>
<span class="info">--- branches/safari-603-branch/Source/WebCore/ChangeLog 2017-02-10 06:37:23 UTC (rev 212071)
+++ branches/safari-603-branch/Source/WebCore/ChangeLog 2017-02-10 06:37:27 UTC (rev 212072)
</span><span class="lines">@@ -1,5 +1,84 @@
</span><span class="cx"> 2017-02-09 Matthew Hanson <matthew_hanson@apple.com>
</span><span class="cx">
</span><ins>+ Merge r211382. rdar://problem/29738514
+
+ 2017-01-30 Myles C. Maxfield <mmaxfield@apple.com>
+
+ Correct spacing regression on inter-element complex path shaping on some fonts
+ https://bugs.webkit.org/show_bug.cgi?id=166013
+
+ Reviewed by Simon Fraser.
+
+ This patch brings the implementation of ComplexTextController in-line with the
+ design at https://trac.webkit.org/wiki/ComplexTextController. Previously,
+ ComplexTextController had a few problems:
+ - The total width computed by ComplexTextController didn't match the width if
+ you iterated over the entire string and added up the advances
+ - FontCascade::getGlyphsAndAdvancesForComplexText() tried to compensate for
+ the above by construing the concepts of paint advances as distinct from layout
+ advances
+ - Initial advances were considered part of layout sometimes and part of painting
+ other times, depending on which function reports the information
+ - For RTL runs, the wrong origin was added to the initial advance, and the origin
+ should have been subtracted instead of added
+
+ This patch exhaustively updates every function in ComplexTextController to be
+ consistent with the design linked to above. This design solves all of these
+ problems.
+
+ Tests: ComplexTextControllerTest.InitialAdvanceWithLeftRunInRTL
+ ComplexTextControllerTest.InitialAdvanceInRTL
+ ComplexTextControllerTest.InitialAdvanceWithLeftRunInLTR
+ ComplexTextControllerTest.InitialAdvanceInLTR
+ ComplexTextControllerTest.InitialAdvanceInRTLNoOrigins
+ ComplexTextControllerTest.LeadingExpansion
+ ComplexTextControllerTest.VerticalAdvances
+
+ * platform/graphics/GlyphBuffer.h:
+ (WebCore::GlyphBuffer::setLeadingExpansion): Deleted. No longer necessary.
+ (WebCore::GlyphBuffer::leadingExpansion): Deleted. Ditto.
+ * platform/graphics/cocoa/FontCascadeCocoa.mm:
+ (WebCore::FontCascade::adjustSelectionRectForComplexText): Removed use of
+ unnecessary leadingExpansion().
+ (WebCore::FontCascade::getGlyphsAndAdvancesForComplexText): This function needs
+ to compute paint advances, which means that it can't base this information off
+ of layout advances. This function uses the trick mentioned at the end of the
+ above link to compute the paint offset of an arbitrary glyph in the middle of
+ an RTL run.
+ * platform/graphics/mac/ComplexTextController.cpp:
+ (WebCore::ComplexTextController::computeExpansionOpportunity): Refactored for
+ testing.
+ (WebCore::ComplexTextController::ComplexTextController): Ditto.
+ (WebCore::ComplexTextController::finishConstruction): Ditto.
+ (WebCore::ComplexTextController::offsetForPosition): This function operates on
+ layout advances, and the initial layout advance is already added into the
+ m_adjustedBaseAdvances vector by adjustGlyphsAndAdvances(). Therefore, there is
+ no need to add it again here.
+ (WebCore::ComplexTextController::advance): This function had completely busted
+ logic about the relationship between initial advances and the first origin in
+ each run. Because of the fortunate choice of only representing layout advances
+ in m_adjustedBaseAdvances, this entire block can be removed and the raw paint
+ initial advance can be reported to the GlyphBuffer. Later in the function, we
+ have to update the logic about how to compute a paint advance given a layout
+ advance and some origins. In particular, there are two tricky pieces here: 1.
+ The layout advance for the first glyph is equal to (initial advance - first
+ origin + first Core Text advance, so computing the paint offset must cancel out
+ the initial layout offset, and 2. the last paint advance in a run must actually
+ end up at the position of the first glyph in the next run, so the next run's
+ initial advance must be queried.
+ (WebCore::ComplexTextController::adjustGlyphsAndAdvances): Previously, we
+ represented an initial advance of a successive run by just adding it to the
+ previous run's last advance. However, this is incompatible with the new model
+ presented in the link above, so we remove this section. We also have to add in
+ the logic that the layout advance for the first glyph is equal to the formula
+ presented above.
+ * platform/graphics/mac/ComplexTextController.h:
+ (WebCore::ComplexTextController::ComplexTextRun::initialAdvance): Adjust comment
+ to reflect reality.
+ (WebCore::ComplexTextController::leadingExpansion): Deleted.
+
+2017-02-09 Matthew Hanson <matthew_hanson@apple.com>
+
</ins><span class="cx"> Merge r211957. rdar://problem/30029354
</span><span class="cx">
</span><span class="cx"> 2017-02-09 Antti Koivisto <antti@apple.com>
</span></span></pre></div>
<a id="branchessafari603branchSourceWebCoreplatformgraphicsGlyphBufferh"></a>
<div class="modfile"><h4>Modified: branches/safari-603-branch/Source/WebCore/platform/graphics/GlyphBuffer.h (212071 => 212072)</h4>
<pre class="diff"><span>
<span class="info">--- branches/safari-603-branch/Source/WebCore/platform/graphics/GlyphBuffer.h 2017-02-10 06:37:23 UTC (rev 212071)
+++ branches/safari-603-branch/Source/WebCore/platform/graphics/GlyphBuffer.h 2017-02-10 06:37:27 UTC (rev 212072)
</span><span class="lines">@@ -102,9 +102,6 @@
</span><span class="cx">
</span><span class="cx"> void setInitialAdvance(GlyphBufferAdvance initialAdvance) { m_initialAdvance = initialAdvance; }
</span><span class="cx"> const GlyphBufferAdvance& initialAdvance() const { return m_initialAdvance; }
</span><del>-
- void setLeadingExpansion(float leadingExpansion) { m_leadingExpansion = leadingExpansion; }
- float leadingExpansion() const { return m_leadingExpansion; }
</del><span class="cx">
</span><span class="cx"> Glyph glyphAt(unsigned index) const
</span><span class="cx"> {
</span><span class="lines">@@ -248,7 +245,6 @@
</span><span class="cx"> #if PLATFORM(WIN)
</span><span class="cx"> Vector<FloatSize, 2048> m_offsets;
</span><span class="cx"> #endif
</span><del>- float m_leadingExpansion;
</del><span class="cx"> };
</span><span class="cx">
</span><span class="cx"> }
</span></span></pre></div>
<a id="branchessafari603branchSourceWebCoreplatformgraphicscocoaFontCascadeCocoamm"></a>
<div class="modfile"><h4>Modified: branches/safari-603-branch/Source/WebCore/platform/graphics/cocoa/FontCascadeCocoa.mm (212071 => 212072)</h4>
<pre class="diff"><span>
<span class="info">--- branches/safari-603-branch/Source/WebCore/platform/graphics/cocoa/FontCascadeCocoa.mm 2017-02-10 06:37:23 UTC (rev 212071)
+++ branches/safari-603-branch/Source/WebCore/platform/graphics/cocoa/FontCascadeCocoa.mm 2017-02-10 06:37:27 UTC (rev 212072)
</span><span class="lines">@@ -497,7 +497,7 @@
</span><span class="cx"> float afterWidth = controller.runWidthSoFar();
</span><span class="cx">
</span><span class="cx"> if (run.rtl())
</span><del>- selectionRect.move(controller.totalWidth() - afterWidth + controller.leadingExpansion(), 0);
</del><ins>+ selectionRect.move(controller.totalWidth() - afterWidth, 0);
</ins><span class="cx"> else
</span><span class="cx"> selectionRect.move(beforeWidth, 0);
</span><span class="cx"> selectionRect.setWidth(LayoutUnit::fromFloatCeil(afterWidth - beforeWidth));
</span><span class="lines">@@ -508,20 +508,25 @@
</span><span class="cx"> float initialAdvance;
</span><span class="cx">
</span><span class="cx"> ComplexTextController controller(*this, run, false, 0, forTextEmphasis);
</span><del>- controller.advance(from);
- float beforeWidth = controller.runWidthSoFar();
</del><ins>+ GlyphBuffer dummyGlyphBuffer;
+ controller.advance(from, &dummyGlyphBuffer);
</ins><span class="cx"> controller.advance(to, &glyphBuffer);
</span><span class="cx">
</span><span class="cx"> if (glyphBuffer.isEmpty())
</span><span class="cx"> return 0;
</span><span class="cx">
</span><del>- float afterWidth = controller.runWidthSoFar();
-
</del><span class="cx"> if (run.rtl()) {
</span><del>- initialAdvance = controller.totalWidth() - afterWidth + controller.leadingExpansion();
</del><ins>+ // Exploit the fact that the sum of the paint advances is equal to
+ // the sum of the layout advances.
+ initialAdvance = controller.totalWidth();
+ for (unsigned i = 0; i < glyphBuffer.size(); ++i)
+ initialAdvance -= glyphBuffer.advanceAt(i).width();
</ins><span class="cx"> glyphBuffer.reverse(0, glyphBuffer.size());
</span><del>- } else
- initialAdvance = beforeWidth;
</del><ins>+ } else {
+ initialAdvance = dummyGlyphBuffer.initialAdvance().width();
+ for (unsigned i = 0; i < dummyGlyphBuffer.size(); ++i)
+ initialAdvance += dummyGlyphBuffer.advanceAt(i).width();
+ }
</ins><span class="cx">
</span><span class="cx"> return initialAdvance;
</span><span class="cx"> }
</span></span></pre></div>
<a id="branchessafari603branchSourceWebCoreplatformgraphicsmacComplexTextControllercpp"></a>
<div class="modfile"><h4>Modified: branches/safari-603-branch/Source/WebCore/platform/graphics/mac/ComplexTextController.cpp (212071 => 212072)</h4>
<pre class="diff"><span>
<span class="info">--- branches/safari-603-branch/Source/WebCore/platform/graphics/mac/ComplexTextController.cpp 2017-02-10 06:37:23 UTC (rev 212071)
+++ branches/safari-603-branch/Source/WebCore/platform/graphics/mac/ComplexTextController.cpp 2017-02-10 06:37:27 UTC (rev 212072)
</span><span class="lines">@@ -107,19 +107,12 @@
</span><span class="cx"> return layout.width(from, len, fallbackFonts);
</span><span class="cx"> }
</span><span class="cx">
</span><del>-ComplexTextController::ComplexTextController(const FontCascade& font, const TextRun& run, bool mayUseNaturalWritingDirection, HashSet<const Font*>* fallbackFonts, bool forTextEmphasis)
- : m_fallbackFonts(fallbackFonts)
- , m_font(font)
- , m_run(run)
- , m_end(run.length())
- , m_expansion(run.expansion())
- , m_mayUseNaturalWritingDirection(mayUseNaturalWritingDirection)
- , m_forTextEmphasis(forTextEmphasis)
</del><ins>+void ComplexTextController::computeExpansionOpportunity()
</ins><span class="cx"> {
</span><span class="cx"> if (!m_expansion)
</span><span class="cx"> m_expansionPerOpportunity = 0;
</span><span class="cx"> else {
</span><del>- unsigned expansionOpportunityCount = FontCascade::expansionOpportunityCount(m_run.text(), m_run.ltr() ? LTR : RTL, run.expansionBehavior()).first;
</del><ins>+ unsigned expansionOpportunityCount = FontCascade::expansionOpportunityCount(m_run.text(), m_run.ltr() ? LTR : RTL, m_run.expansionBehavior()).first;
</ins><span class="cx">
</span><span class="cx"> if (!expansionOpportunityCount)
</span><span class="cx"> m_expansionPerOpportunity = 0;
</span><span class="lines">@@ -126,7 +119,19 @@
</span><span class="cx"> else
</span><span class="cx"> m_expansionPerOpportunity = m_expansion / expansionOpportunityCount;
</span><span class="cx"> }
</span><ins>+}
</ins><span class="cx">
</span><ins>+ComplexTextController::ComplexTextController(const FontCascade& font, const TextRun& run, bool mayUseNaturalWritingDirection, HashSet<const Font*>* fallbackFonts, bool forTextEmphasis)
+ : m_fallbackFonts(fallbackFonts)
+ , m_font(font)
+ , m_run(run)
+ , m_end(run.length())
+ , m_expansion(run.expansion())
+ , m_mayUseNaturalWritingDirection(mayUseNaturalWritingDirection)
+ , m_forTextEmphasis(forTextEmphasis)
+{
+ computeExpansionOpportunity();
+
</ins><span class="cx"> collectComplexTextRuns();
</span><span class="cx"> adjustGlyphsAndAdvances();
</span><span class="cx">
</span><span class="lines">@@ -140,8 +145,6 @@
</span><span class="cx"> glyphCountSoFar += m_complexTextRuns[i]->glyphCount();
</span><span class="cx"> }
</span><span class="cx"> }
</span><del>-
- m_runWidthSoFar = m_leadingExpansion;
</del><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> unsigned ComplexTextController::offsetForPosition(float h, bool includePartialGlyphs)
</span><span class="lines">@@ -149,7 +152,6 @@
</span><span class="cx"> if (h >= m_totalWidth)
</span><span class="cx"> return m_run.ltr() ? m_end : 0;
</span><span class="cx">
</span><del>- h -= m_leadingExpansion;
</del><span class="cx"> if (h < 0)
</span><span class="cx"> return m_run.ltr() ? 0 : m_end;
</span><span class="cx">
</span><span class="lines">@@ -163,8 +165,6 @@
</span><span class="cx"> for (unsigned j = 0; j < complexTextRun.glyphCount(); ++j) {
</span><span class="cx"> size_t index = offsetIntoAdjustedGlyphs + j;
</span><span class="cx"> CGFloat adjustedAdvance = m_adjustedBaseAdvances[index].width;
</span><del>- if (!index)
- adjustedAdvance += complexTextRun.initialAdvance().width;
</del><span class="cx"> if (x < adjustedAdvance) {
</span><span class="cx"> CFIndex hitGlyphStart = complexTextRun.indexAt(j);
</span><span class="cx"> CFIndex hitGlyphEnd;
</span><span class="lines">@@ -544,7 +544,7 @@
</span><span class="cx"> offset = m_end;
</span><span class="cx">
</span><span class="cx"> if (offset <= m_currentCharacter) {
</span><del>- m_runWidthSoFar = m_leadingExpansion;
</del><ins>+ m_runWidthSoFar = 0;
</ins><span class="cx"> m_numGlyphsSoFar = 0;
</span><span class="cx"> m_currentRun = 0;
</span><span class="cx"> m_glyphInCurrentRun = 0;
</span><span class="lines">@@ -555,14 +555,14 @@
</span><span class="cx">
</span><span class="cx"> size_t runCount = m_complexTextRuns.size();
</span><span class="cx">
</span><del>- unsigned leftmostGlyph = 0;
- unsigned currentRunIndex = indexOfCurrentRun(leftmostGlyph);
</del><ins>+ unsigned indexOfLeftmostGlyphInCurrentRun = 0; // Relative to the beginning of ComplexTextController.
+ unsigned currentRunIndex = indexOfCurrentRun(indexOfLeftmostGlyphInCurrentRun);
</ins><span class="cx"> while (m_currentRun < runCount) {
</span><span class="cx"> const ComplexTextRun& complexTextRun = *m_complexTextRuns[currentRunIndex];
</span><span class="cx"> bool ltr = complexTextRun.isLTR();
</span><span class="cx"> size_t glyphCount = complexTextRun.glyphCount();
</span><del>- unsigned g = ltr ? m_glyphInCurrentRun : glyphCount - 1 - m_glyphInCurrentRun;
- unsigned k = leftmostGlyph + g;
</del><ins>+ unsigned glyphIndexIntoCurrentRun = ltr ? m_glyphInCurrentRun : glyphCount - 1 - m_glyphInCurrentRun;
+ unsigned glyphIndexIntoComplexTextController = indexOfLeftmostGlyphInCurrentRun + glyphIndexIntoCurrentRun;
</ins><span class="cx"> if (fallbackFonts && &complexTextRun.font() != &m_font.primaryFont())
</span><span class="cx"> fallbackFonts->add(&complexTextRun.font());
</span><span class="cx">
</span><span class="lines">@@ -569,43 +569,45 @@
</span><span class="cx"> // We must store the initial advance for the first glyph we are going to draw.
</span><span class="cx"> // When leftmostGlyph is 0, it represents the first glyph to draw, taking into
</span><span class="cx"> // account the text direction.
</span><del>- if (glyphBuffer && !leftmostGlyph) {
- CGSize initialAdvance = complexTextRun.initialAdvance();
-#if USE_LAYOUT_SPECIFIC_ADVANCES
- unsigned index = ltr ? 0 : m_adjustedBaseAdvances.size() - 1;
- initialAdvance.width += glyphOrigin(index).x;
- initialAdvance.height += glyphOrigin(index).y;
-#endif
- glyphBuffer->setInitialAdvance(initialAdvance);
</del><ins>+ if (!indexOfLeftmostGlyphInCurrentRun && glyphBuffer)
+ glyphBuffer->setInitialAdvance(complexTextRun.initialAdvance());
</ins><span class="cx">
</span><del>- glyphBuffer->setLeadingExpansion(m_leadingExpansion);
- }
-
</del><span class="cx"> while (m_glyphInCurrentRun < glyphCount) {
</span><del>- unsigned glyphStartOffset = complexTextRun.indexAt(g);
</del><ins>+ unsigned glyphStartOffset = complexTextRun.indexAt(glyphIndexIntoCurrentRun);
</ins><span class="cx"> unsigned glyphEndOffset;
</span><span class="cx"> if (complexTextRun.isMonotonic()) {
</span><span class="cx"> if (ltr)
</span><del>- glyphEndOffset = std::max<unsigned>(glyphStartOffset, static_cast<unsigned>(g + 1 < glyphCount ? complexTextRun.indexAt(g + 1) : complexTextRun.indexEnd()));
</del><ins>+ glyphEndOffset = std::max<unsigned>(glyphStartOffset, static_cast<unsigned>(glyphIndexIntoCurrentRun + 1 < glyphCount ? complexTextRun.indexAt(glyphIndexIntoCurrentRun + 1) : complexTextRun.indexEnd()));
</ins><span class="cx"> else
</span><del>- glyphEndOffset = std::max<unsigned>(glyphStartOffset, static_cast<unsigned>(g > 0 ? complexTextRun.indexAt(g - 1) : complexTextRun.indexEnd()));
</del><ins>+ glyphEndOffset = std::max<unsigned>(glyphStartOffset, static_cast<unsigned>(glyphIndexIntoCurrentRun > 0 ? complexTextRun.indexAt(glyphIndexIntoCurrentRun - 1) : complexTextRun.indexEnd()));
</ins><span class="cx"> } else
</span><del>- glyphEndOffset = complexTextRun.endOffsetAt(g);
</del><ins>+ glyphEndOffset = complexTextRun.endOffsetAt(glyphIndexIntoCurrentRun);
</ins><span class="cx">
</span><del>- CGSize adjustedBaseAdvance = m_adjustedBaseAdvances[k];
</del><ins>+ CGSize adjustedBaseAdvance = m_adjustedBaseAdvances[glyphIndexIntoComplexTextController];
</ins><span class="cx">
</span><span class="cx"> if (glyphStartOffset + complexTextRun.stringLocation() >= m_currentCharacter)
</span><span class="cx"> return;
</span><span class="cx">
</span><span class="cx"> if (glyphBuffer && !m_characterInCurrentGlyph) {
</span><ins>+ auto currentGlyphOrigin = glyphOrigin(glyphIndexIntoComplexTextController);
</ins><span class="cx"> GlyphBufferAdvance paintAdvance = adjustedBaseAdvance;
</span><del>-#if USE_LAYOUT_SPECIFIC_ADVANCES
- if (k + 1 < m_adjustedBaseAdvances.size()) {
- paintAdvance.setWidth(paintAdvance.width() + glyphOrigin(k + 1).x - glyphOrigin(k).x);
- paintAdvance.setHeight(paintAdvance.height() - glyphOrigin(k + 1).y + glyphOrigin(k).y);
</del><ins>+ if (!glyphIndexIntoCurrentRun) {
+ // The first layout advance of every run includes the "initial layout advance." However, here, we need
+ // paint advances, so subtract it out before transforming the layout advance into a paint advance.
+ paintAdvance.setWidth(paintAdvance.width() - (complexTextRun.initialAdvance().width - currentGlyphOrigin.x));
+ paintAdvance.setHeight(paintAdvance.height() - (complexTextRun.initialAdvance().height - currentGlyphOrigin.y));
</ins><span class="cx"> }
</span><del>-#endif
- glyphBuffer->add(m_adjustedGlyphs[k], &complexTextRun.font(), paintAdvance, complexTextRun.indexAt(m_glyphInCurrentRun));
</del><ins>+ paintAdvance.setWidth(paintAdvance.width() + glyphOrigin(glyphIndexIntoComplexTextController + 1).x - currentGlyphOrigin.x);
+ paintAdvance.setHeight(paintAdvance.height() + glyphOrigin(glyphIndexIntoComplexTextController + 1).y - currentGlyphOrigin.y);
+ if (glyphIndexIntoCurrentRun == glyphCount - 1 && currentRunIndex + 1 < runCount) {
+ // Our paint advance points to the end of the run. However, the next run may have an
+ // initial advance, and our paint advance needs to point to the location of the next
+ // glyph. So, we need to add in the next run's initial advance.
+ paintAdvance.setWidth(paintAdvance.width() - glyphOrigin(glyphIndexIntoComplexTextController + 1).x + m_complexTextRuns[currentRunIndex + 1]->initialAdvance().width);
+ paintAdvance.setHeight(paintAdvance.height() - glyphOrigin(glyphIndexIntoComplexTextController + 1).y + m_complexTextRuns[currentRunIndex + 1]->initialAdvance().height);
+ }
+ paintAdvance.setHeight(-paintAdvance.height()); // Increasing y points down
+ glyphBuffer->add(m_adjustedGlyphs[glyphIndexIntoComplexTextController], &complexTextRun.font(), paintAdvance, complexTextRun.indexAt(m_glyphInCurrentRun));
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> unsigned oldCharacterInCurrentGlyph = m_characterInCurrentGlyph;
</span><span class="lines">@@ -619,14 +621,14 @@
</span><span class="cx"> m_glyphInCurrentRun++;
</span><span class="cx"> m_characterInCurrentGlyph = 0;
</span><span class="cx"> if (ltr) {
</span><del>- g++;
- k++;
</del><ins>+ glyphIndexIntoCurrentRun++;
+ glyphIndexIntoComplexTextController++;
</ins><span class="cx"> } else {
</span><del>- g--;
- k--;
</del><ins>+ glyphIndexIntoCurrentRun--;
+ glyphIndexIntoComplexTextController--;
</ins><span class="cx"> }
</span><span class="cx"> }
</span><del>- currentRunIndex = incrementCurrentRun(leftmostGlyph);
</del><ins>+ currentRunIndex = incrementCurrentRun(indexOfLeftmostGlyphInCurrentRun);
</ins><span class="cx"> m_glyphInCurrentRun = 0;
</span><span class="cx"> }
</span><span class="cx"> }
</span><span class="lines">@@ -673,16 +675,6 @@
</span><span class="cx"> unsigned glyphCount = complexTextRun.glyphCount();
</span><span class="cx"> const Font& font = complexTextRun.font();
</span><span class="cx">
</span><del>- // Represent the initial advance for a text run by adjusting the advance
- // of the last glyph of the previous text run in the glyph buffer.
- if (!complexTextRun.glyphOrigins() && runIndex && m_adjustedBaseAdvances.size()) {
- CGSize previousAdvance = m_adjustedBaseAdvances.last();
- previousAdvance.width += complexTextRun.initialAdvance().width;
- previousAdvance.height -= complexTextRun.initialAdvance().height;
- m_adjustedBaseAdvances[m_adjustedBaseAdvances.size() - 1] = previousAdvance;
- }
- widthSinceLastCommit += complexTextRun.initialAdvance().width;
-
</del><span class="cx"> if (!complexTextRun.isLTR())
</span><span class="cx"> m_isLTROnly = false;
</span><span class="cx">
</span><span class="lines">@@ -726,6 +718,15 @@
</span><span class="cx"> glyph = font.spaceGlyph();
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+ if (!i) {
+ advance.width += complexTextRun.initialAdvance().width;
+ advance.height += complexTextRun.initialAdvance().height;
+ if (auto* origins = complexTextRun.glyphOrigins()) {
+ advance.width -= origins[0].x;
+ advance.height -= origins[0].y;
+ }
+ }
+
</ins><span class="cx"> advance.width += font.syntheticBoldOffset();
</span><span class="cx">
</span><span class="cx"> if (hasExtraSpacing) {
</span><span class="lines">@@ -757,20 +758,17 @@
</span><span class="cx"> if (m_expansion) {
</span><span class="cx"> bool expandLeft, expandRight;
</span><span class="cx"> std::tie(expandLeft, expandRight) = expansionLocation(ideograph, treatAsSpace, m_run.ltr(), afterExpansion, forbidLeadingExpansion, forbidTrailingExpansion, forceLeadingExpansion, forceTrailingExpansion);
</span><ins>+ m_expansion -= m_expansionPerOpportunity;
+ advance.width += m_expansionPerOpportunity;
</ins><span class="cx"> if (expandLeft) {
</span><span class="cx"> // Increase previous width
</span><del>- m_expansion -= m_expansionPerOpportunity;
- m_totalWidth += m_expansionPerOpportunity;
</del><span class="cx"> if (m_adjustedBaseAdvances.isEmpty())
</span><del>- m_leadingExpansion = m_expansionPerOpportunity;
</del><ins>+ complexTextRun.growInitialAdvanceHorizontally(m_expansionPerOpportunity);
</ins><span class="cx"> else
</span><span class="cx"> m_adjustedBaseAdvances.last().width += m_expansionPerOpportunity;
</span><span class="cx"> }
</span><del>- if (expandRight) {
- m_expansion -= m_expansionPerOpportunity;
- advance.width += m_expansionPerOpportunity;
</del><ins>+ if (expandRight)
</ins><span class="cx"> afterExpansion = true;
</span><del>- }
</del><span class="cx"> } else
</span><span class="cx"> afterExpansion = false;
</span><span class="cx">
</span><span class="lines">@@ -787,9 +785,7 @@
</span><span class="cx"> if (m_forTextEmphasis && (!FontCascade::canReceiveTextEmphasis(ch) || (U_GET_GC_MASK(ch) & U_GC_M_MASK)))
</span><span class="cx"> glyph = 0;
</span><span class="cx">
</span><del>- advance.height *= -1;
</del><span class="cx"> m_adjustedBaseAdvances.append(advance);
</span><del>-#if USE_LAYOUT_SPECIFIC_ADVANCES
</del><span class="cx"> if (auto* origins = complexTextRun.glyphOrigins()) {
</span><span class="cx"> ASSERT(m_glyphOrigins.size() < m_adjustedBaseAdvances.size());
</span><span class="cx"> m_glyphOrigins.grow(m_adjustedBaseAdvances.size());
</span><span class="lines">@@ -796,7 +792,6 @@
</span><span class="cx"> m_glyphOrigins[m_glyphOrigins.size() - 1] = origins[i];
</span><span class="cx"> ASSERT(m_glyphOrigins.size() == m_adjustedBaseAdvances.size());
</span><span class="cx"> }
</span><del>-#endif
</del><span class="cx"> m_adjustedGlyphs.append(glyph);
</span><span class="cx">
</span><span class="cx"> FloatRect glyphBounds = font.boundsForGlyph(glyph);
</span></span></pre></div>
<a id="branchessafari603branchSourceWebCoreplatformgraphicsmacComplexTextControllerh"></a>
<div class="modfile"><h4>Modified: branches/safari-603-branch/Source/WebCore/platform/graphics/mac/ComplexTextController.h (212071 => 212072)</h4>
<pre class="diff"><span>
<span class="info">--- branches/safari-603-branch/Source/WebCore/platform/graphics/mac/ComplexTextController.h 2017-02-10 06:37:23 UTC (rev 212071)
+++ branches/safari-603-branch/Source/WebCore/platform/graphics/mac/ComplexTextController.h 2017-02-10 06:37:27 UTC (rev 212072)
</span><span class="lines">@@ -51,8 +51,7 @@
</span><span class="cx">
</span><span class="cx"> enum GlyphIterationStyle { IncludePartialGlyphs, ByWholeGlyphs };
</span><span class="cx">
</span><del>-// ComplexTextController is responsible for rendering and measuring glyphs for
-// complex scripts on macOS and iOS.
</del><ins>+// See https://trac.webkit.org/wiki/ComplexTextController for more information about ComplexTextController.
</ins><span class="cx"> class ComplexTextController {
</span><span class="cx"> WTF_MAKE_FAST_ALLOCATED;
</span><span class="cx"> public:
</span><span class="lines">@@ -74,7 +73,6 @@
</span><span class="cx"> float minGlyphBoundingBoxY() const { return m_minGlyphBoundingBoxY; }
</span><span class="cx"> float maxGlyphBoundingBoxY() const { return m_maxGlyphBoundingBoxY; }
</span><span class="cx">
</span><del>- float leadingExpansion() const { return m_leadingExpansion; }
</del><span class="cx">
</span><span class="cx"> private:
</span><span class="cx"> class ComplexTextRun : public RefCounted<ComplexTextRun> {
</span><span class="lines">@@ -101,15 +99,33 @@
</span><span class="cx"> const CGGlyph* glyphs() const { return m_glyphs; }
</span><span class="cx">
</span><span class="cx"> /*
</span><del>- * X (Paint glyph position) X (Paint glyph position) X (Paint glyph position)
- * 7 7 7
- * / / /
- * / (Glyph origin) / (Glyph origin) / (Glyph origin)
- * / / /
- * / / /
- * X-----------------------X--------------------------X--------------------------X---->...
- * (text position ^) (initial advance) (base advance) (base advance)
</del><ins>+ * This is the format of the information CoreText gives us about each run:
+ *
+ * ----->X (Paint glyph position) X (Paint glyph position) X (Paint glyph position)
+ * / 7 7 7
+ * / / / /
+ * (Initial advance) / / (Glyph origin) / (Glyph origin) / (Glyph origin)
+ * ------------------- / / /
+ * / / / /
+ * X X--------------------------X--------------------------X--------------------------X
+ * (text position ^) (base advance) (base advance) (base advance)
+ *
+ *
+ *
+ *
+ *
+ * And here is the output we transform this into (for each run):
+ *
+ * ----->X------------------------->X------------------------->X
+ * / (Paint advance) (Paint advance) \
+ * / \
+ * (Initial advance) / \ (Paint advance)
+ * ------------------- ----------------
+ * / \
+ * X--------------------------------------------------X--------------------------X--------------------------X
+ * (text position ^) (layout advance) (layout advance) (layout advance)
</ins><span class="cx"> */
</span><ins>+ void growInitialAdvanceHorizontally(CGFloat delta) { m_initialAdvance.width += delta; }
</ins><span class="cx"> CGSize initialAdvance() const { return m_initialAdvance; }
</span><span class="cx"> const CGSize* baseAdvances() const { return m_baseAdvances; }
</span><span class="cx"> const CGPoint* glyphOrigins() const { return m_glyphOrigins.size() == glyphCount() ? m_glyphOrigins.data() : nullptr; }
</span><span class="lines">@@ -140,6 +156,7 @@
</span><span class="cx"> bool m_isLTR;
</span><span class="cx"> bool m_isMonotonic;
</span><span class="cx"> };
</span><ins>+ void computeExpansionOpportunity();
</ins><span class="cx">
</span><span class="cx"> static unsigned stringBegin(const ComplexTextRun& run) { return run.stringLocation() + run.indexBegin(); }
</span><span class="cx"> static unsigned stringEnd(const ComplexTextRun& run) { return run.stringLocation() + run.indexEnd(); }
</span><span class="lines">@@ -196,7 +213,6 @@
</span><span class="cx"> unsigned m_characterInCurrentGlyph { 0 };
</span><span class="cx"> float m_expansion { 0 };
</span><span class="cx"> float m_expansionPerOpportunity { 0 };
</span><del>- float m_leadingExpansion { 0 };
</del><span class="cx">
</span><span class="cx"> float m_minGlyphBoundingBoxX { std::numeric_limits<float>::max() };
</span><span class="cx"> float m_maxGlyphBoundingBoxX { std::numeric_limits<float>::min() };
</span></span></pre></div>
<a id="branchessafari603branchToolsChangeLog"></a>
<div class="modfile"><h4>Modified: branches/safari-603-branch/Tools/ChangeLog (212071 => 212072)</h4>
<pre class="diff"><span>
<span class="info">--- branches/safari-603-branch/Tools/ChangeLog 2017-02-10 06:37:23 UTC (rev 212071)
+++ branches/safari-603-branch/Tools/ChangeLog 2017-02-10 06:37:27 UTC (rev 212072)
</span><span class="lines">@@ -1,5 +1,24 @@
</span><span class="cx"> 2017-02-09 Matthew Hanson <matthew_hanson@apple.com>
</span><span class="cx">
</span><ins>+ Merge r211382. rdar://problem/29738514
+
+ 2017-01-30 Myles C. Maxfield <mmaxfield@apple.com>
+
+ Correct spacing regression on inter-element complex path shaping on some fonts
+ https://bugs.webkit.org/show_bug.cgi?id=166013
+
+ Reviewed by Simon Fraser.
+
+ Unskip existing tests and make some new tests:
+ - Testing complex text with no origins
+ - Testing initial expansions
+ - Testing the sign of vertical advances
+
+ * TestWebKitAPI/Tests/WebCore/ComplexTextController.cpp:
+ (TestWebKitAPI::TEST_F):
+
+2017-02-09 Matthew Hanson <matthew_hanson@apple.com>
+
</ins><span class="cx"> Merge r211915. rdar://problem/27607520
</span><span class="cx">
</span><span class="cx"> 2017-02-08 Andy Estes <aestes@apple.com>
</span></span></pre>
</div>
</div>
</body>
</html>