[webkit-reviews] review denied: [Bug 34347] MathML Support for mrow and Stretchy Operators : [Attachment 48392] Updated patch to latest trunk with anonymous blocks removed.

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Alex Milowski
<alex at milowski.com>'s request for review:
Bug 34347: MathML Support for mrow and Stretchy Operators
https://bugs.webkit.org/show_bug.cgi?id=34347

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

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>

> +    int index = -1;
> +    if (firstChar) {
> +	   for (index++; stretchy[index][0]; index++) {
> +	       if (stretchy[index][0] == firstChar)
> +		   break;
> +	   }
> +    }
> +    
> +    bool noStretch = false;
> +    if (mo) {
> +	   AtomicString stretchyAttr =
mo->getAttribute(MathMLNames::stretchyAttr);
> +	   noStretch = equalIgnoringCase(stretchyAttr, "false");
> +    }

So you are checking if we are allowed to stretch? If so, couldn't this be above
the other if (firstChar)?

> +    // either we have no characters, the stretch height is small, or we have
a non-stretchable character
> +    // If so, then we just set the font size and wrap the text
> +    bool canStretch = index >= 0 && stretchy[index][0];

You only need the above index if you are going to stretch. Couldn't you
separate out the code in a private method, isStretchable or so?

The below if cases are huge, so it would be good to have them separated out in
methods as well.

> +    if (noStretch || !firstChar || m_stretchHeight <= 24 || !canStretch) {
> +
> +	   m_isStacked = false;
> +	   RenderBlock* container = new (renderArena()) RenderBlock(node());
> +	   
> +	   RefPtr<RenderStyle> newStyle = RenderStyle::create();
> +	   newStyle->inheritFrom(style());
> +	   newStyle->setDisplay(BLOCK);
> +
> +	   int currentFontSize = style()->fontSize();
> +	   if (!noStretch && m_stretchHeight > 0 && m_stretchHeight <= 24  &&
m_stretchHeight > currentFontSize && canStretch) {

24?

> +	       FontDescription* desc = new FontDescription();
> +	       desc->setIsAbsoluteSize(true);
> +	       desc->setSpecifiedSize(m_stretchHeight);
> +	       desc->setComputedSize(m_stretchHeight);
> +	       newStyle->setFontDescription(*desc);
> +	       newStyle->font().update(newStyle->font().fontSelector());
> +	   }
> +	   
> +	   newStyle->setVerticalAlign(BASELINE);
> +	   container->setStyle(newStyle.release());
> +	   addChild(container);
> +	
> +	   RenderText* text = new (renderArena()) RenderText(node(), m_operator
? StringImpl::create(&m_operator, 1) : StringImpl::create(mo->textContent()));
> +	   text->setStyle(container->style());
> +	   container->addChild(text);
> +    
> +    } else {
> +	   // build stretchable characters
> +	   m_isStacked = true;
> +	   
> +	   if (stretchy[index][4]) {
> +	   
> +	       // we have a middle glyph (e.g. a curly bracket)
> +	       
> +	       int half = (m_stretchHeight - 10) / 2;

You use a lot of constants in the code around here. Where do they come from? I
see 10, -2 and -4 in many places.

> +	       if (half <= 10) {
> +		   // we only have enough space for the top & bottom glyph
> +		   makeGlyph(stretchy[index][1], half);
> +		   makeGlyph(stretchy[index][4], 10, -2);
> +		   makeGlyph(stretchy[index][3], 0, -4);
> +	       } else {
> +		   // we have to extend both the top and bottom to the middle
> +		   makeGlyph(stretchy[index][1], 10);
> +		   int remaining = half-10;

Missing spaces around -

> +		   while (remaining > 0) {
> +		       if (remaining < 10) {
> +			   makeGlyph(stretchy[index][2], remaining);
> +			   remaining = 0;
> +		       } else {
> +			   makeGlyph(stretchy[index][2], 10);
> +			   remaining -= 10;
> +		       }
> +		   }
> +		   
> +		   // The middle glyph
> +		   makeGlyph(stretchy[index][4], 10, -2);
> +		   
> +		   remaining = half-10;

same thing

> +		   // make sure we have the full height in case the height is
odd
> +		   if (m_stretchHeight % 2 == 1)
> +		       remaining++;
> +		       
> +		   while (remaining > 0) {
> +		       if (remaining < 10) {
> +			   makeGlyph(stretchy[index][2], remaining);
> +			   remaining = 0;
> +		       } else {
> +			   makeGlyph(stretchy[index][2], 10);
> +			   remaining -= 10;
> +		       }
> +		   }
> +		   makeGlyph(stretchy[index][3], 0, -4);

For these indices 2, 3 etc, would it make sense using an enum?

> +	       }
> +	   } else {
> +	   
> +	       // we do not have a middle glyph
> +	       

Excessive newlining above

> +	       int remaining = m_stretchHeight - 20;
> +	       makeGlyph(stretchy[index][1], 10);
> +	       while (remaining > 0) {
> +		   if (remaining < 10) {
> +		       makeGlyph(stretchy[index][2], remaining);
> +		       remaining = 0;
> +		   } else {
> +		       makeGlyph(stretchy[index][2], 10);
> +		       remaining -= 10;
> +		   }
> +	       }
> +	       makeGlyph(stretchy[index][3], 0, -4);
> +	   }
> +    }
> +	  
> +}
> +
> +RefPtr<RenderStyle> RenderMathMLOperator::makeStackableStyle(int size, int
topRelative)
> +{
> +    RefPtr<RenderStyle> newStyle = RenderStyle::create();
> +    newStyle->inheritFrom(style());
> +    newStyle->setDisplay(BLOCK);
> +
> +    // 14px is the size we'll use for all characters

ok, why?

> +    FontDescription* desc = new FontDescription();
> +    desc->setIsAbsoluteSize(true);
> +    desc->setSpecifiedSize(14);
> +    desc->setComputedSize(14);
> +    newStyle->setFontDescription(*desc);
> +    newStyle->font().update(newStyle->font().fontSelector());
> +    newStyle->setLineHeight(Length(12, Fixed));

and 12?


> +    virtual void layout();
> +    virtual RefPtr<RenderStyle> makeStackableStyle(int size, int
topRelative);

create* is more common for RefPtr in WebKit, I guess.


> +	   } else if (current->isBoxModelObject()) {
> +
> +	       RenderBoxModelObject* box = toRenderBoxModelObject(current);

excessive newlining

> +
> +	       // Check to see if this box has a larger height
> +	       if (box->offsetHeight() > maxHeight)
> +		   maxHeight = box->offsetHeight();
> +		   
> +	   }

Why a newline above?

> +	   
> +    }
> +    return maxHeight;
> +}
> +
> +
> +
> +void RenderMathMLRow::layout() 

3 newlines? :-)


> +	       if (!block->hasBase() && !block->isRenderMathMLOperator()) {
> +
> +		   // Check to see if this box has a larger height

Newline

> +		   if (block->offsetHeight() > maxHeight)
> +		       maxHeight = block->offsetHeight();
> +	       }
> +	       if (block->isRenderMathMLOperator())
> +		   if (block->offsetHeight() > operatorHeight)
> +		       operatorHeight = block->offsetHeight();
> +	   } else if (current->isBoxModelObject()) {
> +
> +	       RenderBoxModelObject* box = toRenderBoxModelObject(current);
> +
> +	       // Check to see if this box has a larger height

enwlines