Bug 237711 - [LBSE] Activate SVG transform support through layers
Summary: [LBSE] Activate SVG transform support through layers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords: InRadar
Depends on: 237553 237554 237590 237701 239060
Blocks: 90738 239717
  Show dependency treegraph
 
Reported: 2022-03-10 05:58 PST by Nikolas Zimmermann
Modified: 2022-04-25 06:46 PDT (History)
19 users (show)

See Also:


Attachments
Patch, v1 (25.05 KB, patch)
2022-03-10 06:02 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v2 (27.12 KB, patch)
2022-03-10 06:05 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v3 (25.57 KB, patch)
2022-03-10 15:43 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v6 (38.49 KB, text/plain)
2022-03-13 16:43 PDT, Nikolas Zimmermann
no flags Details
Patch, v4 (25.78 KB, patch)
2022-03-13 16:49 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v5 (25.75 KB, patch)
2022-03-16 03:27 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v6 (25.75 KB, patch)
2022-03-21 02:55 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v7 (25.75 KB, patch)
2022-04-04 13:36 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v8 (25.71 KB, patch)
2022-04-05 14:59 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v9 (25.76 KB, patch)
2022-04-07 03:31 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v10 (25.66 KB, patch)
2022-04-07 15:59 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v11 (25.91 KB, patch)
2022-04-11 04:30 PDT, Nikolas Zimmermann
rbuis: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2022-03-10 05:58:21 PST
Finally activate SVG transform support for containers & shapes in LBSE.
Comment 1 Nikolas Zimmermann 2022-03-10 06:02:07 PST
Created attachment 454350 [details]
Patch, v1
Comment 2 Nikolas Zimmermann 2022-03-10 06:02:24 PST
Delaying EWS submission until the dependency patches are landed.
Comment 3 Nikolas Zimmermann 2022-03-10 06:05:39 PST
Created attachment 454351 [details]
Patch, v2
Comment 4 Nikolas Zimmermann 2022-03-10 15:43:30 PST
Created attachment 454412 [details]
Patch, v3
Comment 5 Nikolas Zimmermann 2022-03-13 16:43:47 PDT
Created attachment 454559 [details]
Patch, v6

The latest iteration of the patch introduces FloatOrLayoutRect, which either holds a FloatRect or LayoutRect (realized via variant<FloatRect, LayoutRect>) -- this avoids all LayoutRect(fromFloatRect) hacks, present in previous iterations of the patch and guarantees type-safety again. As benefit it allows us to detect if we need to pixel snap or not (SVG stores FloatRects, no pixel snapping should occour), simplifying quite a few parts in RenderLayer
Comment 6 Nikolas Zimmermann 2022-03-13 16:49:13 PDT
Created attachment 454562 [details]
Patch, v4
Comment 7 Nikolas Zimmermann 2022-03-13 16:50:18 PDT
(In reply to Nikolas Zimmermann from comment #5)
> Created attachment 454559 [details]
> Patch, v6
> 
> The latest iteration of the patch introduces FloatOrLayoutRect, which either
> holds a FloatRect or LayoutRect (realized via variant<FloatRect,
> LayoutRect>) -- this avoids all LayoutRect(fromFloatRect) hacks, present in
> previous iterations of the patch and guarantees type-safety again. As
> benefit it allows us to detect if we need to pixel snap or not (SVG stores
> FloatRects, no pixel snapping should occour), simplifying quite a few parts
> in RenderLayer
Please ignore this comment, this was supposed to go onto ticket 237554, not here.
Comment 8 Nikolas Zimmermann 2022-03-16 03:27:28 PDT
Created attachment 454818 [details]
Patch, v5
Comment 9 Radar WebKit Bug Importer 2022-03-16 05:38:19 PDT
<rdar://problem/90364832>
Comment 10 Nikolas Zimmermann 2022-03-21 02:55:34 PDT
Created attachment 455227 [details]
Patch, v6
Comment 11 Nikolas Zimmermann 2022-04-04 13:36:19 PDT
Created attachment 456619 [details]
Patch, v7
Comment 12 Nikolas Zimmermann 2022-04-05 14:59:50 PDT
Created attachment 456754 [details]
Patch, v8
Comment 13 Nikolas Zimmermann 2022-04-07 03:31:31 PDT
Created attachment 456902 [details]
Patch, v9
Comment 14 Nikolas Zimmermann 2022-04-07 03:33:04 PDT
Depends on 237701, hence the build failures.
Comment 15 Nikolas Zimmermann 2022-04-07 15:59:43 PDT
Created attachment 456979 [details]
Patch, v10
Comment 16 Nikolas Zimmermann 2022-04-10 15:45:39 PDT
The mac-wk1 / mac-wk2 failures are multicol related, not affected by this patch - tested on my machine. There appears to be general flakiness with respect to multicol layout at present?
Comment 17 Nikolas Zimmermann 2022-04-10 15:52:09 PDT
Committed r292690 (249482@trunk): <https://commits.webkit.org/249482@trunk>
Comment 18 Fujii Hironori 2022-04-10 21:13:33 PDT
The EWS result didn't seem false positive.
Some multicol tests are ramdomly failing since 249482@trunk.
See the history of the result.
https://build.webkit.org/results/Apple-Monterey-Debug-AppleSilicon-WK2-Tests/r292690%20(1670)/results.html
Comment 19 WebKit Commit Bot 2022-04-10 21:19:57 PDT
Re-opened since this is blocked by bug 239060
Comment 20 Nikolas Zimmermann 2022-04-11 04:29:48 PDT
(In reply to Fujii Hironori from comment #18)
> The EWS result didn't seem false positive.
> Some multicol tests are ramdomly failing since 249482@trunk.
> See the history of the result.
> https://build.webkit.org/results/Apple-Monterey-Debug-AppleSilicon-WK2-Tests/
> r292690%20(1670)/results.html

Thanks for taking care Fujii - indeed I was misremembering two things: we do build with LBSE enabled by default on macOS, GTK & WPE -- I thought that the changes in the #if ENABLE(LBSE)... blocks are unrelated to the EWS failures :-(

And indeed I saw the flakiness before, however not on results.webkit.org, but a private dashboard ... due to my patch:

A classic one -- uninitialized variable:

diff --git a/Source/WebCore/rendering/RenderObject.cpp b/Source/WebCore/rendering/RenderObject.cpp
index 8a4d80f2b642..a65a7517b56e 100644
--- a/Source/WebCore/rendering/RenderObject.cpp
+++ b/Source/WebCore/rendering/RenderObject.cpp
@@ -2030,6 +2030,9 @@ RenderObject::RenderObjectRareData::RenderObjectRareData()
     , m_isRenderFragmentedFlow(false)
     , m_hasOutlineAutoAncestor(false)
     , m_paintContainmentApplies(false)
+#if ENABLE(LAYER_BASED_SVG_ENGINE)
+    , m_hasSVGTransform(false)
+#endif
 {
 }
 
 That solves the mystery. Will add another patch, and ask Rob for another review.
Side note: We should get rid of ADD_BOOLEAN_BITFIELD() -- the code is dangerous, we have to define the bitfields in the header, and initialize them in the source file.

That leads to bugs like this one, which can be avoided with in-place initialization... I'll note this down for a rainy afternoon.
Comment 21 Nikolas Zimmermann 2022-04-11 04:30:11 PDT
Created attachment 457243 [details]
Patch, v11
Comment 22 Nikolas Zimmermann 2022-04-11 05:54:11 PDT
Committed r292706 (249498@trunk): <https://commits.webkit.org/249498@trunk>