New issue
Advanced search Search tips
Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2014
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment
link

Issue 322365: Clean up CSS to Length conversions

Reported by timloh@chromium.org, Nov 22 2013 Project Member

Issue description

Tracking bug for patches cleaning up conversions from CSSPrimitiveValue to Length.

Previous patches include https://codereview.chromium.org/71253002 and https://codereview.chromium.org/68203023
 

Comment 2 by bugdroid1@chromium.org, Nov 29 2013

Project Member
The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=162881

------------------------------------------------------------------------
r162881 | timloh@chromium.org | 2013-11-29T09:57:19.937293Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/AnimatedStyleBuilder.cpp?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSMatrix.cpp?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSPrimitiveValue.cpp?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/FontBuilder.cpp?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSToStyleMap.h?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSPrimitiveValueMappings.h?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/ViewportStyleResolver.cpp?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/TransformBuilder.cpp?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSPrimitiveValue.h?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/StyleResolverState.cpp?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/animation/AnimatableLength.cpp?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/animation/AnimatableLengthTest.cpp?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/core.gypi?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/MediaQueryEvaluator.cpp?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/TransformBuilder.h?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/StyleResolverState.h?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/FilterOperationResolver.cpp?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/animation/AnimatableLength.h?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/BasicShapeFunctions.cpp?r1=162881&r2=162880&pathrev=162881
   A http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSToLengthConversionData.cpp?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/FilterOperationResolver.h?r1=162881&r2=162880&pathrev=162881
   A http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSToLengthConversionData.h?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSGradientValue.cpp?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/StyleBuilderCustom.cpp?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSCalculationValueTest.cpp?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSCalculationValue.cpp?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSGradientValue.h?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/StyleBuilderCustom.h?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSCalculationValue.h?r1=162881&r2=162880&pathrev=162881
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSToStyleMap.cpp?r1=162881&r2=162880&pathrev=162881

Wrap CSS length conversion arguments in an object

This patch introduces a class CSSToLengthConversionData to wrap the data
required to convert CSS lengths to Lengths. This simplifies the plumbing
that goes on whenever we need to resolve CSS lengths and makes it easier
to update the arguments needed for resolving these (in particular adding
a RenderView for resolving viewport units at style recalc time; removing
the computingFontSize bool also appears possible).

Note that the zoom argument, which was previously a float in some places
and a double in others is now a float.

BUG= 322365 

Review URL: https://codereview.chromium.org/64293008
------------------------------------------------------------------------

Comment 3 by bugdroid1@chromium.org, Dec 2 2013

Project Member
The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=162958

------------------------------------------------------------------------
r162958 | timloh@chromium.org | 2013-12-02T05:43:45.493002Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/CSSToStyleMap.cpp?r1=162958&r2=162957&pathrev=162958

Call convertToLength in CSSToStyleMap instead of having it inlined

Trivial cleanup; reduce copy-paste in CSSToStyleMap. Note that the
case where the primitive value isn't an appropriate type shouldn't
occur since we generally assume the parser only allows valid types
through so we change it to an assertion here.

BUG= 322365 

Review URL: https://codereview.chromium.org/98193002
------------------------------------------------------------------------

Comment 4 by bugdroid1@chromium.org, Dec 18 2013

Project Member
The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=164041

------------------------------------------------------------------------
r164041 | timloh@chromium.org | 2013-12-18T00:38:57.585309Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/StyleBuilderConverter.cpp?r1=164041&r2=164040&pathrev=164041
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSPrimitiveValueMappings.h?r1=164041&r2=164040&pathrev=164041
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/StyleBuilderCustom.cpp?r1=164041&r2=164040&pathrev=164041
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/TransformBuilder.cpp?r1=164041&r2=164040&pathrev=164041
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSCalculationValue.cpp?r1=164041&r2=164040&pathrev=164041
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/FilterOperationResolver.cpp?r1=164041&r2=164040&pathrev=164041
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/CSSToStyleMap.cpp?r1=164041&r2=164040&pathrev=164041
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/style/BasicShapes.h?r1=164041&r2=164040&pathrev=164041

Web Animations: Assert that convertToLength succeeds instead of returning Length(Undefined)

CSSPrimitiveValue::convertToLength should always match one of the cases
for conversion, so instead of returning Length(Undefined) if we somehow
fail and asserting in the callers we aren't undefined, simply assert we
don't fail.

BUG= 322365 

Review URL: https://codereview.chromium.org/108653006
------------------------------------------------------------------------

Comment 5 by bugdroid1@chromium.org, Jan 3 2014

