Bug 84801

Summary: Move Length and CSS length computation to float
Product: WebKit Reporter: Emil A Eklund <eae>
Component: CSSAssignee: Emil A Eklund <eae>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eric, hyatt, leviw, macpherson, menard, mjs, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 60318    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-01 none

Description Emil A Eklund 2012-04-24 16:42:11 PDT
Move length and CSS length computation, specifically CSSPrimitiveValue::computeLength, to floats. This will allow subpixel precision for lengths once we enable subpixel layout.
Comment 1 Emil A Eklund 2012-04-26 17:45:03 PDT
Created attachment 139106 [details]
Patch
Comment 2 Emil A Eklund 2012-04-26 18:24:59 PDT
Created attachment 139116 [details]
Patch
Comment 3 Emil A Eklund 2012-04-26 18:31:55 PDT
The patch includes new test expectations for mac-snowleopard, I intend to grab the updated expectations from the bots post landing.
Comment 4 Luke Macpherson 2012-04-26 19:57:30 PDT
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.
Comment 5 Emil A Eklund 2012-04-26 22:27:39 PDT
Created attachment 139131 [details]
Patch

Thanks, made the changes as suggested.
Comment 6 Emil A Eklund 2012-04-26 22:28:41 PDT
Created attachment 139132 [details]
Patch

Replaced else if statement with if after return.
Comment 7 Eric Seidel (no email) 2012-04-26 23:24:23 PDT
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?
Comment 8 Levi Weintraub 2012-04-27 00:53:00 PDT
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 :)
Comment 9 Emil A Eklund 2012-04-27 09:01:13 PDT
Created attachment 139212 [details]
Patch
Comment 10 Emil A Eklund 2012-04-27 09:04:05 PDT
(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.
Comment 11 Early Warning System Bot 2012-04-27 09:14:21 PDT
Comment on attachment 139212 [details]
Patch

Attachment 139212 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12549238
Comment 12 Early Warning System Bot 2012-04-27 09:18:33 PDT
Comment on attachment 139212 [details]
Patch

Attachment 139212 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12567079
Comment 13 Emil A Eklund 2012-04-27 09:19:08 PDT
Created attachment 139217 [details]
Patch
Comment 14 Eric Seidel (no email) 2012-04-27 10:13:32 PDT
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.
Comment 15 Emil A Eklund 2012-04-27 10:21:37 PDT
(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?
Comment 16 Emil A Eklund 2012-04-27 10:31:57 PDT
I plan to commit this after hours and will monitor perf bots and rollout if needed.
Comment 17 Eric Seidel (no email) 2012-04-27 11:50:41 PDT
Those are the only perf bots I know of. :)  (At least only public ones.)
Comment 18 WebKit Review Bot 2012-04-27 23:02:36 PDT
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
Comment 19 WebKit Review Bot 2012-04-27 23:02:42 PDT
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
Comment 20 Emil A Eklund 2012-04-28 09:26:32 PDT
Committed r115573: <http://trac.webkit.org/changeset/115573>