Bug 230781 - [iOS][GPU Process] support `<attachment>`
Summary: [iOS][GPU Process] support `<attachment>`
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 230912 230913
Blocks: 231244
  Show dependency treegraph
 
Reported: 2021-09-24 18:21 PDT by Devin Rousso
Modified: 2021-10-08 23:18 PDT (History)
17 users (show)

See Also:


Attachments
Patch (3.16 KB, patch)
2021-09-24 19:00 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (88.16 KB, patch)
2021-09-28 11:10 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (20.00 KB, patch)
2021-09-28 17:16 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (20.03 KB, patch)
2021-09-30 12:37 PDT, Devin Rousso
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (20.03 KB, patch)
2021-09-30 12:58 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (20.36 KB, patch)
2021-10-08 12:02 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (19.97 KB, patch)
2021-10-08 16:56 PDT, Devin Rousso
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (22.51 KB, patch)
2021-10-08 17:06 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2021-09-24 18:21:16 PDT
.
Comment 1 Devin Rousso 2021-09-24 18:26:30 PDT
<rdar://problem/70884096>
Comment 2 Devin Rousso 2021-09-24 19:00:39 PDT
Created attachment 439228 [details]
Patch

very basic/straightforward initial approach
Comment 3 Devin Rousso 2021-09-28 11:10:55 PDT
Created attachment 439496 [details]
Patch
Comment 4 Myles C. Maxfield 2021-09-28 12:00:54 PDT
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 5 Devin Rousso 2021-09-28 12:19:24 PDT
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 6 Tim Horton 2021-09-28 12:25:13 PDT
Comment on attachment 439496 [details]
Patch

It might (optionally, not saying you have to) be better to separate the rearchitecture and functional change.
Comment 7 Devin Rousso 2021-09-28 17:16:25 PDT
Created attachment 439548 [details]
Patch
Comment 8 Devin Rousso 2021-09-30 12:37:42 PDT
Created attachment 439771 [details]
Patch
Comment 9 Devin Rousso 2021-09-30 12:58:51 PDT
Created attachment 439774 [details]
Patch
Comment 10 EWS 2021-09-30 14:13:12 PDT
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].
Comment 11 ayumi_kojima 2021-10-05 15:29:31 PDT
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>
Comment 12 Devin Rousso 2021-10-08 12:02:32 PDT
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.
Comment 13 Devin Rousso 2021-10-08 16:56:06 PDT
Created attachment 440688 [details]
Patch

rebase
Comment 14 Devin Rousso 2021-10-08 17:06:02 PDT
Created attachment 440689 [details]
Patch

rebase
Comment 15 EWS 2021-10-08 23:18:27 PDT
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].