[webkit-reviews] review granted: [Bug 238066] [GPU Process] [GraphicsContextState 2/] Make GraphicsContextState keep track of changes till they are applied : [Attachment 455211] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 238066: [GPU Process] [GraphicsContextState 2/] Make GraphicsContextState
keep track of changes till they are applied
https://bugs.webkit.org/show_bug.cgi?id=238066

Attachment 455211: Patch

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




--- Comment #12 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 455211
  --> https://bugs.webkit.org/attachment.cgi?id=455211
Patch

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

> Source/WebCore/platform/graphics/GraphicsContextState.cpp:89
> +#if USE(CG)

Ideally this would be in GraphicsContextCG somewhere.

> Source/WebCore/platform/graphics/GraphicsContextState.cpp:100
> +#if USE(CG)
> +    // CGContextBeginTransparencyLayer() sets the CG global alpha to 1.
Resore our alpha now.
> +    alpha = originalOpacity;
> +#else

Ditto

> Source/WebCore/platform/graphics/GraphicsContextState.cpp:136
> +    unsigned index = static_cast<unsigned>(log2(mask));
> +    return stateChangeNames[index];

I think a switch would be preferable. It's too easy for this to get out of
sync.

> Source/WebCore/platform/graphics/GraphicsTypes.cpp:162
> +	   ts << "DO-NOT-INTERPOLATE";

lowercase please

> Source/WebCore/platform/graphics/GraphicsTypes.cpp:171
> +	   ts << "HEIGH";

spelling

> Source/WebCore/platform/graphics/GraphicsTypes.cpp:227
> +	   ts << "NO-STROKE";

lowercase please

> Source/WebCore/platform/graphics/GraphicsTypes.cpp:252
> +	   ts << "FILL";

lowercase

> Source/WebCore/platform/graphics/SourceBrush.h:66
> +    bool isPrimitive() const { return !m_brush &&
m_color.tryGetAsSRGBABytes(); }

It's not clear what "primitive" means here. Seems like it's asking if it's a
inline sRGB color.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:597
> +    bool shouldFill = context.fillPattern() ||
context.fillColor().isVisible();

Should this be calling a function on the SourceBrush? From reading this, it's
not clear if a gradient fill will return true.