Bug 84801

Summary:

Move Length and CSS length computation to float

Product:

WebKit

Reporter:

Emil A Eklund <eae>

Component:

CSS

Assignee:

Emil A Eklund <eae>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, eric, hyatt, leviw, macpherson, menard, mjs, webkit.review.bot

Priority:

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

Emil A Eklund

Reported 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.

Attachments
Patch (330.66 KB, patch) 2012-04-26 17:45 PDT, Emil A Eklund	no flags	Details Formatted Diff Diff
Patch (290.83 KB, patch) 2012-04-26 18:24 PDT, Emil A Eklund	no flags	Details Formatted Diff Diff
Patch (290.86 KB, patch) 2012-04-26 22:27 PDT, Emil A Eklund	no flags	Details Formatted Diff Diff
Patch (290.86 KB, patch) 2012-04-26 22:28 PDT, Emil A Eklund	no flags	Details Formatted Diff Diff
Patch (291.49 KB, patch) 2012-04-27 09:01 PDT, Emil A Eklund	no flags	Details Formatted Diff Diff
Patch (291.49 KB, patch) 2012-04-27 09:19 PDT, Emil A Eklund	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-01 (6.18 MB, application/zip) 2012-04-27 23:02 PDT, WebKit Review Bot	no flags	Details
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Emil A Eklund

Comment 1 2012-04-26 17:45:03 PDT

Created attachment 139106 [details] Patch

Emil A Eklund

Comment 2 2012-04-26 18:24:59 PDT

Created attachment 139116 [details] Patch

Emil A Eklund

Comment 3 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.

Luke Macpherson

Comment 4 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.

Emil A Eklund

Comment 5 2012-04-26 22:27:39 PDT

Created attachment 139131 [details] Patch Thanks, made the changes as suggested.

Emil A Eklund

Comment 6 2012-04-26 22:28:41 PDT

Created attachment 139132 [details] Patch Replaced else if statement with if after return.

Eric Seidel (no email)

Comment 7 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?

Levi Weintraub

Comment 8 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 :)

Emil A Eklund

Comment 9 2012-04-27 09:01:13 PDT

Created attachment 139212 [details] Patch

Emil A Eklund

Comment 10 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.

Early Warning System Bot

Comment 11 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

Early Warning System Bot

Comment 12 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

Emil A Eklund

Comment 13 2012-04-27 09:19:08 PDT

Created attachment 139217 [details] Patch

Eric Seidel (no email)

Comment 14 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.

Emil A Eklund

Comment 15 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?

Emil A Eklund

Comment 16 2012-04-27 10:31:57 PDT

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

Eric Seidel (no email)

Comment 17 2012-04-27 11:50:41 PDT

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

WebKit Review Bot

Comment 18 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

WebKit Review Bot

Comment 19 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

Emil A Eklund

Comment 20 2012-04-28 09:26:32 PDT

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

Note You need to log in before you can comment on or make changes to this bug.