CSS Length calculation with MediaValues

CSS Length calculation with MediaValues

BUG=357586

[Yoav Weiss, 2014-04-08 06:51:33]

This CL makes sure that MediaValues is used for length calculations in the
MediaQueryParser, making sure that lengths beyond pw and em are supported. It is
also required for future length calculations in the SizesAttributeParser.

[eseidel, 2014-04-08 16:43:37]

I find this CSSToLengthConversionData thing kinda scary.
#1 that it mutates style as a side-effect of calling accessors!
#2 that it can have so many configurations (style being null, rootStyle being
null, and now mediaValues being null.)

I'm not sure I'm excited about the design of CSSToLengthConversionData, but that
doesn't necessarily need to be a negative of your patch.

[eseidel, 2014-04-08 16:51:28]

I think this is generally OK.  Please see my comments below.  Lets give the
other CC'd CSS folks a chance to comment and we can discuss again this evening.

https://codereview.chromium.org/227043007/diff/40001/Source/core/css/CSSToLen...
File Source/core/css/CSSToLengthConversionData.cpp (left):

https://codereview.chromium.org/227043007/diff/40001/Source/core/css/CSSToLen...
Source/core/css/CSSToLengthConversionData.cpp:87:
m_style->setHasViewportUnits();
Accessors shouldn't mutate things as a side-effect.  Obviously not the fault of
your patch, but this design seems wrong.  (I know we have similar problems where
we mutate styles during matching unfortunately)

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

https://codereview.chromium.org/227043007/diff/40001/Source/core/css/CSSToLen...
Source/core/css/CSSToLengthConversionData.h:80: const MediaValues*
m_mediaValues; // Can't be a RefPtr since it would create a dependency cycle.
Could you elaborate more on the cycle?

https://codereview.chromium.org/227043007/diff/40001/Source/core/css/MediaQue...
File Source/core/css/MediaQueryEvaluator.cpp (right):

https://codereview.chromium.org/227043007/diff/40001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluator.cpp:351: result =
primitiveValue->computeLength<int>(CSSToLengthConversionData(mediaValues.style(),
mediaValues.style(), 0, 1.0 /* zoom */, true /* computingFontSize */));
Any time we have to comment args like this we should be considering using a
struct for args instead.

FooArgs args;
args.mediaValues = mediaValues;
args.whatever = 0;
args.zoom = 1.0;
args.computingFontSize = true;

That makes the code much harder to get wrong. :)

Obviously not your "fault" here, but something we should consider if we're in
this code more.

https://codereview.chromium.org/227043007/diff/40001/Source/core/css/MediaQue...
File Source/core/css/MediaQueryEvaluatorTest.cpp (right):

https://codereview.chromium.org/227043007/diff/40001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluatorTest.cpp:81: 16, // Computed font size
Same problem. :)  Structs are how c++ does named arguments.

https://codereview.chromium.org/227043007/diff/40001/Source/core/css/MediaVal...
File Source/core/css/MediaValues.cpp (right):

https://codereview.chromium.org/227043007/diff/40001/Source/core/css/MediaVal...
Source/core/css/MediaValues.cpp:360: if (m_mode == DynamicMode)
I don't understand caching vs. dynamic mode.  Why are these the same object if
they have such different behavior?

[esprehn, 2014-04-08 17:40:38]

Are you using RenderStyle from another thread?

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

https://codereview.chromium.org/227043007/diff/40001/Source/core/css/CSSToLen...
Source/core/css/CSSToLengthConversionData.cpp:53:
CSSToLengthConversionData::CSSToLengthConversionData(const MediaValues*
mediaValues, float zoom, bool computingFontSize)
This should be a reference

https://codereview.chromium.org/227043007/diff/40001/Source/core/css/CSSToLen...
Source/core/css/CSSToLengthConversionData.cpp:58: , m_viewportHeight(mediaValues
? mediaValues->viewportHeight() : 0)
These null checks make no sense, you ASSERT(mediaValues) below.

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

https://codereview.chromium.org/227043007/diff/40001/Source/core/css/CSSToLen...
Source/core/css/CSSToLengthConversionData.h:80: const MediaValues*
m_mediaValues; // Can't be a RefPtr since it would create a dependency cycle.
On 2014/04/08 16:51:28, eseidel wrote:
> Could you elaborate more on the cycle?

CSSLengthToConversionData should only be created on the stack, there should be
no cycles.

https://codereview.chromium.org/227043007/diff/40001/Source/core/css/MediaVal...
File Source/core/css/MediaValues.cpp (right):

https://codereview.chromium.org/227043007/diff/40001/Source/core/css/MediaVal...
Source/core/css/MediaValues.cpp:353: if (m_mode == DynamicMode)
What is DynamicMode? You should use two difference objects instead. One that
dynamically resolves it and one that doesn't.

