Use float literals to decide when to blacklist low scaling factors

Use float literals to decide when to blacklist low scaling factors

The comparison to decide whether to blacklist rounded scaling factors
less than 1.3 is comparing a float (the 'rounded' variable) with a
double (the '1.3' literal) which won't yield the intended results
when 'rounded' is set to the 1.3 float value.

So, we need to use the '1.3f' literal here to make sure it's a float,
otherwise the comparison will effectively work as 'rounded < 1.4f'.

Also, we do the comparison before rounding the scaling factor, so that
we use the original value to decide whether to blacklist it or not,
otherwise it would misbehave for values between 1.25 - 1.29, as they
would get rounded up to 1.3 before the check.

BUG=484400
R=oshima@chromium.org, estade@chromium.org

Committed: https://crrev.com/e2ff3e6efc9f9d58ba0520b12e313e46be3abef8
Cr-Commit-Position: refs/heads/master@{#370248}

[mario.endlessm, 2016-01-13 09:58:02]

The following patch implements the fix proposed in
https://codereview.chromium.org/1324513002/#msg11.

Please review, thanks.

[oshima, 2016-01-13 23:39:44]

https://codereview.chromium.org/1581883003/diff/1/chrome/browser/ui/libgtk2ui...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):

https://codereview.chromium.org/1581883003/diff/1/chrome/browser/ui/libgtk2ui...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:1358: return rounded < 1.3f ? 1.0f :
rounded;
I think it's better to be

return scale < 1.3f ? 1.0f : roundf(scale * 10) / 10;

[mario.endlessm, 2016-01-14 10:43:02]

https://codereview.chromium.org/1581883003/diff/1/chrome/browser/ui/libgtk2ui...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):

https://codereview.chromium.org/1581883003/diff/1/chrome/browser/ui/libgtk2ui...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:1358: return rounded < 1.3f ? 1.0f :
rounded;
On 2016/01/13 23:39:44, oshima wrote:
> I think it's better to be
> 
> return scale < 1.3f ? 1.0f : roundf(scale * 10) / 10;

Agreed. I just did not change it now because that changes the semantics a bit,
although they seem to change them for the better (or at least to be consistent
with the description of the initial patch), so I'll update the patch.

Thanks

[mario.endlessm, 2016-01-14 10:45:08]

Description was changed from

==========
Use float literals to decide when to blacklist low scaling factors

The comparison to decide whether to blacklist rounded scaling factors
less than 1.3 is comparing a float (the 'rounded' variable) with a
double (the '1.3' literal) which won't yield the intended results
when 'rounded' is set to the 1.3 float value.

So, we need to use the '1.3f' literal here to make sure it's a float,
otherwise the comparison will effectively work as 'rounded < 1.4f'.

BUG=484400
R=oshima@chromium.org, estade@chromium.org
==========

to

==========
Use float literals to decide when to blacklist low scaling factors

The comparison to decide whether to blacklist rounded scaling factors
less than 1.3 is comparing a float (the 'rounded' variable) with a
double (the '1.3' literal) which won't yield the intended results
when 'rounded' is set to the 1.3 float value.

So, we need to use the '1.3f' literal here to make sure it's a float,
otherwise the comparison will effectively work as 'rounded < 1.4f'.

Also, we do the comparison before rounding the scaling factor, so that
we use the original value to decide whether to blacklist it or not,
otherwise it would misbehave for values between 1.25 - 1.29, as they
would get rounded up to 1.3 before the check.

BUG=484400
R=oshima@chromium.org, estade@chromium.org
==========

[oshima, 2016-01-14 16:01:59]

lgtm

[mario.endlessm, 2016-01-14 16:03:10]

The CQ bit was checked by mario@endlessm.com

[commit-bot: I haz the power, 2016-01-14 16:04:13]

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

[commit-bot: I haz the power, 2016-01-14 16:11:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-14 16:11:17]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mario.endlessm, 2016-01-14 16:22:31]

On 2016/01/14 16:11:17, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

This failed due to a missing approval from an OWNER.

@estade, please take a look when you have a chance, thanks!

[Evan Stade, 2016-01-19 19:27:49]

lgtm

[mario.endlessm, 2016-01-19 23:36:55]

The CQ bit was checked by mario@endlessm.com

[commit-bot: I haz the power, 2016-01-19 23:38:17]

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

[commit-bot: I haz the power, 2016-01-20 01:08:47]

Description was changed from

==========
Use float literals to decide when to blacklist low scaling factors

The comparison to decide whether to blacklist rounded scaling factors
less than 1.3 is comparing a float (the 'rounded' variable) with a
double (the '1.3' literal) which won't yield the intended results
when 'rounded' is set to the 1.3 float value.

So, we need to use the '1.3f' literal here to make sure it's a float,
otherwise the comparison will effectively work as 'rounded < 1.4f'.

Also, we do the comparison before rounding the scaling factor, so that
we use the original value to decide whether to blacklist it or not,
otherwise it would misbehave for values between 1.25 - 1.29, as they
would get rounded up to 1.3 before the check.

BUG=484400
R=oshima@chromium.org, estade@chromium.org
==========

to

==========
Use float literals to decide when to blacklist low scaling factors

The comparison to decide whether to blacklist rounded scaling factors
less than 1.3 is comparing a float (the 'rounded' variable) with a
double (the '1.3' literal) which won't yield the intended results
when 'rounded' is set to the 1.3 float value.

So, we need to use the '1.3f' literal here to make sure it's a float,
otherwise the comparison will effectively work as 'rounded < 1.4f'.

Also, we do the comparison before rounding the scaling factor, so that
we use the original value to decide whether to blacklist it or not,
otherwise it would misbehave for values between 1.25 - 1.29, as they
would get rounded up to 1.3 before the check.

BUG=484400
R=oshima@chromium.org, estade@chromium.org
==========

[commit-bot: I haz the power, 2016-01-20 01:08:48]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-20 01:10:00]

Description was changed from

==========
Use float literals to decide when to blacklist low scaling factors

The comparison to decide whether to blacklist rounded scaling factors
less than 1.3 is comparing a float (the 'rounded' variable) with a
double (the '1.3' literal) which won't yield the intended results
when 'rounded' is set to the 1.3 float value.

So, we need to use the '1.3f' literal here to make sure it's a float,
otherwise the comparison will effectively work as 'rounded < 1.4f'.

Also, we do the comparison before rounding the scaling factor, so that
we use the original value to decide whether to blacklist it or not,
otherwise it would misbehave for values between 1.25 - 1.29, as they
would get rounded up to 1.3 before the check.

BUG=484400
R=oshima@chromium.org, estade@chromium.org
==========

to

==========
Use float literals to decide when to blacklist low scaling factors

The comparison to decide whether to blacklist rounded scaling factors
less than 1.3 is comparing a float (the 'rounded' variable) with a
double (the '1.3' literal) which won't yield the intended results
when 'rounded' is set to the 1.3 float value.

So, we need to use the '1.3f' literal here to make sure it's a float,
otherwise the comparison will effectively work as 'rounded < 1.4f'.

Also, we do the comparison before rounding the scaling factor, so that
we use the original value to decide whether to blacklist it or not,
otherwise it would misbehave for values between 1.25 - 1.29, as they
would get rounded up to 1.3 before the check.

BUG=484400
R=oshima@chromium.org, estade@chromium.org

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

[commit-bot: I haz the power, 2016-01-20 01:10:03]

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