[webkit-reviews] review denied: [Bug 100626] Full support for semantics element : [Attachment 219758] Patch V3

[bugzilla-daemon at webkit.org]

chris fleizach <cfleizach at apple.com> has denied Frédéric Wang
<fred.wang at free.fr>'s request for review:
Bug 100626: Full support for semantics element
https://bugs.webkit.org/show_bug.cgi?id=100626

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

------- Additional Comments from chris fleizach <cfleizach at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=219758&action=review


> LayoutTests/mathml/presentation/semantics-3.html:24
> +  <p>annotation-xml 7: <math><semantics><csymbol>Content
MathML</csymbol><annotation-xml
encoding="unknown"><mtext>error</mtext></annotation-xml><annotation-xml
encoding="application/mathml-presentation+xml"><mtext>annotation-xml</mtext></a
nnotation-xml><annotation>error</annotation></semantics></math></p>

can you add a case for "application/mathml+xml" since you explicitly called
that out as something we should not support

> Source/WebCore/mathml/MathMLElement.h:57
> +	   return hasTagName(MathMLNames::mtrTag) ||
hasTagName(MathMLNames::mtdTag) || hasTagName(MathMLNames::maligngroupTag) ||
hasTagName(MathMLNames::malignmarkTag) || hasTagName(MathMLNames::mencloseTag)
|| hasTagName(MathMLNames::mglyphTag) || hasTagName(MathMLNames::mlabeledtrTag)
|| hasTagName(MathMLNames::mlongdivTag) || hasTagName(MathMLNames::mpaddedTag)
|| hasTagName(MathMLNames::msTag) || hasTagName(MathMLNames::mscarriesTag) ||
hasTagName(MathMLNames::mscarryTag) || hasTagName(MathMLNames::msgroupTag) ||
hasTagName(MathMLNames::mslineTag) || hasTagName(MathMLNames::msrowTag) ||
hasTagName(MathMLNames::mstackTag);

This might be more efficient, if called frequently, if it's a lazily
constructed static hash.

My personal opinion is that it might be cleaner to have these implementations
in the cpp file

> Source/WebCore/mathml/MathMLSelectElement.cpp:117
>	   } else {

You should have a comment describing what the "else" case handles.

Even better you might break this if (mactionTag) / else into two methods, so

if (mactionTag)
   updateSelectedActionChild()
else
   updateSelectedSemanticChild();

> Source/WebCore/mathml/MathMLSelectElement.cpp:120
> +	       if (child) {

it looks like you already checked if this was 0 before so this is redundant

> Source/WebCore/mathml/MathMLSelectElement.cpp:122
> +		   bool firstChildIsAnnotation = child->isMathMLElement() &&
toMathMLElement(child)->isSemanticAnnotation();

should you output something for the inspector to display if this is invalid?

> Source/WebCore/mathml/MathMLSelectElement.cpp:128
> +			   if (!child->isMathMLElement() &&
toMathMLElement(child)->isPresentationMathML())

it seems like you're duplicating the checks for the first child. Can you roll
the initial checks into the for loop?

> Source/WebCore/mathml/MathMLSelectElement.cpp:132
> +			       // If the <annotation> element has an src
attribute we ignore it.

why do you ignore it if it has a src attribute? (can you comment explain why
here instead of what)

> Source/WebCore/mathml/MathMLSelectElement.cpp:141
> +			       // If the <annotation-xml> element has an src
attribute we ignore it.

ditto for comment

> Source/WebCore/mathml/MathMLSelectElement.cpp:149
> +			       // - Other mime Content-Types for SVG and HTML

these sentences should end in a period.
I believe mime should be MIME

> Source/WebCore/mathml/MathMLSelectElement.cpp:152
> +			       String value =
child->getAttribute(MathMLNames::encodingAttr);

this should be getFastAttribute, which I believe returns an AtomicString