Remove MediaValues' dependency on RenderStyle

Remove MediaValues' dependency on style

MediaValues needed a RenderStyle for default font calculations. However, since a renderer is not always available,
and the default font size can also be concluded from the LocalFrame, this dependency can be removed.

BUG=357586
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171985

[Yoav Weiss, 2014-04-18 08:10:24]

This is a split from https://codereview.chromium.org/236713005/

[eseidel, 2014-04-18 16:50:13]

I think this code is still a bit confused with the cached vs. dynamic split.

What code ever wants dyanamic values?  Why wouldn't it just re-create cached
values every time the Frame has updated and it needs to read something?

Some code explicitly wants cached values, and does not have a Frame, those it
creates using a struct.

some code explicitly wants cached values, and does have a frame, those it
creates from the frame and saves them.

Some code wants dynamic values and has a frame, those it creates with a
MediaVAluesDynamic (although it could just watch the frame for changes instead
and just use a MediaValuesCached for what it's doing this instant and then
discard it when the frame changes)

Some code wants dynamic values but does not have a frame, those it just gets an
empty MediaVAluesCached instead.

rslgtm. But I think we can still make this simpler.

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

https://codereview.chromium.org/242883002/diff/20001/Source/core/css/MediaVal...
Source/core/css/MediaValues.cpp:144: // We're running in a background thread, so
RenderStyle is not available.
Why is this in MediaValues?  MediaValues could be dynamic and thus not be on a
background thread?

https://codereview.chromium.org/242883002/diff/20001/Source/core/css/MediaVal...
File Source/core/css/MediaValuesCached.cpp (right):

https://codereview.chromium.org/242883002/diff/20001/Source/core/css/MediaVal...
Source/core/css/MediaValuesCached.cpp:35: if (frame) {
Early return instead of long indented block?

https://codereview.chromium.org/242883002/diff/20001/Source/core/css/MediaVal...
Source/core/css/MediaValuesCached.cpp:36: m_data.viewportWidth =
calculateViewportWidth(frame);
I almost wonder if this whole construcor shouldn't be replaced with creating a
MediaValuesDynamic and calling a "cachedCopy()" method on it. :)  CLs for
another day.

https://codereview.chromium.org/242883002/diff/20001/Source/core/css/MediaVal...
File Source/core/css/MediaValuesDynamic.cpp (right):

https://codereview.chromium.org/242883002/diff/20001/Source/core/css/MediaVal...
Source/core/css/MediaValuesDynamic.cpp:17: return adoptRef(new
MediaValuesDynamic(frame));
Why do we ever create a MediaValuesDynamic when the frame is null?  Why not have
a PassRefPtr<MediaValues> MediaValues::fromFrame(frame) method which detects
Frame* is null and just returns the default MediaValuesCached?  Then
MediaValuesDynamic never has to null-check.

https://codereview.chromium.org/242883002/diff/20001/Source/core/html/HTMLIma...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/242883002/diff/20001/Source/core/html/HTMLIma...
Source/core/html/HTMLImageElement.cpp:170: m_effectiveSize =
SizesAttributeParser::findEffectiveSize(value,
MediaValuesCached::create(document()));
Why is this cached?

[Yoav Weiss, 2014-04-18 20:32:12]

On 2014/04/18 16:50:13, eseidel wrote:
> I think this code is still a bit confused with the cached vs. dynamic split.
> 
> What code ever wants dyanamic values?  Why wouldn't it just re-create cached
> values every time the Frame has updated and it needs to read something?

StyleResolver seems to keep a single MediaQueryEvaluator around and expect it to
respond to env changes.
I tried to get rid of the "dynamic" part once and tests broke. I can try again
in a separate CL.
Maybe the answer is to modify MediaQueryEvaluator's clients.

> 
> Some code explicitly wants cached values, and does not have a Frame, those it
> creates using a struct.
> 
> some code explicitly wants cached values, and does have a frame, those it
> creates from the frame and saves them.
> 
> Some code wants dynamic values and has a frame, those it creates with a
> MediaVAluesDynamic (although it could just watch the frame for changes instead
> and just use a MediaValuesCached for what it's doing this instant and then
> discard it when the frame changes)
> 
> Some code wants dynamic values but does not have a frame, those it just gets
an
> empty MediaVAluesCached instead.
You're right that this mode is not needed. I got rid of it as you suggested.

