Summary: | Remove remnants of code that assume Lengths are 28 bit integers. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Luke Macpherson <macpherson> | ||||||||||
Component: | New Bugs | Assignee: | Luke Macpherson <macpherson> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | darin, dglazkov, eae, eric, leviw, macpherson, menard, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 81509 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Luke Macpherson
2011-09-12 17:57:23 PDT
Created attachment 107119 [details]
Patch
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 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? 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. 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. Created attachment 107966 [details]
Patch
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 Comment on attachment 107966 [details]
Patch
Looks like this breaks a test?
Created attachment 132527 [details]
Patch
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 Created attachment 132540 [details]
Patch
Comment on attachment 132540 [details]
Patch
OK.
Comment on attachment 132540 [details] Patch Clearing flags on attachment: 132540 Committed r111156: <http://trac.webkit.org/changeset/111156> All reviewed patches have been landed. Closing bug. |