Project Member
The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=164448

------------------------------------------------------------------------
r164448 | timloh@chromium.org | 2014-01-03T08:28:33.534806Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RootInlineBox.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/masking/parsing-clip-path-shape-expected.txt?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSPrimitiveValue.h?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/StyleResolverState.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/virtual/legacy-animations-engine/animations/interpolation/line-height-interpolation-expected.txt?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderMenuList.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/animation/AnimatableLengthTest.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/StyleResolver.h?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/style/RenderStyle.h?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/platform/Length.h?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderView.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/css/computed-offset-with-zoom-expected.txt?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSCalculationValueTest.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/win/virtual/legacy-animations-engine/animations/interpolation/line-height-interpolation-expected.txt?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/HTMLMetaElement-in.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/platform/linux/virtual/legacy-animations-engine/animations/interpolation/line-height-interpolation-expected.txt?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/masking/parsing-clip-path-shape.html?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderGrid.cpp?r1=164448&r2=164447&pathrev=164448
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-page-zoom-expected.txt?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderTable.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSPrimitiveValue.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/FontBuilder.h?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSLengthFunctions.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/web/PageScaleConstraintsSet.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/css/computed-offset-with-zoom.html?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/svg/RenderSVGRoot.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/frame/FrameView.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/MediaQueryEvaluator.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderBR.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSToLengthConversionData.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderScrollbarPart.cpp?r1=164448&r2=164447&pathrev=164448
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-page-zoom.html?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/shapes/parsing/parsing-shape-lengths-expected.txt?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSGradientValue.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderView.h?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderReplaced.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/MatchedPropertiesCache.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/FontBuilder.cpp?r1=164448&r2=164447&pathrev=164448
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-resize-expected.txt?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSComputedStyleDeclaration.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderBoxModelObject.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/TreeScope.h?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/animations/interpolation/line-height-interpolation-expected.txt?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/web/WebViewImpl.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/shapes/parsing/parsing-shape-lengths.html?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSLengthFunctions.h?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderFlexibleBox.h?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/svg/SVGSVGElement.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSToLengthConversionData.h?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderThemeChromiumMac.mm?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/StyleBuilderCustom.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderInline.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSCalculationValue.cpp?r1=164448&r2=164447&pathrev=164448
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/css3/viewport-percentage-lengths/viewport-percentage-lengths-resize.html?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/Document.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/StyleBuilderConverter.h?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/ViewportDescription.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/MatchedPropertiesCache.h?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderBox.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSMatrix.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/platform/LengthFunctions.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderText.h?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderBlock.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/TreeScope.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderTableCell.h?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/CSSPrimitiveValueMappings.h?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/HTMLAreaElement.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/ViewportStyleResolver.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/resolver/StyleResolver.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/style/RenderStyle.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle-expected.txt?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderFlexibleBox.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderObject.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderTableSection.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/animation/css/CSSAnimatableValueFactory.cpp?r1=164448&r2=164447&pathrev=164448
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/Document.h?r1=164448&r2=164447&pathrev=164448

Move viewport unit resolution to style recalc time

This patch moves the resolution of viewport units to style recalc time.
Currently viewport units are left unresolved during style recalcs which
leads to several problems (see linked bugs). Moving the resolution will
fix these problems, as well as reduce the plumbing that goes on. 

This patch is touches a lot of files since the valueForLength functions
now no longer need a RenderView, but the interesting changes are in:

- CSSToLengthConversionData -> CSSPrimitiveValue, for moving resolution
to style recalc time
- Length / LengthFunction, no longer needs to know about viewport units
- FrameView -> Document -> MatchedPropertiesCache, for scheduling style
recalcs upon resize

Viewport Lengths are currently also used in viewport descriptions (that
is, @viewport and the viewport meta tag). I've added to Length two more
enum values (DeviceWidth and DeviceHeight), alongside ExtendToZoom.

Note that getComputedStyle will now do the right thing and return pixel
values when viewport units are used. This behaviour matches Firefox and
the css3-cascade spec.

I've added a test for the most starred bug (124331) and for the various
other issues fixed by this patch I'll make test-only patches.

Note that calc integration with viewport units will be fixed separately
to simplify reviews.

TBR=dglazkov@chromium.org
BUG= 124331 , 137617 , 125709 , 261298 , 173407 , 322365 , 310874 , 311465 

Review URL: https://codereview.chromium.org/82083002
------------------------------------------------------------------------

Comment 6 by timloh@chromium.org, Sep 25 2014

Owner: timloh@chromium.org
Status: Fixed

Sign in to add a comment