Bug 84801 – Move Length and CSS length computation to float

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Emil A Eklund]

Move length and CSS length computation, specifically CSSPrimitiveValue::computeLength, to floats. This will allow subpixel precision for lengths once we enable subpixel layout.

[Emil A Eklund]

Created attachment 139106 [details]
Patch

[Emil A Eklund]

Created attachment 139116 [details]
Patch

[Emil A Eklund]

The patch includes new test expectations for mac-snowleopard, I intend to grab the updated expectations from the bots post landing.

[Luke Macpherson]

Comment on attachment 139116 [details]
Patch

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

> Source/WebCore/css/CSSPrimitiveValue.h:59
> +        value = ceiledValue;

early return here.

> Source/WebCore/css/CSSPrimitiveValue.h:60
> +    else if (proximityToNextInt >= 0.999 && value < 0)

and early return here.

[Emil A Eklund]

Created attachment 139131 [details]
Patch

Thanks, made the changes as suggested.

[Emil A Eklund]

Created attachment 139132 [details]
Patch

Replaced else if statement with if after return.

[Eric Seidel (no email)]

Comment on attachment 139132 [details]
Patch

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

LGTM, but I'd like to see your updated copy per my comments first.

> Source/WebCore/css/CSSPrimitiveValue.cpp:436
> +#if ENABLE(SUBPIXEL_LAYOUT)
> +    return Length(static_cast<float>(computeLengthDouble(style, rootStyle, multiplier, computingFontSize)), Fixed);
> +#else

Did you mean to add this?

> Source/WebCore/css/CSSPrimitiveValue.h:55
> +template<> inline float roundForImpreciseConversion(double value)
> +{

Could you explain better your re-write in the ChangeLOg?

[Levi Weintraub]

Comment on attachment 139132 [details]
Patch

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

>> Source/WebCore/css/CSSPrimitiveValue.cpp:436
>> +#else
> 
> Did you mean to add this?

This belongs in the *next* patch, but not this one. We plan on landing this flag, but it should all come together :)

[Emil A Eklund]

Created attachment 139212 [details]
Patch

[Emil A Eklund]

(In reply to comment #7)
> (From update of attachment 139132 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139132&action=review
> 
> LGTM, but I'd like to see your updated copy per my comments first.
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:436
> > +#if ENABLE(SUBPIXEL_LAYOUT)
> > +    return Length(static_cast<float>(computeLengthDouble(style, rootStyle, multiplier, computingFontSize)), Fixed);
> > +#else
> 
> Did you mean to add this?

Removed this for now, will be in the next patch instead together with the definition of the flag.

> 
> > Source/WebCore/css/CSSPrimitiveValue.h:55
> > +template<> inline float roundForImpreciseConversion(double value)
> > +{
> 
> Could you explain better your re-write in the ChangeLOg?

Certainly, expanded on why we are doing this in the ChangeLog entry.

[Early Warning System Bot]

Comment on attachment 139212 [details]
Patch

Attachment 139212 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12549238

[Early Warning System Bot]

Comment on attachment 139212 [details]
Patch

Attachment 139212 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12567079

[Emil A Eklund]

Created attachment 139217 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 139217 [details]
Patch

I'm interested to know what the perf bots think of this change.  Please make sure you check them after this is landed to make sure we haven't regressed, or if we have please explain what and why in this bug.

[Emil A Eklund]

(In reply to comment #14)
> (From update of attachment 139217 [details])
> I'm interested to know what the perf bots think of this change.  Please make sure you check them after this is landed to make sure we haven't regressed, or if we have please explain what and why in this bug.

I assume you are talking about the chromium perf bots?

[Emil A Eklund]

I plan to commit this after hours and will monitor perf bots and rollout if needed.

[Eric Seidel (no email)]

Those are the only perf bots I know of. :)  (At least only public ones.)

[WebKit Review Bot]

Comment on attachment 139217 [details]
Patch

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

New failing tests:
fast/html/details-position.html
svg/zoom/page/zoom-replaced-intrinsic-ratio-001.htm

[WebKit Review Bot]

Created attachment 139340 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Emil A Eklund]

Committed r115573: <http://trac.webkit.org/changeset/115573>