[webkit-reviews] review granted: [Bug 124572] Map the dir attribute to the CSS direction property : [Attachment 217283] Patch V1

[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 124572: Map the dir attribute to the CSS direction property
https://bugs.webkit.org/show_bug.cgi?id=124572

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

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


This is OK, but I see room for improvement, especially on test coverage.

> Source/WebCore/mathml/MathMLElement.cpp:81
> +    if (name == mathbackgroundAttr || name == mathsizeAttr || name ==
mathcolorAttr || name == fontsizeAttr || name == backgroundAttr || name ==
colorAttr || name == fontstyleAttr || name == fontweightAttr || name ==
fontfamilyAttr || name == dirAttr)

I would be more comfortable with this if statement if the attributes were in
some obvious order, say alphabetical. Also would be nice if there was a test
that would fail if each of these was removed. I am betting that I could remove
one of these 10 without any test failing.

> Source/WebCore/mathml/MathMLElement.cpp:110
> +	   if (hasTagName(mathTag) || hasTagName(mstyleTag) ||
hasTagName(mrowTag) || hasTagName(mtextTag) || hasTagName(msTag) ||
hasTagName(moTag) || hasTagName(miTag) || hasTagName(mnTag))

I would be more comfortable with this if statement if the tag names were in
some obvious order, say alphabetical. Also would be nice if there was a test
that would fail if each of these was removed. I am betting that I could remove
one of these 8 without any test failing.

Also, this might be a good candidate for a named function (can be inline so the
code generated is the same), since it’s not obvious to me what these elements
have in common. Think of the function name as a good way to comment what this
set of tags represents.

> Source/WebCore/mathml/mathtags.in:11
> +mstyle interfaceName=MathMLElement

I’d like to see test coverage for this. We have tests that check if elements
get the right kind of wrapper, and we should cover this tag in that kind of
test.