Bug 34277

Attachment 47632 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/mathml/RenderMathMLUnderOver.cpp:123: Missing spaces around != [whitespace/operators] [3] WebCore/mathml/RenderMathMLUnderOver.cpp:125: One line control clauses should not use braces. [whitespace/braces] [4] WebCore/mathml/RenderMathMLUnderOver.cpp:162: One line control clauses should not use braces. [whitespace/braces] [4] WebCore/mathml/RenderMathMLUnderOver.cpp:164: One line control clauses should not use braces. [whitespace/braces] [4] WebCore/mathml/RenderMathMLUnderOver.cpp:189: One line control clauses should not use braces. [whitespace/braces] [4] WebCore/mathml/RenderMathMLUnderOver.cpp:211: One line control clauses should not use braces. [whitespace/braces] [4] WebCore/mathml/RenderMathMLUnderOver.cpp:226: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 7 If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 47636 [details] Updated patch to fix style errors

Created attachment 47729 [details] Updated patch to include updates from the change in CSS from bug 34275

Created attachment 48389 [details] Updated patch to latest trunk with anonymous blocks removed.

Comment on attachment 48389 [details] Updated patch to latest trunk with anonymous blocks removed. > +static const double underNormalAdjust = -0.4; > +static const double fontAdjust = 0.75; Other places in WebKit we have globals prefixed with g. I think Darin Adler suggested that to me once. Also it would be nice with a comment explaining where these numbers come from. > + > +RenderMathMLUnderOver::RenderMathMLUnderOver(Node* expression) > + : RenderMathMLBlock(expression) > +{ > + Element* element = static_cast<Element*>(expression); > + // Determine what kind of under/over expression we have by element name > + m_kind = Under; > + if (element->hasLocalName(MathMLNames::moverTag)) > + m_kind = Over; > + else if (element->hasLocalName(MathMLNames::munderoverTag)) > + m_kind = UnderOver; > +} So in the above, if it is an "under" you get to call hasLocalName 2 times. So I guess you didn't add an if for 'under' as some kind of optimization? Is that call expensive? Is the 'over' tag the most common? > + > +void RenderMathMLUnderOver::addChild(RenderObject* child, RenderObject* beforeChild) > +{ > + > + RenderMathMLBlock* row = new (renderArena()) RenderMathMLBlock(node()); I have notices that you have one extra newline after {. This is different from current WebKit code. Please avoid that. > + RefPtr<RenderStyle> rowStyle = makeBlockStyle(); > + row->setStyle(rowStyle.release()); > + > + // look through the children for rendered elements > + int blocks = 0; > + RenderObject* current = this->firstChild(); > + while (current) { > + blocks++; > + current = current->nextSibling(); > + } If I didn't miss anything in the above you don't use current for anything, but to count? > +void RenderMathMLUnderOver::stretchToHeight(int height) > +{ > + > + RenderObject* base = firstChild(); > + if (!base) > + return; > + if (m_kind != Under) > + base = base->nextSibling(); > + if (!base) > + return; > + // use the child of the row which is the actual base > + base = base->firstChild(); So if it is not under you have to get the first child of the next sibling, or else the first child will do. It is not exactly obvious to me, maybe add a comment on the logic? > + > + > +void RenderMathMLUnderOver::layout() You don't need two newlines in the above > +{ > + > + RenderBlock::layout(); > + RenderObject* over = 0; > + RenderObject* base = 0; > + switch (m_kind) { > + case Over: > + // We need to calculate the baseline over the over versus the start of the base and > + // adjust the placement of the base. > + over = firstChild(); > + if (over) { > + // FIXME: descending glyphs intrude into base (e.g. lowercase y over base) > + // FIXME: bases that ascend higher than the line box intrude into the over > + int overSpacing = static_cast<int>(0.667 * (getOffsetHeight(over) - over->firstChild()->baselinePosition(true))); Shouldn't you use a descriptive constant instead of 0.667? > + > + // base row wrapper > + base = over->nextSibling(); > + if (base) { > + if (overSpacing>0) Coding style, missing spaces around > > + > +int RenderMathMLUnderOver::baselinePosition(bool , bool) const Remove that space before the , In the future I would put the tests as a separate patch, especially because of the images. Generally looks good, but please fix the above before landing.

(In reply to comment #5) > (From update of attachment 48389 [details]) > > > +static const double underNormalAdjust = -0.4; > > +static const double fontAdjust = 0.75; > > Other places in WebKit we have globals prefixed with g. I think Darin Adler > suggested that to me once. > > Also it would be nice with a comment explaining where these numbers come from. That's actually unused code. I will make a constant for the other factor. > > > + > > +RenderMathMLUnderOver::RenderMathMLUnderOver(Node* expression) > > + : RenderMathMLBlock(expression) > > +{ > > + Element* element = static_cast<Element*>(expression); > > + // Determine what kind of under/over expression we have by element name > > + m_kind = Under; > > + if (element->hasLocalName(MathMLNames::moverTag)) > > + m_kind = Over; > > + else if (element->hasLocalName(MathMLNames::munderoverTag)) > > + m_kind = UnderOver; > > +} > > So in the above, if it is an "under" you get to call hasLocalName 2 times. So I > guess you didn't add an if for 'under' as some kind of optimization? Is that > call expensive? Is the 'over' tag the most common? Nope. I've changed it to check for the localname for munder and then default to Under as a last resort. I don't really think it is any faster but it is probably easier to understand. > > + RefPtr<RenderStyle> rowStyle = makeBlockStyle(); > > + row->setStyle(rowStyle.release()); > > + > > + // look through the children for rendered elements > > + int blocks = 0; > > + RenderObject* current = this->firstChild(); > > + while (current) { > > + blocks++; > > + current = current->nextSibling(); > > + } > > If I didn't miss anything in the above you don't use current for anything, but > to count? Just counting. I don't see a clean way to get the number of children from the RenderObject API. > > > +void RenderMathMLUnderOver::stretchToHeight(int height) > > +{ > > + > > + RenderObject* base = firstChild(); > > + if (!base) > > + return; > > + if (m_kind != Under) > > + base = base->nextSibling(); > > + if (!base) > > + return; > > + // use the child of the row which is the actual base > > + base = base->firstChild(); > > So if it is not under you have to get the first child of the next sibling, or > else the first child will do. It is not exactly obvious to me, maybe add a > comment on the logic? Sure. I add a comment but the base (what things are under/over) is either the first or second child depending on what kind of element you have. > > +{ > > + > > + RenderBlock::layout(); > > + RenderObject* over = 0; > > + RenderObject* base = 0; > > + switch (m_kind) { > > + case Over: > > + // We need to calculate the baseline over the over versus the start of the base and > > + // adjust the placement of the base. > > + over = firstChild(); > > + if (over) { > > + // FIXME: descending glyphs intrude into base (e.g. lowercase y over base) > > + // FIXME: bases that ascend higher than the line box intrude into the over > > + int overSpacing = static_cast<int>(0.667 * (getOffsetHeight(over) - over->firstChild()->baselinePosition(true))); > > Shouldn't you use a descriptive constant instead of 0.667? Just a scaling factor. I'm removing two thirds of the space. I've made that a global constant. > In the future I would put the tests as a separate patch, especially because of > the images. I was told differently and so I've included the pixel tests in all my patches. > > Generally looks good, but please fix the above before landing. There is an updated patch on the way. Just testing it now.

Created attachment 49585 [details] Updated patch to address reviewer comments

Great! > I was told differently and so I've included the pixel tests in all my > patches. Fine, I don't feel strongly about it.

Comment on attachment 49585 [details] Updated patch to address reviewer comments Clearing flags on attachment: 49585 Committed r55310: <http://trac.webkit.org/changeset/55310>

All reviewed patches have been landed. Closing bug.