[webkit-reviews] review denied: [Bug 34278] MathML Support for msubsup : [Attachment 48390] 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 34278: MathML Support for msubsup
https://bugs.webkit.org/show_bug.cgi?id=34278

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

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

> +
> +    RenderObject* object = 0;

Newline at beginning of method, please remove.

> +static const int topAdjustDivisor = 3;
> +static const int subsupScriptMargin = 1;

Mark as global.

> +
> +    
> +RenderMathMLSubSup::RenderMathMLSubSup(Node *subsup) 

Two new lines are not needed. * aligned to the wrong side

> +    m_kind = SubSup;
> +    if (element->hasLocalName(MathMLNames::msubTag))
> +	   m_kind = Sub;
> +    else if (element->hasLocalName(MathMLNames::msupTag))
> +	   m_kind = Sup;
> +}

Same comment as in last patch

> +		   // FIXME: This should be in ems

Is this hard to fix?

> +void  RenderMathMLSubSup::stretchToHeight(int height)
> +{
> +    RenderObject* base = firstChild();
> +    if (!base)
> +	   return;
> +    
> +    if (base->isRenderMathMLBlock()) {
> +	   RenderMathMLBlock* block = toRenderMathMLBlock(base);
> +	   block->stretchToHeight(static_cast<int>(1.2 * height));

where does the constant come from

> +    }
> +    if (height > 0 && m_kind == SubSup && m_scripts) {
> +
> +	   RenderObject* script = m_scripts->firstChild();

No need for the newline above

> +	   if (script) {
> +	       // calculate the script height without the container margins
> +	       int topHeight = 0;
> +	       RenderObject* top = script;
> +	       RenderObject* child = top->firstChild();
> +	       if (child && child->isBoxModelObject()) {
> +		   RenderBoxModelObject* box = toRenderBoxModelObject(child);
> +		   topHeight = box->offsetHeight();
> +	       }
> +	       int bottomHeight = 0;
> +	       RenderObject* bottom = script;
> +	       child = bottom->firstChild();
> +	       if (child && child->isBoxModelObject()) {
> +		   RenderBoxModelObject* box = toRenderBoxModelObject(child);
> +		   bottomHeight = box->offsetHeight();
> +	       }

There is some duplicated code, would it make sense to use an inline method
instead?


> +
> +int RenderMathMLSubSup::nonOperatorHeight() const 
> +{
> +    return 0;
> +}

You have this code in other classes as well, would it make sense using a parent
implementation? or is that one different from 0?

> +
> +void RenderMathMLSubSup::layout() 
> +{
> +
> +    RenderBlock::layout();

Non-needed newline
    
> +    if (m_kind == SubSup) {
> +	   int width = 0;
> +	   RenderObject* current = firstChild();
> +	   while (current) {
> +	       if (current->isBoxModelObject()) {
> +
> +		   RenderBoxModelObject* box = toRenderBoxModelObject(current);


again.

> +int RenderMathMLSubSup::baselinePosition(bool , bool) const

remove space before ,
> +	       return topAdjust + (base ? base->baselinePosition(true) : 0) +
4;

What doest the true stand for here? maybe add a /* comment */ in front of it?

like baselinePosition( /* recalc */ true)...