Bug 110515 – Clean up computePreferredLogicalWidths functions in TableLayout subclasses

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ojan Vafai]

Clean up computePreferredLogicalWidths functions in TableLayout subclasses

[Ojan Vafai]

Created attachment 189606 [details]
Patch

[Tony Chang]

Does this work with bug 109462?  Maybe Julien should review this patch :)

[Ojan Vafai]

(In reply to comment #2)
> Does this work with bug 109462?  Maybe Julien should review this patch :)

As best I can tell, the two patches are totally orthogonal.

[Ojan Vafai]

Also, this patch does not cause any change in behavior. It's just moving code around.

[Tony Chang]

Comment on attachment 189606 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189606&action=review

> Source/WebCore/rendering/AutoTableLayout.cpp:261
> -    Length tableLogicalWidth = m_table->style()->logicalWidth();
> -    if (tableLogicalWidth.isFixed() && tableLogicalWidth.isPositive()) {
> -        minWidth = max<int>(minWidth, tableLogicalWidth.value());
> -        maxWidth = minWidth;
> -    } else if (!remainingPercent && maxNonPercent) {
> -        // if there was no remaining percent, maxWidth is invalid
> +    // if there was no remaining percent, maxWidth is invalid
> +    if (!remainingPercent && maxNonPercent)
>          maxWidth = tableMaxWidth;
> -    }
> +
> +    Length tableLogicalWidth = m_table->style()->logicalWidth();
> +    if (tableLogicalWidth.isFixed() && tableLogicalWidth.isPositive())
> +        minWidth = maxWidth = max<int>(minWidth, tableLogicalWidth.value());

I would revert this section of the patch.  It doesn't seem equally readable and it's slower.

> Source/WebCore/rendering/FixedTableLayout.cpp:82
> +    // FIXME: we might want to wait until we have all of the first row before computing for the first time.

Nit: Capitalize the first letter of the sentence.

[Ojan Vafai]

Committed r143683: <http://trac.webkit.org/changeset/143683>

[Julien Chaffraix]

Comment on attachment 189606 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189606&action=review

The change is good, sorry for the delay.

(In reply to comment #3)
> (In reply to comment #2)
> > Does this work with bug 109462?  Maybe Julien should review this patch :)
> 
> As best I can tell, the two patches are totally orthogonal.

They are indeed. However you make the assumption that r130774 which cause bug 109462 is there to cover your back and properly apply min-width / max-width to the result of computePreferredLogicalWidths thus making it difficult to revert the change.

> Source/WebCore/rendering/FixedTableLayout.cpp:182
> +    minWidth = maxWidth = calcWidthArray() + bordersPaddingAndSpacing;

WebKit style is one assignment per line (though I couldn't find an entry in our coding style). That would make this line a lot more readable IMO.

> Source/WebCore/rendering/FixedTableLayout.cpp:186
> +        minWidth = maxWidth = max<int>(minWidth, tableLogicalWidth.value() - bordersPaddingAndSpacing);

Ditto.

[Ojan Vafai]

(In reply to comment #7)
> > Source/WebCore/rendering/FixedTableLayout.cpp:182
> > +    minWidth = maxWidth = calcWidthArray() + bordersPaddingAndSpacing;
> 
> WebKit style is one assignment per line (though I couldn't find an entry in our coding style). That would make this line a lot more readable IMO.

Discussed this on bug 110520.