RESOLVED FIXED 238066
[GPU Process] [GraphicsContextState 2/] Make GraphicsContextState keep track of changes till they are applied 
https://bugs.webkit.org/show_bug.cgi?id=238066
Summary [GPU Process] [GraphicsContextState 2/] Make GraphicsContextState keep track ...
Said Abou-Hallawa
Reported 2022-03-17 23:47:34 PDT
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.
Attachments
Patch (169.36 KB, patch)
2022-03-17 23:54 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (168.32 KB, patch)
2022-03-18 00:00 PDT, Said Abou-Hallawa 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (188.20 KB, patch)
2022-03-18 01:05 PDT, Said Abou-Hallawa 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (189.05 KB, patch)
2022-03-18 01:32 PDT, Said Abou-Hallawa 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (192.44 KB, patch)
2022-03-18 02:02 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Investigate Cairo failures (194.91 KB, patch)
2022-03-18 15:25 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (223.44 KB, patch)
2022-03-18 17:26 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (223.21 KB, patch)
2022-03-20 00:26 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (223.31 KB, patch)
2022-03-20 02:27 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (216.80 KB, patch)
2022-03-20 19:28 PDT, Said Abou-Hallawa 		simon.fraser: review+
ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (216.84 KB, patch)
2022-03-21 16:43 PDT, Said Abou-Hallawa 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (216.94 KB, patch)
2022-03-21 20:52 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.
Said Abou-Hallawa
Comment 1 2022-03-17 23:54:53 PDT
Created attachment 455069 [details] Patch
Said Abou-Hallawa
Comment 2 2022-03-18 00:00:16 PDT
Created attachment 455070 [details] Patch
Said Abou-Hallawa
Comment 3 2022-03-18 01:05:53 PDT
Created attachment 455075 [details] Patch
Said Abou-Hallawa
Comment 4 2022-03-18 01:32:14 PDT
Created attachment 455077 [details] Patch
Said Abou-Hallawa
Comment 5 2022-03-18 02:02:43 PDT
Created attachment 455079 [details] Patch
Said Abou-Hallawa
Comment 6 2022-03-18 15:25:44 PDT
Created attachment 455143 [details] Investigate Cairo failures
EWS Watchlist
Comment 7 2022-03-18 15:27:12 PDT
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
Comment 8 2022-03-18 17:26:21 PDT
Created attachment 455154 [details] Patch
Said Abou-Hallawa
Comment 9 2022-03-20 00:26:46 PDT
Created attachment 455190 [details] Patch
Said Abou-Hallawa
Comment 10 2022-03-20 02:27:18 PDT
Created attachment 455192 [details] Patch
Said Abou-Hallawa
Comment 11 2022-03-20 19:28:23 PDT
Created attachment 455211 [details] Patch
Simon Fraser (smfr)
Comment 12 2022-03-21 11:10:36 PDT
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
Comment 13 2022-03-21 12:31:13 PDT
<rdar://problem/90585183>
Said Abou-Hallawa
Comment 14 2022-03-21 16:43:42 PDT
Created attachment 455298 [details] Patch
Said Abou-Hallawa
Comment 15 2022-03-21 18:08:03 PDT
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
Comment 16 2022-03-21 20:52:06 PDT
Created attachment 455326 [details] Patch
EWS
Comment 17 2022-03-21 22:27:38 PDT
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
Comment 18 2022-03-22 13:07:42 PDT
*** Bug 236919 has been marked as a duplicate of this bug. ***
Jon Lee
Comment 19 2022-03-22 16:48:28 PDT
*** Bug 237685 has been marked as a duplicate of this bug. ***
Jon Lee
Comment 20 2022-03-22 16:49:20 PDT
*** Bug 237686 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.