Summary: | [iOS][GPU Process] support `<attachment>` | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||||||||||
Component: | New Bugs | Assignee: | Devin Rousso <hi> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | annulen, ayumi_kojima, changseok, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, hi, kondapallykalyan, mmaxfield, pdr, ryuan.choi, sergio, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=230647 | ||||||||||||||||||||
Bug Depends on: | 230912, 230913 | ||||||||||||||||||||
Bug Blocks: | 231244, 232645 | ||||||||||||||||||||
Attachments: |
|
Description
Devin Rousso
2021-09-24 18:21:16 PDT
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]. |