Avoid rounding down viewport dimensions in Media Queries

Avoid rounding down viewport dimensions in Media Queries

Viewport dimensions were assumed to be integer numbers, but that assumption
fails when zoom is taken into account, leading to compatibility issues.
This CL fixes that, by changing viewport dimensions to be treated as floats.

BUG=560874
Committed: https://crrev.com/e4518f06d1a860c5830ff96817ffe6bf71616e87
Cr-Commit-Position: refs/heads/master@{#363181}

[Yoav Weiss, 2015-12-02 21:29:25]

yoav@yoav.ws changed reviewers:
+ timloh@chromium.org

[Yoav Weiss, 2015-12-02 22:04:05]

Description was changed from

==========
Avoid rounding down viewport dimensions in Media Queries

Viewport dimensions were assumed to be integer numbers, but that assumption
fails when zoom is taken into account, leading to compatibility issues.
This CL fixes that, by changing viewport dimensions to be treated as floats.

BUG=560874
==========

to

==========
Avoid rounding down viewport dimensions in Media Queries

Viewport dimensions were assumed to be integer numbers, but that assumption
fails when zoom is taken into account, leading to compatibility issues.
This CL fixes that, by changing viewport dimensions to be treated as floats.

BUG=560874
==========

[Yoav Weiss, 2015-12-02 22:04:05]

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

[rune, 2015-12-04 08:20:59]

https://codereview.chromium.org/1496683002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/MediaValues.cpp (right):

https://codereview.chromium.org/1496683002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/MediaValues.cpp:156: bool
MediaValues::computeLengthImpl(double value, CSSPrimitiveValue::UnitType type,
unsigned defaultFontSize, float viewportWidth, float viewportHeight, double&
result)
Why did you choose float and not double for the viewport height/width? Computing
lengths and adjusting for zoom both multiplies it with double values. Is it to
save memory in CachedMediaValues?

[Yoav Weiss, 2015-12-04 08:26:54]

On 2015/12/04 08:20:59, rune wrote:
>
https://codereview.chromium.org/1496683002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/css/MediaValues.cpp (right):
> 
>
https://codereview.chromium.org/1496683002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/MediaValues.cpp:156: bool
> MediaValues::computeLengthImpl(double value, CSSPrimitiveValue::UnitType type,
> unsigned defaultFontSize, float viewportWidth, float viewportHeight, double&
> result)
> Why did you choose float and not double for the viewport height/width?
Computing
> lengths and adjusting for zoom both multiplies it with double values. Is it to
> save memory in CachedMediaValues?

Yeah, while thinking that double precision is not necessarily required for
viewport dimensions. If you prefer, I can switch it to double

[rune, 2015-12-04 08:39:12]

On 2015/12/04 08:26:54, Yoav Weiss wrote:
> On 2015/12/04 08:20:59, rune wrote:
> >
>
https://codereview.chromium.org/1496683002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/css/MediaValues.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1496683002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/css/MediaValues.cpp:156: bool
> > MediaValues::computeLengthImpl(double value, CSSPrimitiveValue::UnitType
type,
> > unsigned defaultFontSize, float viewportWidth, float viewportHeight, double&
> > result)
> > Why did you choose float and not double for the viewport height/width?
> Computing
> > lengths and adjusting for zoom both multiplies it with double values. Is it
to
> > save memory in CachedMediaValues?
> 
> Yeah, while thinking that double precision is not necessarily required for
> viewport dimensions. If you prefer, I can switch it to double

Just that the code looks less clean and consistent with things like
clampTo<float>. If we don't create a whole lot of MediaValuesCached, I think we
should use double.

[Yoav Weiss, 2015-12-04 09:19:47]

On 2015/12/04 08:39:12, rune wrote:
> On 2015/12/04 08:26:54, Yoav Weiss wrote:
> > On 2015/12/04 08:20:59, rune wrote:
> > >
> >
>
https://codereview.chromium.org/1496683002/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/css/MediaValues.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1496683002/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/css/MediaValues.cpp:156: bool
> > > MediaValues::computeLengthImpl(double value, CSSPrimitiveValue::UnitType
> type,
> > > unsigned defaultFontSize, float viewportWidth, float viewportHeight,
double&
> > > result)
> > > Why did you choose float and not double for the viewport height/width?
> > Computing
> > > lengths and adjusting for zoom both multiplies it with double values. Is
it
> to
> > > save memory in CachedMediaValues?
> > 
> > Yeah, while thinking that double precision is not necessarily required for
> > viewport dimensions. If you prefer, I can switch it to double
> 
> Just that the code looks less clean and consistent with things like
> clampTo<float>. If we don't create a whole lot of MediaValuesCached, I think
we
> should use double.

Fair enough. Changed it to double

[rune, 2015-12-04 09:22:18]

lgtm

[Yoav Weiss, 2015-12-04 09:25:24]

On 2015/12/04 09:22:18, rune wrote:
> lgtm

Thanks for reviewing :)

[Yoav Weiss, 2015-12-04 09:25:32]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2015-12-04 09:25:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1496683002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1496683002/60001

[commit-bot: I haz the power, 2015-12-04 10:30:51]

Description was changed from

==========
Avoid rounding down viewport dimensions in Media Queries

Viewport dimensions were assumed to be integer numbers, but that assumption
fails when zoom is taken into account, leading to compatibility issues.
This CL fixes that, by changing viewport dimensions to be treated as floats.

BUG=560874
==========

to

==========
Avoid rounding down viewport dimensions in Media Queries

Viewport dimensions were assumed to be integer numbers, but that assumption
fails when zoom is taken into account, leading to compatibility issues.
This CL fixes that, by changing viewport dimensions to be treated as floats.

BUG=560874
==========

[commit-bot: I haz the power, 2015-12-04 10:30:52]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-04 10:31:54]

Description was changed from

==========
Avoid rounding down viewport dimensions in Media Queries

Viewport dimensions were assumed to be integer numbers, but that assumption
fails when zoom is taken into account, leading to compatibility issues.
This CL fixes that, by changing viewport dimensions to be treated as floats.

BUG=560874
==========

to

==========
Avoid rounding down viewport dimensions in Media Queries

Viewport dimensions were assumed to be integer numbers, but that assumption
fails when zoom is taken into account, leading to compatibility issues.
This CL fixes that, by changing viewport dimensions to be treated as floats.

BUG=560874

Committed: https://crrev.com/e4518f06d1a860c5830ff96817ffe6bf71616e87
Cr-Commit-Position: refs/heads/master@{#363181}
==========

[commit-bot: I haz the power, 2015-12-04 10:31:55]

Patchset 4 (id:??) landed as
https://crrev.com/e4518f06d1a860c5830ff96817ffe6bf71616e87
Cr-Commit-Position: refs/heads/master@{#363181}