Floating point MQs should not match.

Floating point MQs should not match.

Floating point values inside MQs should not be round down/up
to match the integer dimensions of the viewport. That is in order to align our behavior with the way Gecko are doing that.
This CL fixes that issue.

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

[Yoav Weiss, 2014-10-10 08:24:39]

yoav@yoav.ws changed reviewers:
+ rune@opera.com

[Yoav Weiss, 2014-10-10 08:25:43]

As discussed on https://codereview.chromium.org/639783002/ , floating point MQ
lengths should not match by rounding down/up.

[rune, 2014-10-10 09:01:46]

Please file a bug for this. The Bug and/or CL description should say something
about alignment with Gecko/Trident as well (I haven't checked Trident).

https://codereview.chromium.org/644813005/diff/1/Source/core/css/MediaQueryEv...
File Source/core/css/MediaQueryEvaluatorTest.cpp (right):

https://codereview.chromium.org/644813005/diff/1/Source/core/css/MediaQueryEv...
Source/core/css/MediaQueryEvaluatorTest.cpp:67: {"(height: 499.9px)", 0},
Please add tests for the min/max ones too. max 499.9 -> false, min 500.1 ->
false.

https://codereview.chromium.org/644813005/diff/1/Source/core/css/MediaValues.h
File Source/core/css/MediaValues.h (right):

https://codereview.chromium.org/644813005/diff/1/Source/core/css/MediaValues....
Source/core/css/MediaValues.h:44: virtual bool computeLength(double value,
CSSPrimitiveValue::UnitType, float& result) const = 0;
Why did you start using floats instead of doubles?

[Yoav Weiss, 2014-10-10 09:18:57]

On 2014/10/10 09:01:46, rune wrote:
> Please file a bug for this. The Bug and/or CL description should say something
> about alignment with Gecko/Trident as well (I haven't checked Trident).

Added one (mostly quoting you :) )

> 
>
https://codereview.chromium.org/644813005/diff/1/Source/core/css/MediaQueryEv...
> File Source/core/css/MediaQueryEvaluatorTest.cpp (right):
> 
>
https://codereview.chromium.org/644813005/diff/1/Source/core/css/MediaQueryEv...
> Source/core/css/MediaQueryEvaluatorTest.cpp:67: {"(height: 499.9px)", 0},
> Please add tests for the min/max ones too. max 499.9 -> false, min 500.1 ->
> false.

Added.

> 
> https://codereview.chromium.org/644813005/diff/1/Source/core/css/MediaValues.h
> File Source/core/css/MediaValues.h (right):
> 
>
https://codereview.chromium.org/644813005/diff/1/Source/core/css/MediaValues....
> Source/core/css/MediaValues.h:44: virtual bool computeLength(double value,
> CSSPrimitiveValue::UnitType, float& result) const = 0;
> Why did you start using floats instead of doubles?

ComputeLength had no prototype that accepted floats, and looking into the users
of MediaValues, none of them actually needed a double there.
So I preferred moving everything to using floats.
I'm happy to discuss that if you think floats are insufficient here.

[rune, 2014-10-10 12:45:42]

On 2014/10/10 at 09:18:57, yoav wrote:
> On 2014/10/10 09:01:46, rune wrote:
> > Please file a bug for this. The Bug and/or CL description should say
something
> > about alignment with Gecko/Trident as well (I haven't checked Trident).
> 
> Added one (mostly quoting you :) )
> 
> > 
> >
https://codereview.chromium.org/644813005/diff/1/Source/core/css/MediaQueryEv...
> > File Source/core/css/MediaQueryEvaluatorTest.cpp (right):
> > 
> >
https://codereview.chromium.org/644813005/diff/1/Source/core/css/MediaQueryEv...
> > Source/core/css/MediaQueryEvaluatorTest.cpp:67: {"(height: 499.9px)", 0},
> > Please add tests for the min/max ones too. max 499.9 -> false, min 500.1 ->
> > false.
> 
> Added.
> 
> > 
> >
https://codereview.chromium.org/644813005/diff/1/Source/core/css/MediaValues.h
> > File Source/core/css/MediaValues.h (right):
> > 
> >
https://codereview.chromium.org/644813005/diff/1/Source/core/css/MediaValues....
> > Source/core/css/MediaValues.h:44: virtual bool computeLength(double value,
> > CSSPrimitiveValue::UnitType, float& result) const = 0;
> > Why did you start using floats instead of doubles?
> 
> ComputeLength had no prototype that accepted floats, and looking into the
users of MediaValues, none of them actually needed a double there.
> So I preferred moving everything to using floats.
> I'm happy to discuss that if you think floats are insufficient here.

Now there will be some unnecessary double->float->double conversions.

MediaValues store a float for devicePixelRatio, but its source is a double
(LocalFrame::devicePixelRatio), which also leads to float<->double conversions.
I don't understand the rationale.

[rune, 2014-10-10 12:46:04]

https://codereview.chromium.org/644813005/diff/220001/Source/core/css/parser/...
File Source/core/css/parser/SizesCalcParser.cpp (right):

https://codereview.chromium.org/644813005/diff/220001/Source/core/css/parser/...
Source/core/css/parser/SizesCalcParser.cpp:76: value.value = result;
Now you're taking a double (numericValue()), converts it into a float, and then
back into a double value (value.value). I don't think that makes sense.

[Yoav Weiss, 2014-10-10 15:36:06]

On 2014/10/10 12:46:04, rune wrote:
>
https://codereview.chromium.org/644813005/diff/220001/Source/core/css/parser/...
> File Source/core/css/parser/SizesCalcParser.cpp (right):
> 
>
https://codereview.chromium.org/644813005/diff/220001/Source/core/css/parser/...
> Source/core/css/parser/SizesCalcParser.cpp:76: value.value = result;
> Now you're taking a double (numericValue()), converts it into a float, and
then
> back into a double value (value.value). I don't think that makes sense.

Brought everything back to being double.

Also DRYed out the computeLength calls (changing the type in multiple places is
no fun), and added some test cases.

[rune, 2014-10-10 19:32:14]

Lovely. LGTM.

[Yoav Weiss, 2014-10-10 22:02:48]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-10-10 22:03:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/644813005/320001

[commit-bot: I haz the power, 2014-10-10 22:07:28]

Committed patchset #3 (id:320001) as 183563