Wrap CSS length conversion arguments in an object

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
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162881

[esprehn, 2013-11-24 23:49:52]

On 2013/11/24 23:37:03, Timothy Loh wrote:

You should upload with the no-find-copies flag. It also seems the getters should
return references for root style and style. They're always defined when we'd
access this.

[Timothy Loh, 2013-11-25 00:09:20]

On 2013/11/24 23:49:52, esprehn wrote:
> On 2013/11/24 23:37:03, Timothy Loh wrote:
> 
> You should upload with the no-find-copies flag. It also seems the getters
should
> return references for root style and style. They're always defined when we'd
> access this.

I wanted to do this (or at least assert they were non-null), but the nonstandard
WebKitCSSMatrix interface (CSSMatrix.cpp) doesn't have the styles available.
We assume it is non-null in computeLength but convertToLength returns
Length(Undefined) if we're font-relative and use ems/etc.

Probably we should see if we can remove WebKitCSSMatrix. I think this would let
us
have the styling information as non-null, and also maybe remove
Length(Undefined).

[Timothy Loh, 2013-11-25 02:23:37]

On 2013/11/25 00:09:20, Timothy Loh wrote:
> I wanted to do this (or at least assert they were non-null), but the
nonstandard
> WebKitCSSMatrix interface (CSSMatrix.cpp) doesn't have the styles available.
> We assume it is non-null in computeLength but convertToLength returns
> Length(Undefined) if we're font-relative and use ems/etc.

Okay, changed this in patch set 5 so that CSSMatrix just creates a default
RenderStyle to use, and made the getters return references. I don't expect
anyone would be using ems in a WebKitCSSMatrix anyway, given that it isn't
bound to an element.

[Julien - ping for review, 2013-11-26 05:31:06]

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/CSSToLen...
File Source/core/css/CSSToLengthConversionData.cpp (right):

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/CSSToLen...
Source/core/css/CSSToLengthConversionData.cpp:45: ASSERT(zoom == -1 || zoom >
0);
I don't understand why we need the default value is -1 (magic constant btw). Why
can't we just fill m_zoom from the current style's effective zoom in the
constructor?

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/CSSToLen...
Source/core/css/CSSToLengthConversionData.cpp:55: CSSToLengthConversionData
CSSToLengthConversionData::copyWithAdjustedZoom(float newZoom) const
The need for this function makes me wonder if this is helping us. As you point
out, the new class requires some copying due to changes in zoom, which is really
annoying.

