[webkit-reviews] review granted: [Bug 110515] Clean up computePreferredLogicalWidths functions in TableLayout subclasses : [Attachment 189606] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 110515: Clean up computePreferredLogicalWidths functions in TableLayout
subclasses
https://bugs.webkit.org/show_bug.cgi?id=110515

Attachment 189606: Patch
https://bugs.webkit.org/attachment.cgi?id=189606&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
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.