[webkit-reviews] review granted: [Bug 34277] MathML Support for munder, mover, and munderover : [Attachment 48389] Updated patch to latest trunk with anonymous blocks removed.

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has granted Alex Milowski
<alex at milowski.com>'s request for review:
Bug 34277: MathML Support for munder, mover, and munderover
https://bugs.webkit.org/show_bug.cgi?id=34277

Attachment 48389: Updated patch to latest trunk with anonymous blocks removed.
https://bugs.webkit.org/attachment.cgi?id=48389&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
  
> +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.