Bug 238066 – [GPU Process] [GraphicsContextState 2/] Make GraphicsContextState keep track of changes till they are applied

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

A member of type GraphicsContextState::ChangeFlags will be added to GraphicsContextState to keep track of what has changes since it was last applied. We will eliminate the struct GraphicsContextStateChange.

We will have to have a single member for every GraphicsContextState::Change. 

1. We will combine the color, the pattern, the gradient and the gradient space transform in one class called SourceBrush.
2. We will combine the shadow offset, the shadow blurRadius, the show color and the radius mode in one struct called DropShadow
3. We will combine the CompositeOperator and the BledMode in one struct called CompositeMode.

GraphicsContextStateChange will handle setting its members, its encoding and decoding and its streaming to text.

[Said Abou-Hallawa]

Created attachment 455069 [details]
Patch

[Said Abou-Hallawa]

Created attachment 455070 [details]
Patch

[Said Abou-Hallawa]

Created attachment 455075 [details]
Patch

[Said Abou-Hallawa]

Created attachment 455077 [details]
Patch

[Said Abou-Hallawa]

Created attachment 455079 [details]
Patch

[Said Abou-Hallawa]

Created attachment 455143 [details]
Investigate Cairo failures

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Said Abou-Hallawa]

Created attachment 455154 [details]
Patch

[Said Abou-Hallawa]

Created attachment 455190 [details]
Patch

[Said Abou-Hallawa]

Created attachment 455192 [details]
Patch

[Said Abou-Hallawa]

Created attachment 455211 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 455211 [details]
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.

[Radar WebKit Bug Importer]

<rdar://problem/90585183>

[Said Abou-Hallawa]

Created attachment 455298 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 455211 [details]
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.

But this is also called by DisplayList::Recorder.

>> 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.

I called it isInlineColor().

>> 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.

I added the function SourceBrush::isVisible() to replace these checks. And it is okay to check or not check the gradient because the only caller to this function (i.e. drawPath()) handles the gradient case separately before calling this function.

[Said Abou-Hallawa]

Created attachment 455326 [details]
Patch

[EWS]

Committed r291604 (248697@main): <https://commits.webkit.org/248697@main>

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

[Jon Lee]

*** Bug 236919 has been marked as a duplicate of this bug. ***

[Jon Lee]

*** Bug 237685 has been marked as a duplicate of this bug. ***

[Jon Lee]

*** Bug 237686 has been marked as a duplicate of this bug. ***