Attachment 345917 Details for Bug 188032 – Patch

[patch] Patch

bug-188032-20180727091627.patch (text/plain), 17.90 KB, created by Myles C. Maxfield on 2018-07-27 09:16:28 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Myles C. Maxfield

Created: 2018-07-27 09:16:28 PDT

Size: 17.90 KB

patch

obsolete

>Subversion Revision: 234309
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 0ff7fc87e61852842c1494a36168dcf108441ef7..01bd289a6c27e3342a9ec0cacce69eabfd67133f 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,55 @@
>+2018-07-27  Myles C. Maxfield  <mmaxfield@apple.com>
>+
>+        [WIN] Crash when trying to access store pages
>+        https://bugs.webkit.org/show_bug.cgi?id=188032
>+        <rdar://problem/42467016>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The Windows implementation of GlyphBuffer has an additional member, m_offsets, which represents
>+        an additional offset to the position to paint each glyph. It also has two add() functions, one
>+        which appends to this vector, and one which doesn't. The one that doesn't append to the vector
>+        should never be called on Windows (because Windows requires this vector to be full).
>+
>+        There were two situations where it was getting called:
>+        1) Inside ComplexTextController
>+        2) Inside display list playback
>+
>+        Windows shouldn't be using ComplexTextController because the Windows implementation of this 
>+        class isn't ready yet; instead it should be using UniscribeController. The display list playback
>+        code should be used on Windows.
>+
>+        Rather than fix the function to append an offset, we actually don't need the m_offsets vector
>+        in the first place. Instead, we can do it the same way that the Cocoa ports do it, which is to
>+        bake the offsets into the glyph advances. This is possible because the GlyphBuffer doesn't need
>+        to distinguish between layout advances and paint advances, so we can bake them together and
>+        just put paint advances in the GlyphBuffer. This should be a small (probably within-the-noise)
>+        performance and memory improvement.
>+
>+        * platform/graphics/ComplexTextController.cpp:
>+        (WebCore::ComplexTextController::ComplexTextController): Make sure that ComplexTextController
>+        isn't used on Windows.
>+        * platform/graphics/FontCascade.cpp:
>+        (WebCore::FontCascade::widthOfTextRange const): Switch from ComplexTextController to
>+        UniscribeController on Windows.
>+        (WebCore::FontCascade::drawGlyphBuffer const): After deleting the m_offsets vector, there's
>+        no reason to consult it when drawing.
>+        * platform/graphics/GlyphBuffer.h: Remove m_offsets
>+        (WebCore::GlyphBuffer::clear):
>+        (WebCore::GlyphBuffer::advanceAt const):
>+        (WebCore::GlyphBuffer::add):
>+        (WebCore::GlyphBuffer::expandLastAdvance):
>+        (WebCore::GlyphBuffer::shrink):
>+        (WebCore::GlyphBuffer::swap):
>+        (WebCore::GlyphBuffer::offsetAt const): Deleted.
>+        * platform/graphics/win/FontCGWin.cpp:
>+        (WebCore::FontCascade::drawGlyphs): After deleting the m_offsets vector, there's no reason
>+        to consult it when drawing.
>+        * platform/graphics/win/FontCascadeDirect2D.cpp:
>+        (WebCore::FontCascade::drawGlyphs): Ditto.
>+        * platform/graphics/win/UniscribeController.cpp:
>+        (WebCore::UniscribeController::shapeAndPlaceItem): Bake in the offsets into the glyph advances.
>+
> 2018-07-26  Andy VanWagoner  <andy@vanwagoner.family>
> 
>         [INTL] Remove INTL sub-feature compile flags
>diff --git a/Source/WebCore/platform/graphics/ComplexTextController.cpp b/Source/WebCore/platform/graphics/ComplexTextController.cpp
>index f9f18a80fbd97ed33d2d609c3094ea5e15762649..2e2bbbe2ea5c4e362dbe878549c07bc25797598c 100644
>--- a/Source/WebCore/platform/graphics/ComplexTextController.cpp
>+++ b/Source/WebCore/platform/graphics/ComplexTextController.cpp
>@@ -144,6 +144,10 @@ ComplexTextController::ComplexTextController(const FontCascade& font, const Text
>     , m_mayUseNaturalWritingDirection(mayUseNaturalWritingDirection)
>     , m_forTextEmphasis(forTextEmphasis)
> {
>+#if PLATFORM(WIN)
>+    ASSERT_NOT_REACHED();
>+#endif
>+
>     computeExpansionOpportunity();
> 
>     collectComplexTextRuns();
>diff --git a/Source/WebCore/platform/graphics/FontCascade.cpp b/Source/WebCore/platform/graphics/FontCascade.cpp
>index 5462209d7502764f9cde6ea8957b7cdc2add7787..932789461acf36a12dd158a8306bdf945505881a 100644
>--- a/Source/WebCore/platform/graphics/FontCascade.cpp
>+++ b/Source/WebCore/platform/graphics/FontCascade.cpp
>@@ -41,6 +41,10 @@
> #include <wtf/text/AtomicStringHash.h>
> #include <wtf/text/StringBuilder.h>
> 
>+#if PLATFORM(WIN)
>+#include "UniscribeController.h"
>+#endif
>+
> namespace WebCore {
> using namespace WTF;
> using namespace Unicode;
>@@ -343,6 +347,15 @@ float FontCascade::widthOfTextRange(const TextRun& run, unsigned from, unsigned
> 
>     auto codePathToUse = codePath(run);
>     if (codePathToUse == Complex) {
>+#if PLATFORM(WIN)
>+        UniscribeController it(this, run);
>+        it.advance(from);
>+        offsetBeforeRange = it.runWidthSoFar();
>+        it.advance(to);
>+        offsetAfterRange = it.runWidthSoFar();
>+        it.advance(to);
>+        totalWidth = it.runWidthSoFar();
>+#else
>         ComplexTextController complexIterator(*this, run, false, fallbackFonts);
>         complexIterator.advance(from, nullptr, IncludePartialGlyphs, fallbackFonts);
>         offsetBeforeRange = complexIterator.runWidthSoFar();
>@@ -350,6 +363,7 @@ float FontCascade::widthOfTextRange(const TextRun& run, unsigned from, unsigned
>         offsetAfterRange = complexIterator.runWidthSoFar();
>         complexIterator.advance(run.length(), nullptr, IncludePartialGlyphs, fallbackFonts);
>         totalWidth = complexIterator.runWidthSoFar();
>+#endif
>     } else {
>         WidthIterator simpleIterator(this, run, fallbackFonts);
>         simpleIterator.advance(from, nullptr);
>@@ -1418,7 +1432,7 @@ void FontCascade::drawGlyphBuffer(GraphicsContext& context, const GlyphBuffer& g
> {
>     // Draw each contiguous run of glyphs that use the same font data.
>     const Font* fontData = glyphBuffer.fontAt(0);
>-    FloatSize offset = glyphBuffer.offsetAt(0);
>+    // FIXME: Why do we subtract the initial advance's height but not its width???
>     FloatPoint startPoint(point.x(), point.y() - glyphBuffer.initialAdvance().height());
>     float nextX = startPoint.x() + glyphBuffer.advanceAt(0).width();
>     float nextY = startPoint.y() + glyphBuffer.advanceAt(0).height();
>@@ -1426,15 +1440,13 @@ void FontCascade::drawGlyphBuffer(GraphicsContext& context, const GlyphBuffer& g
>     unsigned nextGlyph = 1;
>     while (nextGlyph < glyphBuffer.size()) {
>         const Font* nextFontData = glyphBuffer.fontAt(nextGlyph);
>-        FloatSize nextOffset = glyphBuffer.offsetAt(nextGlyph);
> 
>-        if (nextFontData != fontData || nextOffset != offset) {
>+        if (nextFontData != fontData) {
>             if (shouldDrawIfLoading(*fontData, customFontNotReadyAction))
>                 context.drawGlyphs(*fontData, glyphBuffer, lastFrom, nextGlyph - lastFrom, startPoint, m_fontDescription.fontSmoothing());
> 
>             lastFrom = nextGlyph;
>             fontData = nextFontData;
>-            offset = nextOffset;
>             startPoint.setX(nextX);
>             startPoint.setY(nextY);
>         }
>diff --git a/Source/WebCore/platform/graphics/GlyphBuffer.h b/Source/WebCore/platform/graphics/GlyphBuffer.h
>index 1752dcbd08e8f6cc799be45637cccfcc52e78c3c..bf0e94c767b831008edfd537eb04023ef1b5d58f 100644
>--- a/Source/WebCore/platform/graphics/GlyphBuffer.h
>+++ b/Source/WebCore/platform/graphics/GlyphBuffer.h
>@@ -27,8 +27,7 @@
>  * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  */
> 
>-#ifndef GlyphBuffer_h
>-#define GlyphBuffer_h
>+#pragma once
> 
> #include "FloatSize.h"
> #include "Glyph.h"
>@@ -85,9 +84,6 @@ public:
>         m_advances.clear();
>         if (m_offsetsInString)
>             m_offsetsInString->clear();
>-#if PLATFORM(WIN)
>-        m_offsets.clear();
>-#endif
>     }
> 
>     GlyphBufferGlyph* glyphs(unsigned from) { return m_glyphs.data() + from; }
>@@ -110,44 +106,17 @@ public:
>     {
>         return m_advances[index];
>     }
>-
>-    FloatSize offsetAt(unsigned index) const
>-    {
>-#if PLATFORM(WIN)
>-        return m_offsets[index];
>-#else
>-        UNUSED_PARAM(index);
>-        return FloatSize();
>-#endif
>-    }
>     
>     static const unsigned noOffset = UINT_MAX;
>-    void add(Glyph glyph, const Font* font, float width, unsigned offsetInString = noOffset, const FloatSize* offset = 0)
>+    void add(Glyph glyph, const Font* font, float width, unsigned offsetInString = noOffset)
>     {
>-        m_font.append(font);
>-        m_glyphs.append(glyph);
>+        GlyphBufferAdvance advance;
>+        advance.setWidth(width);
>+        advance.setHeight(0);
> 
>-#if USE(CG)
>-        CGSize advance = { width, 0 };
>-        m_advances.append(advance);
>-#else
>-        m_advances.append(FloatSize(width, 0));
>-#endif
>-
>-#if PLATFORM(WIN)
>-        if (offset)
>-            m_offsets.append(*offset);
>-        else
>-            m_offsets.append(FloatSize());
>-#else
>-        UNUSED_PARAM(offset);
>-#endif
>-        
>-        if (offsetInString != noOffset && m_offsetsInString)
>-            m_offsetsInString->append(offsetInString);
>+        add(glyph, font, advance, offsetInString);
>     }
>-    
>-#if !USE(WINGDI)
>+
>     void add(Glyph glyph, const Font* font, GlyphBufferAdvance advance, unsigned offsetInString = noOffset)
>     {
>         m_font.append(font);
>@@ -158,7 +127,6 @@ public:
>         if (offsetInString != noOffset && m_offsetsInString)
>             m_offsetsInString->append(offsetInString);
>     }
>-#endif
> 
>     void reverse(unsigned from, unsigned length)
>     {
>@@ -172,6 +140,14 @@ public:
>         GlyphBufferAdvance& lastAdvance = m_advances.last();
>         lastAdvance.setWidth(lastAdvance.width() + width);
>     }
>+
>+    void expandLastAdvance(GlyphBufferAdvance expansion)
>+    {
>+        ASSERT(!isEmpty());
>+        GlyphBufferAdvance& lastAdvance = m_advances.last();
>+        lastAdvance.setWidth(lastAdvance.width() + expansion.width());
>+        lastAdvance.setHeight(lastAdvance.height() + expansion.height());
>+    }
>     
>     void saveOffsetsInString()
>     {
>@@ -191,9 +167,6 @@ public:
>         m_advances.shrink(truncationPoint);
>         if (m_offsetsInString)
>             m_offsetsInString->shrink(truncationPoint);
>-#if PLATFORM(WIN)
>-        m_offsets.shrink(truncationPoint);
>-#endif
>     }
> 
> private:
>@@ -210,12 +183,6 @@ private:
>         GlyphBufferAdvance s = m_advances[index1];
>         m_advances[index1] = m_advances[index2];
>         m_advances[index2] = s;
>-
>-#if PLATFORM(WIN)
>-        FloatSize offset = m_offsets[index1];
>-        m_offsets[index1] = m_offsets[index2];
>-        m_offsets[index2] = offset;
>-#endif
>     }
> 
>     Vector<const Font*, 2048> m_font;
>@@ -223,10 +190,6 @@ private:
>     Vector<GlyphBufferAdvance, 2048> m_advances;
>     GlyphBufferAdvance m_initialAdvance;
>     std::unique_ptr<Vector<unsigned, 2048>> m_offsetsInString;
>-#if PLATFORM(WIN)
>-    Vector<FloatSize, 2048> m_offsets;
>-#endif
> };
> 
> }
>-#endif
>diff --git a/Source/WebCore/platform/graphics/win/FontCGWin.cpp b/Source/WebCore/platform/graphics/win/FontCGWin.cpp
>index d70e6e495989f280b1e105317600bb30ea5207dc..7aaf0685afe2472a378fd8ba3a3b35cf6e345036 100644
>--- a/Source/WebCore/platform/graphics/win/FontCGWin.cpp
>+++ b/Source/WebCore/platform/graphics/win/FontCGWin.cpp
>@@ -174,9 +174,6 @@ void FontCascade::drawGlyphs(GraphicsContext& graphicsContext, const Font& font,
>     CGAffineTransform savedMatrix = CGContextGetTextMatrix(cgContext);
>     CGContextSetTextMatrix(cgContext, matrix);
> 
>-    // Uniscribe gives us offsets to help refine the positioning of combining glyphs.
>-    FloatSize translation = glyphBuffer.offsetAt(from);
>-
>     CGContextSetFontSize(cgContext, platformData.size());
>     wkSetCGContextFontRenderingStyle(cgContext, font.platformData().isSystemFont(), false, font.platformData().useGDI());
> 
>@@ -192,22 +189,22 @@ void FontCascade::drawGlyphs(GraphicsContext& graphicsContext, const Font& font,
>         Color fillColor = graphicsContext.fillColor();
>         Color shadowFillColor(shadowColor.red(), shadowColor.green(), shadowColor.blue(), shadowColor.alpha() * fillColor.alpha() / 255);
>         graphicsContext.setFillColor(shadowFillColor);
>-        float shadowTextX = point.x() + translation.width() + shadowOffset.width();
>+        float shadowTextX = point.x() + shadowOffset.width();
>         // If shadows are ignoring transforms, then we haven't applied the Y coordinate flip yet, so down is negative.
>-        float shadowTextY = point.y() + translation.height() + shadowOffset.height() * (graphicsContext.shadowsIgnoreTransforms() ? -1 : 1);
>+        float shadowTextY = point.y() + shadowOffset.height() * (graphicsContext.shadowsIgnoreTransforms() ? -1 : 1);
>         CGContextSetTextPosition(cgContext, shadowTextX, shadowTextY);
>         CGContextShowGlyphsWithAdvances(cgContext, glyphBuffer.glyphs(from), static_cast<const CGSize*>(glyphBuffer.advances(from)), numGlyphs);
>         if (font.syntheticBoldOffset()) {
>-            CGContextSetTextPosition(cgContext, point.x() + translation.width() + shadowOffset.width() + font.syntheticBoldOffset(), point.y() + translation.height() + shadowOffset.height());
>+            CGContextSetTextPosition(cgContext, point.x() + shadowOffset.width() + font.syntheticBoldOffset(), point.y() + shadowOffset.height());
>             CGContextShowGlyphsWithAdvances(cgContext, glyphBuffer.glyphs(from), static_cast<const CGSize*>(glyphBuffer.advances(from)), numGlyphs);
>         }
>         graphicsContext.setFillColor(fillColor);
>     }
> 
>-    CGContextSetTextPosition(cgContext, point.x() + translation.width(), point.y() + translation.height());
>+    CGContextSetTextPosition(cgContext, point.x(), point.y());
>     CGContextShowGlyphsWithAdvances(cgContext, glyphBuffer.glyphs(from), static_cast<const CGSize*>(glyphBuffer.advances(from)), numGlyphs);
>     if (font.syntheticBoldOffset()) {
>-        CGContextSetTextPosition(cgContext, point.x() + translation.width() + font.syntheticBoldOffset(), point.y() + translation.height());
>+        CGContextSetTextPosition(cgContext, point.x() + font.syntheticBoldOffset(), point.y());
>         CGContextShowGlyphsWithAdvances(cgContext, glyphBuffer.glyphs(from), static_cast<const CGSize*>(glyphBuffer.advances(from)), numGlyphs);
>     }
> 
>diff --git a/Source/WebCore/platform/graphics/win/FontCascadeDirect2D.cpp b/Source/WebCore/platform/graphics/win/FontCascadeDirect2D.cpp
>index d18addf3b2462ee25ede618dd2ed70310af22006..b5d2c41e1b7b1bcb95580cb4f9ce8da35a689b8a 100644
>--- a/Source/WebCore/platform/graphics/win/FontCascadeDirect2D.cpp
>+++ b/Source/WebCore/platform/graphics/win/FontCascadeDirect2D.cpp
>@@ -83,9 +83,6 @@ void FontCascade::drawGlyphs(GraphicsContext& graphicsContext, const Font& font,
>         context->SetTransform(matrix * skewMatrix);
>     }
> 
>-    // Uniscribe gives us offsets to help refine the positioning of combining glyphs.
>-    FloatSize translation = glyphBuffer.offsetAt(from);
>-
>     RELEASE_ASSERT(platformData.dwFont());
>     RELEASE_ASSERT(platformData.dwFontFace());
> 
>@@ -125,9 +122,9 @@ void FontCascade::drawGlyphs(GraphicsContext& graphicsContext, const Font& font,
>         graphicsContext.clearShadow();
>         Color fillColor = graphicsContext.fillColor();
>         Color shadowFillColor(shadowColor.red(), shadowColor.green(), shadowColor.blue(), shadowColor.alpha() * fillColor.alpha() / 255);
>-        float shadowTextX = point.x() + translation.width() + shadowOffset.width();
>+        float shadowTextX = point.x() + shadowOffset.width();
>         // If shadows are ignoring transforms, then we haven't applied the Y coordinate flip yet, so down is negative.
>-        float shadowTextY = point.y() + translation.height() + shadowOffset.height() * (graphicsContext.shadowsIgnoreTransforms() ? -1 : 1);
>+        float shadowTextY = point.y() + shadowOffset.height() * (graphicsContext.shadowsIgnoreTransforms() ? -1 : 1);
> 
>         auto shadowBrush = graphicsContext.brushWithColor(shadowFillColor);
>         context->DrawGlyphRun(D2D1::Point2F(shadowTextX, shadowTextY), &glyphRun, shadowBrush);
>@@ -135,9 +132,9 @@ void FontCascade::drawGlyphs(GraphicsContext& graphicsContext, const Font& font,
>             context->DrawGlyphRun(D2D1::Point2F(shadowTextX + font.syntheticBoldOffset(), shadowTextY), &glyphRun, shadowBrush);
>     }
> 
>-    context->DrawGlyphRun(D2D1::Point2F(point.x() + translation.width(), point.y() + translation.height()), &glyphRun, graphicsContext.solidFillBrush());
>+    context->DrawGlyphRun(D2D1::Point2F(point.x(), point.y()), &glyphRun, graphicsContext.solidFillBrush());
>     if (font.syntheticBoldOffset())
>-        context->DrawGlyphRun(D2D1::Point2F(point.x() + translation.width() + font.syntheticBoldOffset(), point.y() + translation.height()), &glyphRun, graphicsContext.solidFillBrush());
>+        context->DrawGlyphRun(D2D1::Point2F(point.x() + font.syntheticBoldOffset(), point.y()), &glyphRun, graphicsContext.solidFillBrush());
> 
>     if (hasSimpleShadow)
>         graphicsContext.setShadow(shadowOffset, shadowBlur, shadowColor);
>diff --git a/Source/WebCore/platform/graphics/win/UniscribeController.cpp b/Source/WebCore/platform/graphics/win/UniscribeController.cpp
>index 35d793571c721baa6fed33f80150372e893e4003..a0147734475a3d25f72662a8c8a87c8be58065d6 100644
>--- a/Source/WebCore/platform/graphics/win/UniscribeController.cpp
>+++ b/Source/WebCore/platform/graphics/win/UniscribeController.cpp
>@@ -364,8 +364,13 @@ bool UniscribeController::shapeAndPlaceItem(const UChar* cp, unsigned i, const F
>         // as well, so that when the time comes to draw those glyphs, we can apply the appropriate
>         // translation.
>         if (glyphBuffer) {
>-            FloatSize size(offsetX, -offsetY);
>-            glyphBuffer->add(glyph, fontData, advance, GlyphBuffer::noOffset, &size);
>+            GlyphBufferAdvance origin(offsetX, -offsetY);
>+            if (!glyphBuffer->advancesCount())
>+                glyphBuffer->setInitialAdvance(origin);
>+            else
>+                glyphBuffer->expandLastAdvance(origin);
>+            GlyphBufferAdvance advance(-origin.width() + advance, -origin.height());
>+            glyphBuffer->add(glyph, fontData, advance);
>         }
> 
>         FloatRect glyphBounds = fontData->boundsForGlyph(glyph);

Flags:

ews-watchlist: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 188032: 345806 | 345838 | 345897 | 345902 | 345917 | 345930