Note that accessing a RenderStyle off the main thread is never safe, you should
be using totally different objects on the other thread.

[Yoav Weiss, 2014-04-08 21:51:34]

On 2014/04/08 17:40:38, esprehn wrote:
> Are you using RenderStyle from another thread?

No. MediaValues caches the required valuesfrom MQ eval from RenderStyle and
LocalFrame. RenderStyle and LocaFrame themselves are not passed around between
threads.

> 
>
https://codereview.chromium.org/227043007/diff/40001/Source/core/css/CSSToLen...
> File Source/core/css/CSSToLengthConversionData.cpp (right):
> 
>
https://codereview.chromium.org/227043007/diff/40001/Source/core/css/CSSToLen...
> Source/core/css/CSSToLengthConversionData.cpp:53:
> CSSToLengthConversionData::CSSToLengthConversionData(const MediaValues*
> mediaValues, float zoom, bool computingFontSize)
> This should be a reference
> 
>
https://codereview.chromium.org/227043007/diff/40001/Source/core/css/CSSToLen...
> Source/core/css/CSSToLengthConversionData.cpp:58: ,
m_viewportHeight(mediaValues
> ? mediaValues->viewportHeight() : 0)
> These null checks make no sense, you ASSERT(mediaValues) below.
> 
>
https://codereview.chromium.org/227043007/diff/40001/Source/core/css/CSSToLen...
> File Source/core/css/CSSToLengthConversionData.h (right):
> 
>
https://codereview.chromium.org/227043007/diff/40001/Source/core/css/CSSToLen...
> Source/core/css/CSSToLengthConversionData.h:80: const MediaValues*
> m_mediaValues; // Can't be a RefPtr since it would create a dependency cycle.
> On 2014/04/08 16:51:28, eseidel wrote:
> > Could you elaborate more on the cycle?
> 
> CSSLengthToConversionData should only be created on the stack, there should be
> no cycles.

The cycle I saw here is a header include cycle, not a ref counting cycle.
Adding a RefPtr<MediaValues> here requires including MediaValues.h, which
requires StyleResolverState.h (because of a RefPtr<RenderStyle>), which requires
CSSToLengthConversionData.h
> 
>
https://codereview.chromium.org/227043007/diff/40001/Source/core/css/MediaVal...
> File Source/core/css/MediaValues.cpp (right):
> 
>
https://codereview.chromium.org/227043007/diff/40001/Source/core/css/MediaVal...
> Source/core/css/MediaValues.cpp:353: if (m_mode == DynamicMode)
> What is DynamicMode? You should use two difference objects instead. One that
> dynamically resolves it and one that doesn't.

Yeah, I think I'll create a single interface, and 2 objects that implement it,
one for dynamic values, on main thread, that uses RenderStyle and LocalFrame,
and the other for cached values, off the main thread.
> 
> Note that accessing a RenderStyle off the main thread is never safe, you
should
> be using totally different objects on the other thread.

I'm aware of that, and not using RenderStyle off main thread, but I'll separate
MediaValues into 2 separate objects with a single interface.

[esprehn, 2014-04-08 21:59:04]

On 2014/04/08 21:51:34, Yoav Weiss wrote:
> ...
>
https://codereview.chromium.org/227043007/diff/40001/Source/core/css/CSSToLen...
> > Source/core/css/CSSToLengthConversionData.h:80: const MediaValues*
> > m_mediaValues; // Can't be a RefPtr since it would create a dependency
cycle.
> > On 2014/04/08 16:51:28, eseidel wrote:
> > > Could you elaborate more on the cycle?
> > 
> > CSSLengthToConversionData should only be created on the stack, there should
be
> > no cycles.
> 
> The cycle I saw here is a header include cycle, not a ref counting cycle.
> Adding a RefPtr<MediaValues> here requires including MediaValues.h, which
> requires StyleResolverState.h (because of a RefPtr<RenderStyle>), which
requires
> CSSToLengthConversionData.h
> > 
> >

You should break that cycle by forward declaring the MediaValues class and
moving the constructor and destructor out of line.

[Yoav Weiss, 2014-04-09 14:09:55]

I've uploaded a new patchset that decouples MediaValues from
CSSToLengthConversionData, and add a single interface to both, so that
computeLength can use it.

