Bug 67976 – Remove remnants of code that assume Lengths are 28 bit integers.

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

[Luke Macpherson]

Remove remnants of code that assume Lengths are 28 bit integers.

[Luke Macpherson]

Created attachment 107119 [details]
Patch

[WebKit Review Bot]

Comment on attachment 107119 [details]
Patch

Attachment 107119 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9651043

New failing tests:
fast/table/max-width-integer-overflow.html

[Eric Seidel (no email)]

Comment on attachment 107119 [details]
Patch

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

> Source/WebCore/rendering/AutoTableLayout.cpp:271
> +        maxWidth = std::numeric_limits<LayoutUnit>::max();

Why is this LayoutUnit::max and the other is INT_MAX?  Does Length use LayoutUnit or int?

[Luke Macpherson]

Comment on attachment 107119 [details]
Patch

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

> Source/WebCore/css/CSSPrimitiveValue.cpp:333
> +    return Length(roundForImpreciseConversion<int, INT_MAX, INT_MIN>(computeLengthDouble(style, rootStyle, multiplier, computingFontSize)), Fixed);

I used INT_MAX here in order to be consistent with the existing code there that uses INT_MAX/SHRT_MAX/USHRT_MAX etc. for rounding. I'll make the computeLengthDouble macro take a single template parameter in a follow up change.

>> Source/WebCore/rendering/AutoTableLayout.cpp:271
>> +        maxWidth = std::numeric_limits<LayoutUnit>::max();
> 
> Why is this LayoutUnit::max and the other is INT_MAX?  Does Length use LayoutUnit or int?

Context: LayoutUnit is an intermediate typedef that is currently an int, and is intended to be switched to a float at some point in the future once eae is done refactoring to use floats everywhere.
As far as I can tell, using the 28 bit max here was incorrect, as mawWidth is a LayoutUnit, not a Length anyway, so it was using an irrelevant constant from a different type...
Now, to your questions:
1) I used numeric_limits<LayoutUnit>::max() here because the underlying type or LayoutUnit is intended to change in the future.
2) Length does not use LayoutUnit, but maxWidth is not a Length, it is a LayoutUnit.

[Luke Macpherson]

Comment on attachment 107119 [details]
Patch

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

>> Source/WebCore/css/CSSPrimitiveValue.cpp:333
>> +    return Length(roundForImpreciseConversion<int, INT_MAX, INT_MIN>(computeLengthDouble(style, rootStyle, multiplier, computingFontSize)), Fixed);
> 
> I used INT_MAX here in order to be consistent with the existing code there that uses INT_MAX/SHRT_MAX/USHRT_MAX etc. for rounding. I'll make the computeLengthDouble macro take a single template parameter in a follow up change.

That should have been roundForImpreciseConversion, and it's not a macro, it's a function. I think I need caffeine.

[Luke Macpherson]

Created attachment 107966 [details]
Patch

[WebKit Review Bot]

Comment on attachment 107966 [details]
Patch

Attachment 107966 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9765237

New failing tests:
fast/table/max-width-integer-overflow.html

[Eric Seidel (no email)]

Comment on attachment 107966 [details]
Patch

Looks like this breaks a test?

[Luke Macpherson]

Created attachment 132527 [details]
Patch

[WebKit Review Bot]

Comment on attachment 132527 [details]
Patch

Attachment 132527 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11976892

New failing tests:
fast/table/max-width-integer-overflow.html

[Luke Macpherson]

Created attachment 132540 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 132540 [details]
Patch

OK.

[WebKit Review Bot]

Comment on attachment 132540 [details]
Patch

Clearing flags on attachment: 132540

Committed r111156: <http://trac.webkit.org/changeset/111156>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.