Bug 236185 - [LBSE] Begin stub implementation of transform support for SVG layers
Summary: [LBSE] Begin stub implementation of transform support for SVG 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:
Blocks: 90738 236194
  Show dependency treegraph
 
Reported: 2022-02-05 16:19 PST by Nikolas Zimmermann
Modified: 2022-02-11 00:05 PST (History)
17 users (show)

See Also:


Attachments
Patch, v1 (6.91 KB, patch)
2022-02-05 16:27 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v2 (20.88 KB, patch)
2022-02-08 04:43 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v3 (21.06 KB, patch)
2022-02-09 04:43 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v4 (21.07 KB, patch)
2022-02-10 03:47 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v5 (21.04 KB, patch)
2022-02-10 06:00 PST, Nikolas Zimmermann
rbuis: review+
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-02-05 16:19:32 PST
Prepare RenderLayer/RenderLayerBacking for RenderSVGModelObject support:

Add stubs in all relevant places that deal with transformations in RenderLayer/RenderLayerBacking where RenderSVGModelObject support needs to be added. The actual implementation of the SVG transformation <-> RenderLayer integration belongs to a separated patch series, therefore this patch only contains the necessary stub implementation.
Comment 1 Nikolas Zimmermann 2022-02-05 16:27:16 PST
Created attachment 451010 [details]
Patch, v1
Comment 2 Rob Buis 2022-02-07 06:32:29 PST
Comment on attachment 451010 [details]
Patch, v1

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

> Source/WebCore/ChangeLog:10
> +        that hooks in SVG transformations within layers will follow in seperated

separated.

> Source/WebCore/rendering/RenderLayer.cpp:1336
> +                m_transform->makeIdentity();

Ditto.

> Source/WebCore/rendering/RenderLayer.cpp:1340
> +            return;

Is the logic as wanted? SVG always returns here now.

> Source/WebCore/rendering/RenderLayerBacking.cpp:667
> +                t.makeIdentity();

I am guessing TransformationMatrix is identity by default? If so this does not seem needed (even though I know it is temporary).

> Source/WebCore/rendering/RenderLayerBacking.cpp:671
> +            return;

Ditto.
Comment 3 Nikolas Zimmermann 2022-02-08 01:48:04 PST
(In reply to Rob Buis from comment #2)
> Comment on attachment 451010 [details]
> Patch, v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451010&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        that hooks in SVG transformations within layers will follow in seperated
> 
> separated.
Thanks.

> 
> > Source/WebCore/rendering/RenderLayer.cpp:1336
> > +                m_transform->makeIdentity();
> 
> Ditto.
Yeah, it's a hack to let me enclose the FIXME comment in braces, otherwise style check complains about the brace. Is there another preferred way to write this?

> 
> > Source/WebCore/rendering/RenderLayer.cpp:1340
> > +            return;
> 
> Is the logic as wanted? SVG always returns here now.
No it's not, good catch, it's not supposed to return. It doesn't matter at the moment, as the transformation is identity anyhow, but it matters when we add transformations (next patch series).

> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:667
> > +                t.makeIdentity();
> 
> I am guessing TransformationMatrix is identity by default? If so this does
> not seem needed (even though I know it is temporary).
See above, only a hack. (void) t; would do as well, or UNUSED_PARAM(t), however none really feels right.
 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:671
> > +            return;
> 
> Ditto.

Will fix as well, once we agree on avoiding makeIdentity() hack.
Comment 4 Rob Buis 2022-02-08 02:24:14 PST
Comment on attachment 451010 [details]
Patch, v1

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

>>> Source/WebCore/rendering/RenderLayerBacking.cpp:667
>>> +                t.makeIdentity();
>> 
>> I am guessing TransformationMatrix is identity by default? If so this does not seem needed (even though I know it is temporary).
> 
> See above, only a hack. (void) t; would do as well, or UNUSED_PARAM(t), however none really feels right.

I think it is good enough if you add an extra FIXME to remove the makeIdentity call.
Comment 5 Nikolas Zimmermann 2022-02-08 04:43:46 PST
Created attachment 451237 [details]
Patch, v2
Comment 6 Nikolas Zimmermann 2022-02-08 04:44:53 PST
(In reply to Rob Buis from comment #4)
> I think it is good enough if you add an extra FIXME to remove the
> makeIdentity call.

As discussed, this patch is a bit too thin, I could at least add the new 'applyTransform()' helper methods, and add the // FIXME: Add support for SVG comments there, instead of adding more branches to updateTransform(), making it harder to read. See v2.
Comment 7 Rob Buis 2022-02-08 04:49:54 PST
Comment on attachment 451237 [details]
Patch, v2

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

> Source/WebCore/rendering/RenderLayer.h:994
> +#endif

Does this need an ASSERT_NOT_REACHED here?
Comment 8 Nikolas Zimmermann 2022-02-09 04:43:15 PST
Created attachment 451360 [details]
Patch, v3
Comment 9 Nikolas Zimmermann 2022-02-09 04:43:28 PST
(In reply to Rob Buis from comment #7)
> Comment on attachment 451237 [details]
> Patch, v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451237&action=review
> 
> > Source/WebCore/rendering/RenderLayer.h:994
> > +#endif
> 
> Does this need an ASSERT_NOT_REACHED here?
Done.
Comment 10 Nikolas Zimmermann 2022-02-10 03:38:20 PST
Holding back commit until EWS is happy - investigating.
Comment 11 Nikolas Zimmermann 2022-02-10 03:41:59 PST
Comment on attachment 451360 [details]
Patch, v3

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

> Source/WebCore/rendering/RenderLayer.cpp:-1334
> -        m_transform->makeIdentity();

Bazinga, we're now accumulating transforms due to the accidental removal of this line while refactoring in the latest iteration of this patch :-) Good that we have EWS.
Comment 12 Nikolas Zimmermann 2022-02-10 03:47:10 PST
Created attachment 451511 [details]
Patch, v4
Comment 13 Nikolas Zimmermann 2022-02-10 06:00:19 PST
Hehe, too much refactoring, new problems:

frame #2: 0x0000000284adb0f0 WebCore`WebCore::RenderLayer::rendererLocation(this=0x000000010753b7b0) const at RenderLayer.h:985:9
   982               return downcast<RenderSVGModelObject>(renderer()).layoutLocation();
   983   #endif
   984   
-> 985           ASSERT_NOT_REACHED();
   986           return LayoutPoint();
   987       }
   988   
(lldb) p renderer().renderName()
(const char *) $0 = 0x0000000286a66d15 "RenderInline"

rendererLocation() is called for RenderInlines, just not for RenderSVGInlines -- therefore the ASSERT_NOT_REACHED() in rendererLocation() is wrong.

Previously we had: LayoutPoint renderBoxLocation() const { return is<RenderBox>(renderer()) ? downcast<RenderBox>(renderer()).location() : LayoutPoint(); }

For RenderInlines that create RenderLayers the renderBoxLocation() == rendererLocation() is defined to be LayoutPoint() -- will adapt the patch once again.
Comment 14 Nikolas Zimmermann 2022-02-10 06:00:55 PST
Created attachment 451523 [details]
Patch, v5
Comment 15 Nikolas Zimmermann 2022-02-11 00:04:21 PST
Committed r289606 (247119@trunk): <https://commits.webkit.org/247119@trunk>
Comment 16 Radar WebKit Bug Importer 2022-02-11 00:05:17 PST
<rdar://problem/88802071>