Round when calculating minimum height of WebViews

Round when calculating minimum height of WebViews

Currently WebViews will truncate pixel numbers when calculating the
minimum height for WebViews. This will lead to incorrect results on some
devices, which consider the default zoom level to be slightly
less than 1 and truncate at multiplication.
Adding .5 before truncating numbers is a simple and efficient way to
round, thus leading to correct sizes on all devices.

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

[kraush, 2015-04-09 23:12:33]

kraush@amazon.com changed reviewers:
+ kbr@chromium.org

[kraush, 2015-04-09 23:14:41]

Hi Ken,

Here's a proposed fix for bug number 475730.
PTAL

Thanks! :)
Holger

[Ken Russell (switch to Gerrit), 2015-04-10 02:02:53]

kbr@chromium.org changed reviewers:
+ aelias@chromium.org, esprehn@chromium.org, jchaffraix@chromium.org

[Ken Russell (switch to Gerrit), 2015-04-10 02:02:54]

I'm not a good reviewer for this change. CC'ing a few others who may be able to
help.

[aelias_OOO_until_Jul13, 2015-04-10 02:40:41]

Looks like the linked bug refers to an float epsilon error.  In general, I think
0.5-based rounding is too big of a hammer for this kind of bug.

The best option would be to fix the math in the code that originated the
epsilon-off floating value to eliminate.  Do you know where it comes from?

If we determine the math isn't viable to change, then I generally prefer a
minimal rounding that only rounds up if within a tiny epsilon of the integer
(like 1e-5 ish).

[aelias_OOO_until_Jul13, 2015-04-10 02:44:00]

aelias@chromium.org changed reviewers:
- jchaffraix@chromium.org, kbr@chromium.org

[aelias_OOO_until_Jul13, 2015-04-10 02:44:11]

aelias@chromium.org changed reviewers:
- esprehn@chromium.org

[kraush, 2015-04-10 15:51:57]

On 2015/04/10 02:40:41, aelias wrote:
> Looks like the linked bug refers to an float epsilon error.  In general, I
think
> 0.5-based rounding is too big of a hammer for this kind of bug.
> 
> The best option would be to fix the math in the code that originated the
> epsilon-off floating value to eliminate.  Do you know where it comes from?
> 
> If we determine the math isn't viable to change, then I generally prefer a
> minimal rounding that only rounds up if within a tiny epsilon of the integer
> (like 1e-5 ish).

Hi aelias,

Thanks a lot for your feedback! :)
Unfortunately I don't think we can fix the underlying bug.
The default zoom level is basically determined by calculating 1.2^(log(1.0) /
log(1.2))
This should theoretically return 1.0, but in the libc implementation of Android
4.4.2 and below it returns the slightly lower double. (Works fine in 4.4.3 and
above)

Regarding the general concept of rounding though: Can you elaborate on why we
shouldn't round here? (e.g. by adding .5 and truncating, which should be natural
rounding)
It seems width is already rounded, only height isn't.
If you look at the test I added, it shows exactly that fact, because width with
the current implementation results in 100 while height results in 99 (and thus
breaks the test)
In case we add something like 1e-5, height will still return 99 in this new test
case while width will return 100.
Can you comment on whether or not this is intended behavior?
Should width also be truncated rather than rounded?

Thanks!
Holger

[esprehn, 2015-04-11 02:11:48]

What changed between Android 4.4.2 and 4.4.3 ?

[kraush, 2015-04-11 02:58:58]

On 2015/04/11 02:11:48, esprehn wrote:
> What changed between Android 4.4.2 and 4.4.3 ?

The default zoom level seems to have changed between 4.4.2 and 4.4.3
It is calculated using 1.2^(log(1.0) / log(1.2))
We have tested this on a Nexus 7 on 4.4.3 and another Android device running
Lollipop and it returned exactly 1.0 (a double with the Hex value
3FF0000000000000)
However on a HTC One on 4.4.2 as well as the same device we tried with Lollipop
but this time running 4.4.2, a value slightly below 1.0 was returned (double
with the hex value 3FEFFFFFFFFFFFFE)
The assumption is that this is due to a libc change in the platform.
On most devices that poses no problem as the CPU will round this up during
multiplication, but some other processors like the one used by Kindle Fire HDX
7" don't.

But that aside, I'd like to mention again that even if this small double
deviation would not exist, rounding instead of truncating seems to be the
correct behavior here to align height with width (which is already rounded)
At least that is the way I understood it. Please let me know if they are not
supposed to behave the same or if both should be truncated instead.

[aelias_OOO_until_Jul13, 2015-04-13 03:12:52]

Hmm, but Chromium has no plans to ship new WebViews on <5.0, so if the bug is
exclusive to those older versions, it doesn't seem any change is needed here. 
If this change is meant to support a backported WebView project you're working
on, I'd recommend keeping the change as a diff in your private branch.

[kraush, 2015-04-13 16:11:21]

