Bug 230913 – Allow `DrawGlyphsRecorder` to be used with any `GraphicsContext` instead of just `DisplayList::Recorder`

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

[Devin Rousso]

there's really nothing about `DrawGlyphsRecorder` that's specific to display lists other than it only being used by `DisplayList::Recorder`

[Devin Rousso]

Created attachment 439530 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 439530 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439530&action=review

> Source/WebCore/platform/graphics/GraphicsContext.h:358
> +    virtual const GraphicsContextState& lastStateChange() const { return state(); }

Still don't like the word "change"

> Source/WebCore/platform/graphics/GraphicsContext.h:477
> +        drawGlyphs(font, glyphs, advances, numGlyphs, point, fontSmoothingMode);

If all the arguments are identical, why doesn't DrawGlyphsRecorder just call drawGlyphs() instead of this renamed function?

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:153
> +const GraphicsContextState& Recorder::lastStateChange() const

ditto

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:97
> +    const GraphicsContextState& lastStateChange() const final;

ditto.

[Devin Rousso]

Comment on attachment 439530 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439530&action=review

>> Source/WebCore/platform/graphics/GraphicsContext.h:358
>> +    virtual const GraphicsContextState& lastStateChange() const { return state(); }
> 
> Still don't like the word "change"

Do you have any suggestions as to a different name?

Alternatively, do you know why `m_owner.currentState().stateChange.m_state` was written the way it was originally?  Perhaps if I understand that then I can come up with a better name :)

>> Source/WebCore/platform/graphics/GraphicsContext.h:477
>> +        drawGlyphs(font, glyphs, advances, numGlyphs, point, fontSmoothingMode);
> 
> If all the arguments are identical, why doesn't DrawGlyphsRecorder just call drawGlyphs() instead of this renamed function?

`drawGlyphs` is what calls into `DrawGlyphsRecorder`, which then calls into `drawGlyphsAndCacheFont` (which `DisplayList::Recorder` overrides to append a `DrawGlyphs` display list item)

Also FWIW this code already existed before.  I just moved it from being only on `DisplayList::Recorder` to also being on `GraphicsContext`.  Currently the only caller of `DrawGlyphsRecorder::drawGlyphs` is `DisplayList::Recorder::drawGlyphs`, meaning that we need some other method that `DrawGlyphsRecorder` can call once it's done its work to pass it back to the `DisplayList::Recorder`.  I'm open to other naming ideas if you have any suggestions :)

[Devin Rousso]

Created attachment 439542 [details]
Patch

[Wenson Hsieh]

Comment on attachment 439542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439542&action=review

> Source/WebCore/ChangeLog:41
> +        for the `state` "getter" so that `DrawGlypsRecorder` is able to access the current state in

Nit - DrawGlypsRecorder

[Devin Rousso]

Created attachment 439664 [details]
Patch

fix typo in ChangeLog

[EWS]

Committed r283273 (242302@main): <https://commits.webkit.org/242302@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439664 [details].

[Radar WebKit Bug Importer]

<rdar://problem/83693148>