Bug 234954

Created attachment 448582 [details] Patch, v1

Created attachment 448584 [details] Patch, v2

Comment on attachment 448582 [details] Patch, v1 View in context: https://bugs.webkit.org/attachment.cgi?id=448582&action=review > Source/WebCore/ChangeLog:10 > + layout / paint / hit-testing looks quite, much more similar to RenderBox & co quite what? :) > Source/WebCore/rendering/svg/RenderSVGResource.cpp:251 > + return; Maybe instead there could be an ASSERT that this does not happen? AFAICS only filter provide nullptr for both but that seems a different code path. > Source/WebCore/rendering/svg/RenderSVGResource.cpp:254 > + ASSERT(shape->isSVGShapeOrLegacySVGShape()); How about ASSERT(!shape || shape->isSVGShapeOrLegacySVGShape()); ? > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:32 > +#include "LegacyRenderSVGShape.h" Is this needed? > Source/WebCore/svg/SVGAnimateMotionElement.cpp:28 > +#include "RenderElement.h" Ditto? > Source/WebCore/svg/SVGLineElement.cpp:25 > +#include "LegacyRenderSVGShape.h" Ditto?

(In reply to Rob Buis from comment #3) Thanks for the first review. > > Source/WebCore/ChangeLog:10 > > + layout / paint / hit-testing looks quite, much more similar to RenderBox & co > > quite what? :) different :-) > > Source/WebCore/rendering/svg/RenderSVGResource.cpp:251 > > + return; > > Maybe instead there could be an ASSERT that this does not happen? AFAICS > only filter provide nullptr for both but that seems a different code path. > > > Source/WebCore/rendering/svg/RenderSVGResource.cpp:254 > > + ASSERT(shape->isSVGShapeOrLegacySVGShape()); > > How about ASSERT(!shape || shape->isSVGShapeOrLegacySVGShape()); ? Having two statements in an assertion doesn't tell you precisely what failed, which is annoying :-) However we could turn both the if (!shape && !path) return and the if(shape) ASSERT... into: if (shape) ASSERT(shape->isSVGShapeOrLegacySVGShape()); else ASSERT(path); This is even more ugly, IMHO. So I went for: void RenderSVGResource::fillAndStrokePathOrShape(..) { { if (shape) { ASSERT(shape->isSVGShapeOrLegacySVGShape()); if (resourceMode.contains(RenderSVGResourceMode::ApplyToFill)) { if (is<LegacyRenderSVGShape>(shape)) downcast<LegacyRenderSVGShape>(shape)->fillShape(context); ... } if (resourceMode.contains(RenderSVGResourceMode::ApplyToStroke)) { if (is<LegacyRenderSVGShape>(shape)) downcast<LegacyRenderSVGShape>(shape)->strokeShape(context); ... } return; } if (path) { if (resourceMode.contains(RenderSVGResourceMode::ApplyToFill)) context.fillPath(*path); if (resourceMode.contains(RenderSVGResourceMode::ApplyToStroke)) context.strokePath(*path); return; } ASSERT_NOT_REACHED(); } That is more pleasant to my eyes :-) Fine with you? > > > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:32 > > +#include "LegacyRenderSVGShape.h" > > Is this needed? Leftover, probably not. > > > Source/WebCore/svg/SVGAnimateMotionElement.cpp:28 > > +#include "RenderElement.h" > > Ditto? Nope on purpose, I removed a RenderElement.h include in an unrelated place, and this is fixing the wrong places, that forgot the include before. > > Source/WebCore/svg/SVGLineElement.cpp:25 > > +#include "LegacyRenderSVGShape.h" > > Ditto? Nope, (caught by WinCairo EWS).

> > > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:32 > > > +#include "LegacyRenderSVGShape.h" > > > > Is this needed? > Leftover, probably not. ./rendering/svg/SVGRenderSupport.cpp:161:16: error: unknown type name 'LegacyRenderSVGShape'; did you mean 'LegacyRenderSVGRoot'? if (is<LegacyRenderSVGShape>(current) && downcast<LegacyRenderSVGShape>(current).isRenderingDisabled()) ^~~~~~~~~~~~~~~~~~~~ LegacyRenderSVGRoot The new include is needed, as I've removed the LegacyRenderSVGShape.h include from RenderSVGResource.h, which is no longer needed there, since postApplyResource() is now taking a RenderElement (common-base class for LegacyRenderSVGShape/RenderSVGShape) instead of a LegacyRenderSVGShape. I've also double-checked that all places that handle LegacyRenderSVGShape, also handle RenderSVGShape now. Previously I missed SVGGeometryElement (getTotalLength() / etc.). I will include all fixes in a new patch, that also hopefully satisfies win/wincairo builds.

Created attachment 448628 [details] Patch, v3

Created attachment 448637 [details] Patch, v4

(In reply to Rob Buis from comment #3)> > > Source/WebCore/rendering/svg/RenderSVGResource.cpp:251 > > + return; > > Maybe instead there could be an ASSERT that this does not happen? AFAICS > only filter provide nullptr for both but that seems a different code path. EWS lectured me: shape=path=nullptr is possible. void SVGInlineTextBox::restoreGraphicsContextAfterTextPainting(GraphicsContext*& context) { releasePaintingResource(context, /* path */nullptr); -> void SVGInlineTextBox::releasePaintingResource(GraphicsContext*& context, const Path* path) { ASSERT(m_paintingResource); m_paintingResource->postApplyResource(parent()->renderer(), context, paintingResourceMode(), path, /*LegacyRenderSVGShape*/ nullptr); -> both shape and path are nullptr. The ASSERT_NOT_REACHED() has to go, or we fix the call sites. However, due to the scope of this patch, it's better to remove the ASSERT_NOT_REACHED.

Created attachment 448666 [details] Patch, v5

Comment on attachment 448666 [details] Patch, v5 View in context: https://bugs.webkit.org/attachment.cgi?id=448666&action=review > Source/WebCore/rendering/svg/RenderSVGResource.cpp:280 > + return; Not needed. > Source/WebCore/rendering/svg/RenderSVGResourceSolidColor.h:23 > +#include "FloatRect.h" Is this needed?

(In reply to Rob Buis from comment #10) > Comment on attachment 448666 [details] > Patch, v5 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=448666&action=review > > > Source/WebCore/rendering/svg/RenderSVGResource.cpp:280 > > + return; > > Not needed. True. But then I could also make it if (!path) return early return.. > > > Source/WebCore/rendering/svg/RenderSVGResourceSolidColor.h:23 > > +#include "FloatRect.h" > > Is this needed? It is, we use FloatRect() in resourceBoundingBox(), and no other class now provides this include.

Comment on attachment 448666 [details] Patch, v5 View in context: https://bugs.webkit.org/attachment.cgi?id=448666&action=review >>> Source/WebCore/rendering/svg/RenderSVGResource.cpp:280 >>> + return; >> >> Not needed. > > True. But then I could also make it if (!path) return early return.. Actually, that is probably preferred, it was also remove indentation.

Committed r287832 (245884@trunk): <https://commits.webkit.org/245884@trunk>

<rdar://problem/87330324>