Bug 110520 – Add computeInstrinsicLogicalWidths functions to TableLayout subclasses

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

[Ojan Vafai]

Add computeInstrinsicLogicalWidths functions to TableLayout subclasses

[Ojan Vafai]

Created attachment 189611 [details]
Patch

[Ojan Vafai]

This patch depends on the cleanup in bug 110515.

[Ojan Vafai]

Created attachment 189662 [details]
Sync to ToT

[Tony Chang]

Comment on attachment 189662 [details]
Sync to ToT

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

> Source/WebCore/rendering/AutoTableLayout.cpp:251
> +    // if there was no remaining percent, maxWidth is invalid

Nit: Capitalize the first letter and end the sentence with a period.

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

Julien is right that this style is rare.  I see it mostly when setting min and max preferred width to the same value.  I don't think it's actually against the style guide and this seems fine to me.

[Ojan Vafai]

(In reply to comment #4)
> (From update of attachment 189662 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189662&action=review
> 
> > Source/WebCore/rendering/FixedTableLayout.cpp:181
> > +    minWidth = maxWidth = calcWidthArray();
> 
> Julien is right that this style is rare.  I see it mostly when setting min and max preferred width to the same value.  I don't think it's actually against the style guide and this seems fine to me.

Yup. As to whether it's more readable, it just seems like a matter of taste. I find this more clear for these functions that all have the same pattern of setting the min and max widths.

[Ojan Vafai]

Committed r143762: <http://trac.webkit.org/changeset/143762>

[Julien Chaffraix]

Comment on attachment 189662 [details]
Sync to ToT

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

>>> Source/WebCore/rendering/FixedTableLayout.cpp:181
>>> +    minWidth = maxWidth = calcWidthArray();
>> 
>> Julien is right that this style is rare.  I see it mostly when setting min and max preferred width to the same value.  I don't think it's actually against the style guide and this seems fine to me.
> 
> Yup. As to whether it's more readable, it just seems like a matter of taste. I find this more clear for these functions that all have the same pattern of setting the min and max widths.

The readability argument stems from not being consistent with the rest of the code (which is not something subjective). It's easy to browse over this code and miss that both logical widths are set up.

Also it *is* a style violation, I just couldn't find the relevant point in the coding style: see http://www.webkit.org/coding/coding-style.html, Line breaking, point 1. "Each statement should get its own line."

[Ojan Vafai]

(In reply to comment #7)
> The readability argument stems from not being consistent with the rest of the code (which is not something subjective). It's easy to browse over this code and miss that both logical widths are set up.

It *is* consistent with all the computePreferredLogicalWidth functions that do this. I feel like this pattern makes it much easier to browse this code and see that they're just being set to the same value.

> Also it *is* a style violation, I just couldn't find the relevant point in the coding style: see http://www.webkit.org/coding/coding-style.html, Line breaking, point 1. "Each statement should get its own line."

I think this case is not what this style rule is referring to, but I see how it's open to interpretation. If you want an official ruling on this, we should probably discuss this on webkit-dev.

[Julien Chaffraix]

(In reply to comment #8)
> (In reply to comment #7)
> > The readability argument stems from not being consistent with the rest of the code (which is not something subjective). It's easy to browse over this code and miss that both logical widths are set up.
> 
> It *is* consistent with all the computePreferredLogicalWidth functions that do this. I feel like this pattern makes it much easier to browse this code and see that they're just being set to the same value.

I would expect computePreferredLogicalWidth's code to be old and thus don't match our current style very well.

> > Also it *is* a style violation, I just couldn't find the relevant point in the coding style: see http://www.webkit.org/coding/coding-style.html, Line breaking, point 1. "Each statement should get its own line."
> 
> I think this case is not what this style rule is referring to, but I see how it's open to interpretation. If you want an official ruling on this, we should probably discuss this on webkit-dev.

I pointed it out on webkit-dev: https://lists.webkit.org/pipermail/webkit-dev/2013-February/023947.html