> 
> rslgtm. But I think we can still make this simpler.
> 
>
https://codereview.chromium.org/242883002/diff/20001/Source/core/css/MediaVal...
> File Source/core/css/MediaValues.cpp (right):
> 
>
https://codereview.chromium.org/242883002/diff/20001/Source/core/css/MediaVal...
> Source/core/css/MediaValues.cpp:144: // We're running in a background thread,
so
> RenderStyle is not available.
> Why is this in MediaValues?  MediaValues could be dynamic and thus not be on a
> background thread?
Left over comment. I've removed it.

> 
>
https://codereview.chromium.org/242883002/diff/20001/Source/core/css/MediaVal...
> File Source/core/css/MediaValuesCached.cpp (right):
> 
>
https://codereview.chromium.org/242883002/diff/20001/Source/core/css/MediaVal...
> Source/core/css/MediaValuesCached.cpp:35: if (frame) {
> Early return instead of long indented block?
Definitely :)

> 
>
https://codereview.chromium.org/242883002/diff/20001/Source/core/css/MediaVal...
> Source/core/css/MediaValuesCached.cpp:36: m_data.viewportWidth =
> calculateViewportWidth(frame);
> I almost wonder if this whole construcor shouldn't be replaced with creating a
> MediaValuesDynamic and calling a "cachedCopy()" method on it. :)  CLs for
> another day.
> 
>
https://codereview.chromium.org/242883002/diff/20001/Source/core/css/MediaVal...
> File Source/core/css/MediaValuesDynamic.cpp (right):
> 
>
https://codereview.chromium.org/242883002/diff/20001/Source/core/css/MediaVal...
> Source/core/css/MediaValuesDynamic.cpp:17: return adoptRef(new
> MediaValuesDynamic(frame));
> Why do we ever create a MediaValuesDynamic when the frame is null?  Why not
have
> a PassRefPtr<MediaValues> MediaValues::fromFrame(frame) method which detects
> Frame* is null and just returns the default MediaValuesCached?  Then
> MediaValuesDynamic never has to null-check.
I've added a MediaValues::createDynamicIfFrameExists that does that.

> 
>
https://codereview.chromium.org/242883002/diff/20001/Source/core/html/HTMLIma...
> File Source/core/html/HTMLImageElement.cpp (right):
> 
>
https://codereview.chromium.org/242883002/diff/20001/Source/core/html/HTMLIma...
> Source/core/html/HTMLImageElement.cpp:170: m_effectiveSize =
> SizesAttributeParser::findEffectiveSize(value,
> MediaValuesCached::create(document()));
> Why is this cached?
Since this is used just once and is on the main thread, it can be either.

[Yoav Weiss, 2014-04-18 20:37:00]

Also - I've removed some code from the MediaQueryEvaluator callers, since they
no longer need to provide it with RenderStyle.

[eseidel, 2014-04-18 20:41:33]

lgtm

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

https://codereview.chromium.org/242883002/diff/40001/Source/core/css/MediaVal...
Source/core/css/MediaValuesCached.cpp:37: if (!frame)
I'm not sure you wnat this.  I would just ASSERT(frame) and make the caller
handle !frame.

if (frame)
   MediaValuesCached(frame);
return MediaValuesCached(MediaValuesData);

But this is also OK.

https://codereview.chromium.org/242883002/diff/40001/Source/core/css/MediaVal...
File Source/core/css/MediaValuesDynamic.cpp (left):

https://codereview.chromium.org/242883002/diff/40001/Source/core/css/MediaVal...
Source/core/css/MediaValuesDynamic.cpp:130: return
calculateStrictMode(m_frame.get());
This is much nicer now that Dynamic doesn't have to worry about the !frame case.
:)

[Yoav Weiss, 2014-04-18 22:22:36]

Seems that eliminating the RenderStyle creation have reduced the number of
recalcs.
I've changed the minimal recalc tests that failed as a result accordingly.

[Yoav Weiss, 2014-04-18 22:24:09]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-04-18 22:24:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoav@yoav.ws/242883002/70001

[commit-bot: I haz the power, 2014-04-18 23:25:44]

Change committed as 171985

[eseidel, 2014-04-19 00:42:15]

https://codereview.chromium.org/242883002/diff/70001/Source/core/css/MediaQue...
File Source/core/css/MediaQueryMatcher.cpp (left):

https://codereview.chromium.org/242883002/diff/70001/Source/core/css/MediaQue...
Source/core/css/MediaQueryMatcher.cpp:85: RefPtr<RenderStyle> rootStyle =
styleResolver.styleForElement(documentElement, 0 /*defaultParent*/,
DisallowStyleSharing, MatchOnlyUserAgentRules);
Yes, you're removing this recalc, so it's not surprisng that those number go
down.