On 2015/04/13 03:12:52, aelias wrote:
> Hmm, but Chromium has no plans to ship new WebViews on <5.0, so if the bug is
> exclusive to those older versions, it doesn't seem any change is needed here. 
> If this change is meant to support a backported WebView project you're working
> on, I'd recommend keeping the change as a diff in your private branch.

Good point!
Since WebViews are not used before 5.0, there is little point in fixing the
double rounding issue due to incorrect zoom levels (which never occur in 5.0+).
However I'm still convinced that we should address the issue that width is
rounded while height is truncated.
Should I close the linked bug and open a new one?
If so, can I still reuse this CL for it? (The change and test would remain the
same, just the description and linked bug should be different)

Thanks,
Holger

[aelias_OOO_until_Jul13, 2015-04-15 18:08:50]

Can you clarify what you're hoping to achieve with that?  Is it just on
principle for consistency between width and height or is there some other issue?

I agree in general they should be consistent, but I haven't made up my mind on
the best thing to converge to.  Flooring, ceiling and rounding all are
questionable in their own way.  Right now I'm leaning towards making them both
flooring.

[kraush, 2015-04-15 20:31:55]

On 2015/04/15 18:08:50, aelias wrote:
> Can you clarify what you're hoping to achieve with that?  Is it just on
> principle for consistency between width and height or is there some other
issue?
> 
> I agree in general they should be consistent, but I haven't made up my mind on
> the best thing to converge to.  Flooring, ceiling and rounding all are
> questionable in their own way.  Right now I'm leaning towards making them both
> flooring.

On 2015/04/15 18:08:50, aelias wrote:
> Can you clarify what you're hoping to achieve with that?  Is it just on
> principle for consistency between width and height or is there some other
issue?
> 
> I agree in general they should be consistent, but I haven't made up my mind on
> the best thing to converge to.  Flooring, ceiling and rounding all are
> questionable in their own way.  Right now I'm leaning towards making them both
> flooring.

Consistency is definitely one of the main reasons.
However since this also breaks a test on certain devices as outlined in the
linked bug, I'd also consider fixing that failure an important reason.
One way to do so would be to only run that test on a certain API-Level of
Android. (Currently anyone running webkit_unittests on specific 4.4.2 or lower
devices can run into the issue.)
While that would solve the problem, I'd still rather address the underlying
issue of inconsistent height and width. That might not be a problem when we are
talking about large dimensions, but particularly with small elements a single
pixel might definitely impact the user experience. (10x10 and 10x9 might be
quite visible differences)

In case we go forward with flooring, we should still have the test only run on
API level 21 and above.

Do you have any information on why rounding was chosen for height?
As far as I can tell from the file's history, flooring was used at some point
and later it was changed to rounding instead.

[aelias_OOO_until_Jul13, 2015-04-16 15:48:14]

aelias@chromium.org changed reviewers:
+ leviw@chromium.org

[aelias_OOO_until_Jul13, 2015-04-16 15:51:28]

On 2015/04/15 at 20:31:55, kraush wrote:
> However since this also breaks a test on certain devices as outlined in the
linked bug, I'd also consider fixing that failure an important reason.
> One way to do so would be to only run that test on a certain API-Level of
Android. (Currently anyone running webkit_unittests on specific 4.4.2 or lower
devices can run into the issue.)

Hmm, it's unexpected that webkit_unittests would behave differently based on
Android version.  Those tests are generally supposed to be isolated from the
operating system.  I don't think Chromium uses the system libc either.  There
might be something poorly insulated in the unit test package that could be fixed
there to avoid the dependency, if you can find it.

> Do you have any information on why rounding was chosen for height?
> As far as I can tell from the file's history, flooring was used at some point
and later it was changed to rounding instead.

Looks like that was just changed last week in
https://codereview.chromium.org/1061153002 .  Levi, is there a reason why you
updated the width and not the height?

[leviw_travelin_and_unemployed, 2015-05-11 03:14:46]

On 2015/04/16 at 15:51:28, aelias wrote:
> On 2015/04/15 at 20:31:55, kraush wrote:
> > However since this also breaks a test on certain devices as outlined in the
linked bug, I'd also consider fixing that failure an important reason.
> > One way to do so would be to only run that test on a certain API-Level of
Android. (Currently anyone running webkit_unittests on specific 4.4.2 or lower
devices can run into the issue.)
> 
> Hmm, it's unexpected that webkit_unittests would behave differently based on
Android version.  Those tests are generally supposed to be isolated from the
operating system.  I don't think Chromium uses the system libc either.  There
might be something poorly insulated in the unit test package that could be fixed
there to avoid the dependency, if you can find it.
> 
> > Do you have any information on why rounding was chosen for height?
> > As far as I can tell from the file's history, flooring was used at some
point and later it was changed to rounding instead.
> 
> Looks like that was just changed last week in
https://codereview.chromium.org/1061153002 .  Levi, is there a reason why you
updated the width and not the height?

Apologies for missing this thread for so long! Thanks for pinging me, Holger :)

