[webkit-reviews] review denied: [Bug 88188] Accept HTML and MathML namespaces as valid requiredExtensions : [Attachment 220438] Patch V2

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Frédéric Wang <fred.wang at free.fr>'s
request for review:
Bug 88188: Accept HTML and MathML namespaces as valid requiredExtensions
https://bugs.webkit.org/show_bug.cgi?id=88188

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=220438&action=review


Patch looks good in general. r- because of a missing test for SVGDOM.

> Source/WebCore/svg/SVGSwitchElement.cpp:55
> +    // We create a renderer for the first valid SVG element child.
> +    // FIXME: The renderer must be updated after dynamic change of the
requiredFeatures, requiredExtensions and systemLanguage attributes
(http://wkbug/122297).

Yes, it looks like this is correct. The link is not valid. Even if it would, it
references something different (MathML related). Please open a bug with a test
case (if you haven't done so yet) and use the link to this bug instead.

> Source/WebCore/svg/SVGTests.cpp:102
> +bool SVGTests::hasExtension(const String& extension) const

This is used by SVGDOM as well. However, you do not have one JS test to check
the correct behavior. Could you add those?

> Source/WebCore/svg/SVGTests.cpp:104
> +    // We recognize XHTML and MathML, as implemented in Gecko and suggested
in the SVG Tiny recommendation
(http://www.w3.org/TR/SVGTiny12/struct.html#RequiredExtensionsAttribute).

We do not implement SVG 1.2 Tiny. But the normative text between SVG 1.1 and
SVG 1.2 Tiny is basically the same. Just use
http://www.w3.org/TR/SVG11/struct.html#RequiredExtensionsAttribute

> Source/WebCore/svg/SVGTests.cpp:109
> +#if ENABLE(MATHML)
> +    return extension == HTMLNames::xhtmlNamespaceURI || extension ==
MathMLNames::mathmlNamespaceURI;
> +#else
> +    return extension == HTMLNames::xhtmlNamespaceURI;
> +#endif

I wonder, even though if it would be foolish of an author to do that, if we
shouldn't check for the SVG namespace as well.

> Source/WebCore/svg/SVGTests.cpp:117
>	   if (value.isEmpty() || !DOMImplementation::hasFeature(value,
String()))

To answer your question: value can be empty. However, it is not necessary to
have the check though. It could be an optimization on the other hand. Just keep
it.

> Source/WebCore/svg/SVGTests.cpp:129
> +    for (unsigned i = 0; i < extensionsSize; ++i) {

Well, this is inconsistent. If you change the other to size_t, this should be
size_t as well. (Ditto on the other for-loops.) However, I am not sure if I
agree with Chris. We always used to use unsigned.