One solution would be to move zoom out of the class and pass it along to
computeLength(). Another solution is to expose some setZoom setter.

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/CSSToLen...
File Source/core/css/CSSToLengthConversionData.h (right):

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/CSSToLen...
Source/core/css/CSSToLengthConversionData.h:38: class CSSToLengthConversionData
{
Do we really need to allow the copy constructor / assignment?

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/CSSToLen...
Source/core/css/CSSToLengthConversionData.h:40: CSSToLengthConversionData() :
m_style(0), m_rootStyle(0), m_zoom(-1), m_computingFontSize(false) { }
Blink style is one variable per line.

Also why do we need a default constructor?

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/MediaQue...
File Source/core/css/MediaQueryEvaluator.cpp (left):

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.cpp:350: result =
primitiveValue->computeLength<int>(initialStyle, initialStyle, 1.0 /* multiplier
*/, true /* computingFontSize */);
/* zoom */ ?

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/resolver...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/resolver...
Source/core/css/resolver/StyleBuilderCustom.cpp:347: static Length
mmLength(double mm) { return Length(mm * cssPixelsPerInch / 25.4, Fixed); }
25.4 == another magic constant!

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/resolver...
File Source/core/css/resolver/ViewportStyleResolver.cpp (right):

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/resolver...
Source/core/css/resolver/ViewportStyleResolver.cpp:187: return
primitiveValue->computeLength<Length>(CSSToLengthConversionData(m_document->renderStyle(),
m_document->renderStyle(), 1));
1.0f <- let's avoid implicit conversion.

[Timothy Loh, 2013-11-27 05:44:50]

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/CSSToLen...
File Source/core/css/CSSToLengthConversionData.cpp (right):

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/CSSToLen...
Source/core/css/CSSToLengthConversionData.cpp:45: ASSERT(zoom == -1 || zoom >
0);
On 2013/11/26 05:31:07, Julien Chaffraix - AEDT wrote:
> I don't understand why we need the default value is -1 (magic constant btw).
Why
> can't we just fill m_zoom from the current style's effective zoom in the
> constructor?

Note that we'd also need to change the zoom whenever the style or style's zoom
is changed or style is changed (this is probably just
StyleResolverState::setStyle/setZoom/setEffectiveZoom). I went with changing the
magic constant to a separate boolean which says if we want to use the style's
zoom (this requires an extra ctor). WDYT?

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/CSSToLen...
Source/core/css/CSSToLengthConversionData.cpp:55: CSSToLengthConversionData
CSSToLengthConversionData::copyWithAdjustedZoom(float newZoom) const
On 2013/11/26 05:31:07, Julien Chaffraix - AEDT wrote:
> The need for this function makes me wonder if this is helping us. As you point
> out, the new class requires some copying due to changes in zoom, which is
really
> annoying.
> 
> One solution would be to move zoom out of the class and pass it along to
> computeLength(). Another solution is to expose some setZoom setter.

Agreed that this is a bit annoying. Having a setter is probably not what we
want; we'd have to probably set the zoom back after and it makes it a bit messy
(also we'll have to unconst the StyleResolverState getter). Passing the zoom
alongside the rest of the data is what this patch is trying to avoid.

The copying here is pretty cheap, and presumably we do about as much currently
anyway when all the arguments get plumbed through everywhere.

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/CSSToLen...
File Source/core/css/CSSToLengthConversionData.h (right):

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/CSSToLen...
Source/core/css/CSSToLengthConversionData.h:38: class CSSToLengthConversionData
{
On 2013/11/26 05:31:07, Julien Chaffraix - AEDT wrote:
> Do we really need to allow the copy constructor / assignment?

I don't think it matters. Disallowing to avoid any unneeded copies.

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/CSSToLen...
Source/core/css/CSSToLengthConversionData.h:40: CSSToLengthConversionData() :
m_style(0), m_rootStyle(0), m_zoom(-1), m_computingFontSize(false) { }
On 2013/11/26 05:31:07, Julien Chaffraix - AEDT wrote:
> Blink style is one variable per line.
> 
> Also why do we need a default constructor?

I've seen having constructor lists on the same line in quite a few places and
grep turns up plenty of ctors with the ctor list on the same line.

Removed default ctor.

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/MediaQue...
File Source/core/css/MediaQueryEvaluator.cpp (left):

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.cpp:350: result =
primitiveValue->computeLength<int>(initialStyle, initialStyle, 1.0 /* multiplier
*/, true /* computingFontSize */);
On 2013/11/26 05:31:07, Julien Chaffraix - AEDT wrote:
> /* zoom */ ?

Err okay sure. Not sure why I dropped the comment.

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/resolver...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/resolver...
Source/core/css/resolver/StyleBuilderCustom.cpp:347: static Length
mmLength(double mm) { return Length(mm * cssPixelsPerInch / 25.4, Fixed); }
On 2013/11/26 05:31:07, Julien Chaffraix - AEDT wrote:
> 25.4 == another magic constant!

Fixed

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/resolver...
File Source/core/css/resolver/ViewportStyleResolver.cpp (right):

https://codereview.chromium.org/64293008/diff/340001/Source/core/css/resolver...
Source/core/css/resolver/ViewportStyleResolver.cpp:187: return
primitiveValue->computeLength<Length>(CSSToLengthConversionData(m_document->renderStyle(),
m_document->renderStyle(), 1));
On 2013/11/26 05:31:07, Julien Chaffraix - AEDT wrote:
> 1.0f <- let's avoid implicit conversion.

The style guide says not to append .0f... but after adding the extra ctor (see
other comment) it's needed to disambiguate between bool and float ;-)

[alancutter (OOO until 2018), 2013-11-27 09:30:58]

lgtm, this is a very welcome step towards taming the ugliness around resolving
CSS lengths.

https://codereview.chromium.org/64293008/diff/420001/Source/core/css/CSSToLen...
File Source/core/css/CSSToLengthConversionData.cpp (right):

https://codereview.chromium.org/64293008/diff/420001/Source/core/css/CSSToLen...
Source/core/css/CSSToLengthConversionData.cpp:55: }
It probably doesn't hurt to have these in the header file where they can get
inlined.

https://codereview.chromium.org/64293008/diff/420001/Source/core/css/CSSToSty...
File Source/core/css/CSSToStyleMap.h (right):

https://codereview.chromium.org/64293008/diff/420001/Source/core/css/CSSToSty...
Source/core/css/CSSToStyleMap.h:79: const RenderStyle* style() const;
The style() method can probably be removed from CSSToStyleMap now, there appears
to only be one usage of it left.

https://codereview.chromium.org/64293008/diff/420001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolverState.cpp (right):

https://codereview.chromium.org/64293008/diff/420001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolverState.cpp:50:
m_cssToLengthConversionData.setRootStyle(rootElementStyle());
This can be moved into the constructor list (as discussed).

[Timothy Loh, 2013-11-28 03:41:53]

https://codereview.chromium.org/64293008/diff/420001/Source/core/css/CSSToLen...
File Source/core/css/CSSToLengthConversionData.cpp (right):

https://codereview.chromium.org/64293008/diff/420001/Source/core/css/CSSToLen...
Source/core/css/CSSToLengthConversionData.cpp:55: }
On 2013/11/27 09:30:58, alancutter wrote:
> It probably doesn't hurt to have these in the header file where they can get
> inlined.

Ok moved ctors to header. The zoom function requires the RenderStyle include so
leaving that in the .cpp

https://codereview.chromium.org/64293008/diff/420001/Source/core/css/CSSToSty...
File Source/core/css/CSSToStyleMap.h (right):

https://codereview.chromium.org/64293008/diff/420001/Source/core/css/CSSToSty...
Source/core/css/CSSToStyleMap.h:79: const RenderStyle* style() const;
On 2013/11/27 09:30:58, alancutter wrote:
> The style() method can probably be removed from CSSToStyleMap now, there
appears
> to only be one usage of it left.

Done.

https://codereview.chromium.org/64293008/diff/420001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolverState.cpp (right):

https://codereview.chromium.org/64293008/diff/420001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolverState.cpp:50:
m_cssToLengthConversionData.setRootStyle(rootElementStyle());
On 2013/11/27 09:30:58, alancutter wrote:
> This can be moved into the constructor list (as discussed).

Done.

[dstockwell, 2013-11-29 03:54:36]

rslgtm

[commit-bot: I haz the power, 2013-11-29 03:58:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/timloh@chromium.org/64293008/460001

[commit-bot: I haz the power, 2013-11-29 04:42:15]

Retried try job too often on mac_layout for step(s) webkit_lint
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-11-29 07:03:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/timloh@chromium.org/64293008/480001

[commit-bot: I haz the power, 2013-11-29 09:57:21]

Change committed as 162881