I got into this space because our DOM api was wonky and I wanted to fix it. When
it came to the WebViewImpl, my only goal was to make the smallest change
possible to help a regression paulmeyer@ were dealing with and then run away
from WebViewImpl again.

Rounding should be the proper method in general, as ultimately we round the maxX
and maxY values to get the rendered sizes of a page, and the width of a view is
it's maxX. These values should be consistent.

[kraush, 2015-05-11 16:04:13]

On 2015/05/11 03:14:46, leviw wrote:
> On 2015/04/16 at 15:51:28, aelias wrote:
> > On 2015/04/15 at 20:31:55, kraush wrote:
> > > However since this also breaks a test on certain devices as outlined in
the
> linked bug, I'd also consider fixing that failure an important reason.
> > > One way to do so would be to only run that test on a certain API-Level of
> Android. (Currently anyone running webkit_unittests on specific 4.4.2 or lower
> devices can run into the issue.)
> > 
> > Hmm, it's unexpected that webkit_unittests would behave differently based on
> Android version.  Those tests are generally supposed to be isolated from the
> operating system.  I don't think Chromium uses the system libc either.  There
> might be something poorly insulated in the unit test package that could be
fixed
> there to avoid the dependency, if you can find it.
> > 
> > > Do you have any information on why rounding was chosen for height?
> > > As far as I can tell from the file's history, flooring was used at some
> point and later it was changed to rounding instead.
> > 
> > Looks like that was just changed last week in
> https://codereview.chromium.org/1061153002 .  Levi, is there a reason why you
> updated the width and not the height?
> 
> Apologies for missing this thread for so long! Thanks for pinging me, Holger
:)
> 
> I got into this space because our DOM api was wonky and I wanted to fix it.
When
> it came to the WebViewImpl, my only goal was to make the smallest change
> possible to help a regression paulmeyer@ were dealing with and then run away
> from WebViewImpl again.
> 
> Rounding should be the proper method in general, as ultimately we round the
maxX
> and maxY values to get the rendered sizes of a page, and the width of a view
is
> it's maxX. These values should be consistent.

Thanks for the reply Levi!
If rounding is the right approach, would you consider the solution proposed in
this change to be okay?
I'm not aware of any central rounding utility used in Chrome and as far as I
understand std::round should currently not be used either.
Adding .5 and casting seemed like the most straight forward way, but I'm
definitely open to other approaches for rounding.

Thanks,
Holger

[leviw_travelin_and_unemployed, 2015-05-13 00:26:30]

The CQ bit was checked by leviw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-13 00:26:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1071383002/1

[commit-bot: I haz the power, 2015-05-13 01:49:53]

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

[commit-bot: I haz the power, 2015-05-13 01:49:54]

Dry run: This issue passed the CQ dry run.

[kraush, 2015-05-20 21:50:47]

On 2015/05/13 01:49:54, I haz the power (commit-bot) wrote:
> Dry run: This issue passed the CQ dry run.

Thanks for triggering the dry run! :)
With the dry run being passed and rounding being the right way to go, would it
be okay to merge as is?
Or is there anything that needs to be changed?

[leviw_travelin_and_unemployed, 2015-05-20 22:05:06]

Code change seems fine. Can we get some more coverage?

https://codereview.chromium.org/1071383002/diff/1/Source/web/tests/WebViewTes...
File Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/1071383002/diff/1/Source/web/tests/WebViewTes...
Source/web/tests/WebViewTest.cpp:2353:
webView->setZoomLevel(WebView::zoomFactorToZoomLevel(0.9995));
Can we get some more test cases? 1.0005 maybe?

[leviw_travelin_and_unemployed, 2015-05-21 18:39:02]

Ping :)

[kraush, 2015-05-21 20:56:46]

On 2015/05/21 18:39:02, leviw wrote:
> Ping :)

Added a test that rounds down as suggested (using 1.005)
Do we need any more coverage?

I was thinking about really tiny (smallest bit) deviations from 1.0, but don't
think that would catch anything 0.995 or 1.005 wouldn't.
Any other suggestions for test cases?

[leviw_travelin_and_unemployed, 2015-05-21 21:11:51]

On 2015/05/21 at 20:56:46, kraush wrote:
> On 2015/05/21 18:39:02, leviw wrote:
> > Ping :)
> 
> Added a test that rounds down as suggested (using 1.005)
> Do we need any more coverage?
> 
> I was thinking about really tiny (smallest bit) deviations from 1.0, but don't
think that would catch anything 0.995 or 1.005 wouldn't.
> Any other suggestions for test cases?

It verifies that we're rounding, not ceiling or flooring.

LGTM.

[aelias_OOO_until_Jul13, 2015-06-04 19:04:49]

lgtm

[kraush, 2015-06-04 20:13:56]

The CQ bit was checked by kraush@amazon.com

[commit-bot: I haz the power, 2015-06-04 20:14:26]

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

[commit-bot: I haz the power, 2015-06-04 21:22:07]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196530