On 2014/04/08 16:51:28, eseidel wrote:
> I think this is generally OK.  Please see my comments below.  Lets give the
> other CC'd CSS folks a chance to comment and we can discuss again this
evening.
> 
>
https://codereview.chromium.org/227043007/diff/40001/Source/core/css/CSSToLen...
> File Source/core/css/CSSToLengthConversionData.cpp (left):
> 
>
https://codereview.chromium.org/227043007/diff/40001/Source/core/css/CSSToLen...
> Source/core/css/CSSToLengthConversionData.cpp:87:
> m_style->setHasViewportUnits();
> Accessors shouldn't mutate things as a side-effect.  Obviously not the fault
of
> your patch, but this design seems wrong.  (I know we have similar problems
where
> we mutate styles during matching unfortunately)
> 
>
https://codereview.chromium.org/227043007/diff/40001/Source/core/css/CSSToLen...
> File Source/core/css/CSSToLengthConversionData.h (right):
> 
>
https://codereview.chromium.org/227043007/diff/40001/Source/core/css/CSSToLen...
> Source/core/css/CSSToLengthConversionData.h:80: const MediaValues*
> m_mediaValues; // Can't be a RefPtr since it would create a dependency cycle.
> Could you elaborate more on the cycle?

No longer relevant.

> 
>
https://codereview.chromium.org/227043007/diff/40001/Source/core/css/MediaQue...
> File Source/core/css/MediaQueryEvaluator.cpp (right):
> 
>
https://codereview.chromium.org/227043007/diff/40001/Source/core/css/MediaQue...
> Source/core/css/MediaQueryEvaluator.cpp:351: result =
>
primitiveValue->computeLength<int>(CSSToLengthConversionData(mediaValues.style(),
> mediaValues.style(), 0, 1.0 /* zoom */, true /* computingFontSize */));
> Any time we have to comment args like this we should be considering using a
> struct for args instead.
> 
> FooArgs args;
> args.mediaValues = mediaValues;
> args.whatever = 0;
> args.zoom = 1.0;
> args.computingFontSize = true;
> 
> That makes the code much harder to get wrong. :)
> 
> Obviously not your "fault" here, but something we should consider if we're in
> this code more.
> 
>
https://codereview.chromium.org/227043007/diff/40001/Source/core/css/MediaQue...
> File Source/core/css/MediaQueryEvaluatorTest.cpp (right):
> 
>
https://codereview.chromium.org/227043007/diff/40001/Source/core/css/MediaQue...
> Source/core/css/MediaQueryEvaluatorTest.cpp:81: 16, // Computed font size
> Same problem. :)  Structs are how c++ does named arguments.
> 
>
https://codereview.chromium.org/227043007/diff/40001/Source/core/css/MediaVal...
> File Source/core/css/MediaValues.cpp (right):
> 
>
https://codereview.chromium.org/227043007/diff/40001/Source/core/css/MediaVal...
> Source/core/css/MediaValues.cpp:360: if (m_mode == DynamicMode)
> I don't understand caching vs. dynamic mode.  Why are these the same object if
> they have such different behavior?

Dynamic mode means that the values are taken from RenderStyle and LocalFrame
directly, whenever requested. This is a "main thread only" mode.
Cached mode means that the values are initialized from RendrStyle and
LocalFrame, and then never change. RenderStyle and LocalFrame are not kept
around.
The purpose of all this is to provide a single interface to MQEvaluator and to
do the calcs in a single place.
I plan to refactor MediaValues to have an abstract base class with a cached and
dynamic variants. I'll also how "raw value initialization" can be made better.
Should this refactoring be a part of this CL, or a separate one?

[eseidel, 2014-04-09 16:28:29]

I think this is interesting as long as it doesn't hurt perf.  Length-related
functions tend to be super hot. :)

[Yoav Weiss, 2014-04-09 16:31:23]

On 2014/04/09 16:28:29, eseidel wrote:
> I think this is interesting as long as it doesn't hurt perf.  Length-related
> functions tend to be super hot. :)

Any suggested perf tests to test this on?

[esprehn, 2014-04-09 16:38:16]

On 2014/04/09 16:31:23, Yoav Weiss wrote:
> On 2014/04/09 16:28:29, eseidel wrote:
> > I think this is interesting as long as it doesn't hurt perf.  Length-related
> > functions tend to be super hot. :)
> 
> Any suggested perf tests to test this on?

We can't make this change, applyMatchedProperties is super hot and we can't make
all the length conversions it does (which is lots) virtual. You just added
thousands of virtual calls during page load on typical pages.

You can try html-full-render.

[Yoav Weiss, 2014-04-11 13:17:40]

On 2014/04/09 16:38:16, esprehn wrote:
> On 2014/04/09 16:31:23, Yoav Weiss wrote:
> > On 2014/04/09 16:28:29, eseidel wrote:
> > > I think this is interesting as long as it doesn't hurt perf. 
Length-related
> > > functions tend to be super hot. :)
> > 
> > Any suggested perf tests to test this on?
> 
> We can't make this change, applyMatchedProperties is super hot and we can't
make
> all the length conversions it does (which is lots) virtual. You just added
> thousands of virtual calls during page load on typical pages.
> 
> You can try html-full-render.

Abandoning this approach, due to the performance concerns.