Bug 230781 – [iOS][GPU Process] support `<attachment>`

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Devin Rousso]

.

[Devin Rousso]

<rdar://problem/70884096>

[Devin Rousso]

Created attachment 439228 [details]
Patch

very basic/straightforward initial approach

[Devin Rousso]

Created attachment 439496 [details]
Patch

[Myles C. Maxfield]

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?

[Devin Rousso]

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).

[Tim Horton]

Comment on attachment 439496 [details]
Patch

It might (optionally, not saying you have to) be better to separate the rearchitecture and functional change.

[Devin Rousso]

Created attachment 439548 [details]
Patch

[Devin Rousso]

Created attachment 439771 [details]
Patch

[Devin Rousso]

Created attachment 439774 [details]
Patch

[EWS]

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>

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 440688 [details]
Patch

rebase

[Devin Rousso]

Created attachment 440689 [details]
Patch

rebase

[EWS]

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].