[webkit-reviews] review granted: [Bug 94469] enable <animate> of viewBox attribute in <view> element : [Attachment 378072] Patch

[bugzilla-daemon at webkit.org]

Said Abou-Hallawa <sabouhallawa at apple.com> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 94469: enable <animate> of viewBox attribute in <view> element
https://bugs.webkit.org/show_bug.cgi?id=94469

Attachment 378072: Patch

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




--- Comment #17 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 378072
  --> https://bugs.webkit.org/attachment.cgi?id=378072
Patch

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

> Source/WebCore/ChangeLog:12
> +	   The settings from the SVGViewElement are only pasrsed onco in
SVGSVGElement::scrollToFragment(). Dynamic updates

onco -> once

> Source/WebCore/ChangeLog:14
> +	   the SVGSVGElement does not re-evaluates its viewBox. Fix that by
introducing a link between SVGSVGElement

does not re-evaluates -> does not re-evaluate

> LayoutTests/ChangeLog:22
> +	   Verify that dynamic updates to the <view> element in the external
SVG, are reflect in the SVGViewSpec API as well (SVGSVGElement::currentView).

This test is great but I think it deserves a separate patch because most of it
is not related to this patch and can pass without it.

> LayoutTests/svg/dom/SVGViewSpec-multiple-views.html:124
> +   
iframeElement.contentDocument.documentElement.getElementById("view2").setAttrib
ute("viewBox", "10 10 30 30");
> +
> +    debug("");
> +    debug("Check viewBox value after modification");
> +    shouldBe("currentView.viewBox.baseVal.x", "10");
> +    shouldBe("currentView.viewBox.baseVal.y", "10");
> +    shouldBe("currentView.viewBox.baseVal.width", "30");
> +    shouldBe("currentView.viewBox.baseVal.height", "30");
> +    shouldBeEqualToString("currentView.viewBoxString", "10 10 30 30");

This section is the only one that calls setAttribute("viewBox"). So I think the
rest of this test can pass without this patch. I guess this is because of the
missing SVGViewElement::svgAttributeChanged().

So I would suggest submitting this test in a separate patch before this one
after removing this section. Then add this section the test in this patch or
make a new test for setAttribute("viewBox").