[webkit-reviews] review granted: [Bug 88188] Accept HTML and MathML namespaces as valid requiredExtensions : [Attachment 220931] Patch with reftests

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 220931: Patch with reftests
https://bugs.webkit.org/attachment.cgi?id=220931&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=220931&action=review


> Source/WebCore/svg/SVGSwitchElement.cpp:55
> +    // FIXME: The renderer must be updated after dynamic change of the
requiredFeatures, requiredExtensions and systemLanguage attributes
(http://wkb.ug/74749).

I don’t think it’s appropriate to use wkb.ug URLs in the WebKit source code.
It’s neat how short they are, but we don’t want to use them here for the same
rason we don’t use them in the change log.

> Source/WebCore/svg/SVGTests.cpp:32
> +#if ENABLE(MATHML)
> +#include "MathMLNames.h"
> +#endif

Conditional includes need to go in their own paragraph, rather than attempting
to sort them in with the unconditional includes.

We should add this to the coding style guide; I looked and it’s not mentioned
there!

> Source/WebCore/svg/SVGTests.cpp:133
> -    if (!m_requiredExtensions.value.isEmpty())
> -	   return false;
> +    unsigned extensionsSize = m_requiredExtensions.value.size();
> +    for (unsigned i = 0; i < extensionsSize; ++i) {
> +	   String value = m_requiredExtensions.value.at(i);
> +	   if (!hasExtension(value))
> +	       return false;
> +    }

This should be written like this:

    for (auto& extension : m_requiredExtensions.value) {
	if (!hasExtension(extension))
	    return false;
    }

Lets not do the old style for loops in new code unless there is a reason.