[webkit-reviews] review denied: [Bug 40327] Leftover calls to RenderStyle color accessors, which are no longer public methods. : [Attachment 58889] Updated patch in response to comments.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Alex Milowski <alex at milowski.com>'s
request for review:
Bug 40327: Leftover calls to RenderStyle color accessors, which are no longer
public methods.
https://bugs.webkit.org/show_bug.cgi?id=40327

Attachment 58889: Updated patch in response to comments.
https://bugs.webkit.org/attachment.cgi?id=58889&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -	   RenderBlock* container = new (renderArena()) RenderBlock(node());
> +	   RenderBlock* container = new (renderArena())
RenderMathMLBlock(node());

What does this change have to do with the bug you're fixing? This seems OK to
change, but unrelated to the change log or other changes.
>	   
> -    RenderBlock* container = new (renderArena()) RenderBlock(node());
> +    RenderBlock* container = new (renderArena()) RenderMathMLBlock(node());

Ditto.

> -    toRenderMathMLBlock(current)->style()->setVerticalAlign(BASELINE);
> +    // FIXME: should we really be setting the vertical align here?
> +    current->style()->setVerticalAlign(BASELINE);

Comment sentence should be capitalized. Also, would be better if the comment
said why it's questionable. As it is, the comment just says "should this next
line of code exist", which doesn't give context for what your doubt is.

> +    // Adjust the bottom position of the index.
>      indexBox->style()->setBottom(Length(radicalHeight +
style()->paddingBottom().value(), Fixed));

This comment says the same thing the line of code does. We normally leave those
out, and make sure comments say something the code does not say.

Patch otherwise looks fine. review- because of the seemingly unrelated changes