[webkit-reviews] review granted: [Bug 124128] [regression] foreign content not displayed in MathML : [Attachment 226488] Patch

[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 124128: [regression] foreign content not displayed in MathML
https://bugs.webkit.org/show_bug.cgi?id=124128

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

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


Seems OK, but might need some additional refinement to be completely correct.

> Source/WebCore/mathml/MathMLTextElement.cpp:92
> +    return (e.isMathMLElement() && e.hasTagName(MathMLNames::mathTag)) ||
(e.isSVGElement() && e.hasTagName(SVGNames::svgTag)) || (e.isHTMLElement() &&
(e.hasTagName(HTMLNames::aTag) || e.hasTagName(HTMLNames::abbrTag) ||
e.hasTagName(HTMLNames::areaTag) || e.hasTagName(HTMLNames::audioTag) ||
e.hasTagName(HTMLNames::bTag) || e.hasTagName(HTMLNames::bdiTag) ||
e.hasTagName(HTMLNames::bdoTag) || e.hasTagName(HTMLNames::brTag) ||
e.hasTagName(HTMLNames::buttonTag) || e.hasTagName(HTMLNames::canvasTag) ||
e.hasTagName(HTMLNames::citeTag) || e.hasTagName(HTMLNames::codeTag) ||
e.hasTagName(HTMLNames::datalistTag) || e.hasTagName(HTMLNames::delTag) ||
e.hasTagName(HTMLNames::dfnTag) || e.hasTagName(HTMLNames::emTag) ||
e.hasTagName(HTMLNames::embedTag) || e.hasTagName(HTMLNames::iTag) ||
e.hasTagName(HTMLNames::iframeTag) || e.hasTagName(HTMLNames::imgTag) ||
e.hasTagName(HTMLNames::inputTag) || e.hasTagName(HTMLNames::insTag) ||
e.hasTagName(HTMLNames::kbdTag) || e.hasTagName(HTMLNames::keygenTag) ||
e.hasTagName(HTMLNames::labelTag) || e.hasTagName(HTMLNames::mapTag) ||
e.hasTagName(HTMLNames::markTag) || e.hasTagName(HTMLNames::meterTag) ||
e.hasTagName(HTMLNames::noscriptTag) || e.hasTagName(HTMLNames::objectTag) ||
e.hasTagName(HTMLNames::outputTag) || e.hasTagName(HTMLNames::progressTag) ||
e.hasTagName(HTMLNames::qTag) || e.hasTagName(HTMLNames::rubyTag) ||
e.hasTagName(HTMLNames::sTag) || e.hasTagName(HTMLNames::sampTag) ||
e.hasTagName(HTMLNames::scriptTag) || e.hasTagName(HTMLNames::selectTag) ||
e.hasTagName(HTMLNames::smallTag) || e.hasTagName(HTMLNames::spanTag) ||
e.hasTagName(HTMLNames::strongTag) || e.hasTagName(HTMLNames::subTag) ||
e.hasTagName(HTMLNames::supTag) || e.hasTagName(HTMLNames::templateTag) ||
e.hasTagName(HTMLNames::textareaTag) || e.hasTagName(HTMLNames::uTag) ||
e.hasTagName(HTMLNames::varTag) || e.hasTagName(HTMLNames::videoTag) ||
e.hasTagName(HTMLNames::wbrTag)));

Writing this all out on one line is messy. I recommend separate sections for
MathML, SVG, and HTML. Also, could use hasLocalName here for better efficiency,
although that will change once my patch for bug 130090 lands. Once that is in,
we’ll want to use hasTagName, except that we’ll want to cast the element
reference to the more specific element type.

> Source/WebCore/rendering/mathml/RenderMathMLToken.cpp:65
> +    m_isTextOnly = !element().firstElementChild();

What this implements is “has no element descendants”, which is not exactly “is
text only”. It would be better to name this consistent with the implementation
unless the two are truly precisely equivalent.

I’m also concerned that we are making a decision here based on DOM tree rather
than rendering tree. What if there’s an element child that is styled "display:
none"?

> Source/WebCore/rendering/mathml/RenderMathMLToken.h:57
>      virtual void updateStyle();
> +    bool m_isTextOnly;

I would suggest a blank line between the function members and the data member.