[webkit-reviews] review granted: [Bug 204204] Updating a polygon referenced by clip-path doesn't repaint : [Attachment 436960] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 204204: Updating a polygon referenced by clip-path doesn't repaint
https://bugs.webkit.org/show_bug.cgi?id=204204

Attachment 436960: Patch

https://bugs.webkit.org/attachment.cgi?id=436960&action=review




--- Comment #9 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 436960
  --> https://bugs.webkit.org/attachment.cgi?id=436960
Patch

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

r=me with stable test

> Source/WebCore/rendering/ReferencedSVGResources.cpp:39
> +class CSSSVGResourceElementClient final : public SVGResourceElementClient {

Why subclassing? Do we expect other SVGResourceElementClients?

> Source/WebCore/rendering/ReferencedSVGResources.cpp:50
> +    RenderElement& m_clientRenderer;

Ryosuke would tell you to use WeakPtr

> Source/WebCore/rendering/ReferencedSVGResources.cpp:92
> +HashMap<AtomString, QualifiedName>
ReferencedSVGResources::referencedSVGResourceIDs(const RenderStyle& style)
> +{
> +    HashMap<AtomString, QualifiedName> referencedResourceIDToTagNameMap;
> +    if (is<ReferenceClipPathOperation>(style.clipPath())) {
> +	   auto& clipPath =
downcast<ReferenceClipPathOperation>(*style.clipPath());
> +	   if (!clipPath.fragment().isEmpty())
> +	       referencedResourceIDToTagNameMap.add(clipPath.fragment(),
SVGNames::clipPathTag);
> +    }

Wouldn't HashSet<std::pair<AtomString, QualifiedName>> be a more natural type
for this? Or just Vector<std::pair<>> as they are going to be stuffed into a
HashMap later anyway.

I think with HashMap referencing a wrong type with a fragment breaks any
further correct use of the the same fragment.

> Source/WebCore/rendering/ReferencedSVGResources.cpp:123
> +	   m_elementClients.ensure(targetID, [&] {
> +	       auto client =
WTF::makeUnique<CSSSVGResourceElementClient>(m_renderer);
> +	       element->addReferencingCSSClient(*client);
> +	       return client;
> +	   });

For symmetry it might be nice to have helper for add case too (to pair with
removeClientForTarget)

> Source/WebCore/rendering/ReferencedSVGResources.cpp:133
> +// SVG code uses getRenderSVGResourceById<>, but that works in terms of
renderers. We need to find resources
> +// before the render tree is fully constructed, so this works on Elements.

It seems unfortunate to have two mechanisms for this. Like how do we even know
they produce the same results? Can we have just one?

> Source/WebCore/rendering/ReferencedSVGResources.cpp:147
> +    if (referenceFilter.fragment().isEmpty())
> +	   return nullptr;
> +    AtomString targetID = referenceFilter.fragment();

could either move the local to the beginning and test for it, or just get rid
of it entirely.

> Source/WebCore/rendering/ReferencedSVGResources.cpp:156
> +    if (clipPath.fragment().isEmpty())
> +	   return nullptr;
> +    AtomString targetID = clipPath.fragment();

Same here.

> Source/WebCore/rendering/ReferencedSVGResources.cpp:158
> +    // For some reason, SVG stores a cache of id -> renderer, rather than
just using getElementById() and renderer().
> +    return getRenderSVGResourceById<RenderSVGResourceClipper>(document,
targetID);

y tho