[webkit-reviews] review denied: [Bug 15799] textPath element does not re-render when referenced path changes : [Attachment 99588] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Rob Buis
<rwlbuis at gmail.com>'s request for review:
Bug 15799: textPath element does not re-render when referenced path changes
https://bugs.webkit.org/show_bug.cgi?id=15799

Attachment 99588: Patch
https://bugs.webkit.org/attachment.cgi?id=99588&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=99588&action=review

Good job! Some comments: In general patch doesn't apply so you'll need to
reupload it, so we get EWS results.

> LayoutTests/ChangeLog:6
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Bug 15799: textPath element does not re-render when referenced path
changes
> +	   https://bugs.webkit.org/show_bug.cgi?id=15799

According to a recent thread on webkit-dev, these blocks need to be swapped.
Bug block comes first.

> Source/WebCore/ChangeLog:5
> +	   Need a short description and bug URL (OOPS!)

Should be removed.

> Source/WebCore/ChangeLog:8
> +	   Bug 15799: textPath element does not re-render when referenced path
changes
> +	   https://bugs.webkit.org/show_bug.cgi?id=15799

Swap blocks.

> Source/WebCore/svg/SVGElementRareData.h:24
> +#include "Element.h"
> +#include "EventNames.h"

Can we move the implementation of
createAndInstallEventListener/removeEventListenerIfNeeded to the .cpp file, to
avoid having to include EventNames everywhere (this adds a huge dependency for
all SVG*Element files)

> Source/WebCore/svg/SVGElementRareData.h:25
> +#include "SubtreeModificationEventListener.h"

Isn't a class forward enough here?

> Source/WebCore/svg/SVGPathElement.cpp:252
> +	   if (Node* parent = parentNode()) {
> +	       RefPtr<MutationEvent> mutationEvent =
MutationEvent::create(eventNames().DOMSubtreeModifiedEvent, true);
> +	       parent->handleLocalEvents(mutationEvent.get());
> +	   }

Why does only SVGPathElement need this logic? This will immediately come to my
mind when reading this. So you have to explain it with a comment, also it's not
quite clear for me yet.

> Source/WebCore/svg/SVGStyledElement.cpp:353
> +	   if (Node* parent = parentNode()) {
> +	       RefPtr<MutationEvent> mutationEvent =
MutationEvent::create(eventNames().DOMSubtreeModifiedEvent, true);
> +	       parent->handleLocalEvents(mutationEvent.get());
> +	   }

Why do you need to fire mutation events manually anyway? I guess when the
change is triggered from SVG DOM?
When it comes from XML DOM via Element::attributeChanged, mutation events
should have been dispatched already, no?

> Source/WebCore/svg/SVGTRefElement.cpp:95
> +	   ensureRareSVGData()->removeEventListenerIfNeeded(treeScope());

Only call removeEventListener at all, when hasRareSVGData is true.

> Source/WebCore/svg/SVGTRefElement.cpp:186
> +    ensureRareSVGData()->removeEventListenerIfNeeded(treeScope());

See comments below, check hasRareSVGData first.

> Source/WebCore/svg/SVGTextPathElement.cpp:35
> +

One \n too much.

> Source/WebCore/svg/SVGTextPathElement.cpp:114
> +	   ensureRareSVGData()->removeEventListenerIfNeeded(treeScope());

Check hasRareSVGData first, to see whether you need to call a method or not.

> Source/WebCore/svg/SVGTextPathElement.cpp:120
> +	   Element* target = treeScope()->getElementById(id);
> +	   if (!target)
> +	       document()->accessSVGExtensions()->addPendingResource(id, this);

> +	   else 
> +	       ensureRareSVGData()->createAndInstallEventListener(this, id,
target);

That could be made more readable:
if (Element* target = treeScope...)
     ensureRareSVGData()->createAndInstall..
else
    document()...

> Source/WebCore/svg/SVGTextPathElement.cpp:224
> +    ensureRareSVGData()->removeEventListenerIfNeeded(treeScope());

Why ensureRareSVGData here? If hasRareSVGData is false, you don't need to do
anything. Especially not forcing to create a rare svg data object, if not
present.