[webkit-reviews] review granted: [Bug 230781] [iOS][GPU Process] support `<attachment>` : [Attachment 439496] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 230781: [iOS][GPU Process] support `<attachment>`
https://bugs.webkit.org/show_bug.cgi?id=230781

Attachment 439496: Patch

https://bugs.webkit.org/attachment.cgi?id=439496&action=review




--- Comment #4 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 439496
  --> https://bugs.webkit.org/attachment.cgi?id=439496
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/DisplayListDrawGlyphsRecorderCore
Text.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?