Bug 38064 - Refactoring: RenderBox::calcHeight() could look better
Summary: Refactoring: RenderBox::calcHeight() could look better
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-23 16:29 PDT by Shinichiro Hamaji
Modified: 2010-06-20 10:46 PDT (History)
4 users (show)

See Also:


Attachments
Patch v1 (3.52 KB, patch)
2010-04-23 16:30 PDT, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
Patch v2 (3.53 KB, patch)
2010-04-24 19:24 PDT, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinichiro Hamaji 2010-04-23 16:29:29 PDT
I think we can eliminate boolean value checkMinMaxHeight . Also, it calls borderAndPaddingHeight() twice for flexboxes but we need only once.
Comment 1 Shinichiro Hamaji 2010-04-23 16:30:26 PDT
Created attachment 54201 [details]
Patch v1
Comment 2 Darin Adler 2010-04-24 09:27:06 PDT
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.
Comment 3 Shinichiro Hamaji 2010-04-24 19:24:42 PDT
Created attachment 54227 [details]
Patch v2
Comment 4 Shinichiro Hamaji 2010-04-24 19:36:13 PDT
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?
Comment 5 Darin Adler 2010-04-25 22:01:11 PDT
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.
Comment 6 Shinichiro Hamaji 2010-04-25 22:26:51 PDT
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.
Comment 7 Adam Barth 2010-06-20 10:46:47 PDT
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.