Bug 38064 – Refactoring: RenderBox::calcHeight() could look better

• Status: NEW
• Component: WebCore Misc.

[Shinichiro Hamaji]

I think we can eliminate boolean value checkMinMaxHeight . Also, it calls borderAndPaddingHeight() twice for flexboxes but we need only once.

[Shinichiro Hamaji]

Created attachment 54201 [details]
Patch v1

[Darin Adler]

Comment on attachment 54201 [details]
Patch v1

>          if (hasOverrideSize() && parent()->isFlexibleBox() && parent()->style()->boxOrient() == VERTICAL
> -                && parent()->isFlexingChildren())
> -            h = Length(overrideSize() - borderAndPaddingHeight(), Fixed);

>          if (hasOverrideSize() && parent()->isFlexibleBox() && parent()->style()->boxOrient() == VERTICAL
> +            && parent()->isFlexingChildren()) {
> +            heightResult = overrideSize();

The extra indent here to make sure that the "&&" does not line up with the body of the if statement is something I often do intentionally. I'm not sure if our style guide has anything to say about this, but I think it's harder to read when lined up the way the new code is.

[Shinichiro Hamaji]

Created attachment 54227 [details]
Patch v2

[Shinichiro Hamaji]

I see. I've added the extra indentations for both the if condition you mentioned and "else if" condition below. Or, it would be better just to make them one line conditions? If so, I'll change them.

I didn't imagine these indentations are intentional. Actually, the style guide has an example which doesn't have the extra indentation:

if (attr->name() == srcAttr
    || attr->name() == lowsrcAttr
    || (attr->name() == usemapAttr && attr->value().domString()[0] != '#'))
    return;

I hope we'll have a rule which clarifies this and fix this example. I have no preference on this decision, but I want a consistent rule. Maybe, "we should add 4 more indentations before conditional operators when we break line in conditions" or something like this?

[Darin Adler]

Comment on attachment 54227 [details]
Patch v2

Style looks good to me, but I am not sure of the refactoring. Someone who knows the rendering code a little better can probably help with that.

[Shinichiro Hamaji]

Maybe mitz or hyatt? Could you review this, please? Though this patch looks complicated a bit, I believe this patch doesn't change the behavior.

[Adam Barth]

Comment on attachment 54227 [details]
Patch v2

This patch has been up for review for a long time and we haven't been able to interest rendering experts in reviewing it.  I'm not entirely sure what this patch is doing or whether or not it's an improvement.  I'm clearing the review flag for now to remove this from pending-review.  You should feel free to recruit a rendering expert to review your patch.