Bug 236185

Created attachment 451010 [details] Patch, v1

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.

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

Created attachment 451237 [details] Patch, v2

(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 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?

Created attachment 451360 [details] Patch, v3

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

Holding back commit until EWS is happy - investigating.

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.

Created attachment 451511 [details] Patch, v4

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.

Created attachment 451523 [details] Patch, v5

Committed r289606 (247119@trunk): <https://commits.webkit.org/247119@trunk>

<rdar://problem/88802071>