Bug 230781

<rdar://problem/70884096>

Created attachment 439228 [details] Patch very basic/straightforward initial approach

Created attachment 439496 [details] Patch

Comment on attachment 439496 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439496&action=review Please make sure there is no EWS red before landing. > Source/WebCore/ChangeLog:18 > + Is there a reason this line is blank? > Source/WebCore/ChangeLog:28 > + * platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp: Renamed from Source/WebCore/platform/graphics/displaylists/DisplayListDrawGlyphsRecorderCoreText.cpp. > + (WebCore::DrawGlyphsRecorder::createInternalContext): > + (WebCore::DrawGlyphsRecorder::DrawGlyphsRecorder): > + (WebCore::DrawGlyphsRecorder::populateInternalState): > + (WebCore::DrawGlyphsRecorder::prepareInternalContext): > + (WebCore::DrawGlyphsRecorder::recordDrawGlyphs): > + (WebCore::DrawGlyphsRecorder::drawGlyphs): > + (WebCore::DrawGlyphsRecorder::drawNativeText): Added. What did you change here? Because you moved the file, the diff just shows all green so it's unclear what you changed. > Source/WebCore/platform/graphics/DrawGlyphsRecorder.h:58 > + void drawNativeText(CTLineRef, CGRect lineRect, CTFontRef, CGFloat fontSize); nit: can the font go second instead of third? > Source/WebCore/platform/graphics/GraphicsContext.h:357 > + virtual const GraphicsContextState& lastStateChange() const { return m_state; } is it a change? the variable name and the type name don't seem to indicate that it's a change. > Source/WebCore/platform/graphics/GraphicsContext.h:475 > + drawGlyphs(font, glyphs, advances, count, localAnchor, smoothingMode); Why doesn't this cause a stack overflow? Doesn't drawGlyphs() end up going through DrawGlyphsRecorder, which ends up calling this again?

Comment on attachment 439496 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439496&action=review >> Source/WebCore/ChangeLog:18 >> + > > Is there a reason this line is blank? I personally find a ChangeLog much easier to read when there's whitespace in between "groups" of changed things. >> Source/WebCore/ChangeLog:28 >> + (WebCore::DrawGlyphsRecorder::drawNativeText): Added. > > What did you change here? Because you moved the file, the diff just shows all green so it's unclear what you changed. Other than the newly added `drawNativeText` method, all the other changes are - replace `DisplayList::Recorder` with `GraphicsContext` - renamed `DrawGlyphsDeconstruction` to `DeconstructDrawGlyphs` so that it reads better - added `DeriveFontFromContext` (which adds some new logic inside `recordDrawGlyphs `) - replaced uses of `m_owner.currentState()` with other methods that exist on `GraphicsContext` instead of only on `DisplayList::Recorder` - `m_owner.currentState().ctm` -> `m_owner.getCTM()` - `m_owner.currentState().stateChange.m_state` -> `m_owner.lastStateChange()` I thought I'd given enough info in the paragraph below, but if you feel I haven't please let me know and I can expand. >> Source/WebCore/platform/graphics/DrawGlyphsRecorder.h:58 >> + void drawNativeText(CTLineRef, CGRect lineRect, CTFontRef, CGFloat fontSize); > > nit: can the font go second instead of third? Sure. I'd like to keep the `CTFontRef` and `CGFloat fontSize` nearby eachother so i'll put both of those first. >> Source/WebCore/platform/graphics/GraphicsContext.h:357 >> + virtual const GraphicsContextState& lastStateChange() const { return m_state; } > > is it a change? the variable name and the type name don't seem to indicate that it's a change. The previous logic for `DrawGlyphsRecorder` used `m_owner.currentState().stateChange.m_state`, but the concept of `currentState()` (and `stateChange`) are both specific to `DisplayList::Recorder`, but I needed something more generic so that `DrawGlyphsRecorder` could be used with any `GraphicsContext`, not just `DisplayList::Recorder`. Frankly I'm not entirely sure why `m_owner.currentState().stateChange.m_state` was needed over just `m_owner.state()` or `m_owner.currentState().lastDrawingState`, so I tried to use a name that sounded as close to the literal code as possible. >> Source/WebCore/platform/graphics/GraphicsContext.h:475 >> + drawGlyphs(font, glyphs, advances, count, localAnchor, smoothingMode); > > Why doesn't this cause a stack overflow? Doesn't drawGlyphs() end up going through DrawGlyphsRecorder, which ends up calling this again? When not using the GPUProcess, `GraphicsContext::drawGlyphs` doesn't even involve `DrawGlyphsRecorder` and just invokes `FontCascade::drawGlyphs`. When using the GPUProcess, in the WebProcess `GraphicsContext::drawGlyphs` is overridden by `DisplayList::Recorder::drawGlyphs`, which does call into `drawGlyphsAndCacheFont`, but that is also overridden by `DisplayList::Recorder::drawGlyphsAndCacheFont` which appends a `DrawGlyphs` display list item (and then in the GPUProcess it behaves like the above).

Comment on attachment 439496 [details] Patch It might (optionally, not saying you have to) be better to separate the rearchitecture and functional change.

Created attachment 439548 [details] Patch

Created attachment 439771 [details] Patch

Created attachment 439774 [details] Patch

Committed r283339 (242356@main): <https://commits.webkit.org/242356@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 439774 [details].

Reverted r283339 for reason: Reverting because this commit may have caused fast/attachment tests to crash Committed r283577 (242538@main): <https://commits.webkit.org/242538@main>

Created attachment 440655 [details] Patch After talking with @Myles Maxfield, we believe that the failing `ASSERT` is no longer (and may never have been) accurate/needed. CT is allowed to modify the text position between the `CGContextSetTextPosition` call and `DrawGlyphsRecorder::recordDrawGlyphs`. I tested this by calling `CGContextSetTextPosition` with manually overridden coordinates and visually confirming that the output was moved by the relevant amount.

Created attachment 440688 [details] Patch rebase

Created attachment 440689 [details] Patch rebase

Committed r283863 (242740@main): <https://commits.webkit.org/242740@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 440689 [details].