[Element / Autofill] Add boundsInViewportFloat() to fix <input> popup misalignment.

[Element / Autofill] Add boundsInViewportFloat() to fix <input> popup misalignment.

For <input> elements with fractional x value, the autocomplete popup
location is mismatched with <input> element. To fix this, we:
1. Add Element::boundsInViewportFloat() to compute a more precise
  bounds for the <input> element, to be passed to popup rendering.
2. Fix popup rendering to round the received bounds in a way that's
  consistent with <input> element's location.

BUG=529867

[huangs, 2015-12-10 00:41:56]

Description was changed from

==========
[Element / Autofill] Add boundsInViewportFloat(), use it to fix <input>
Autocomplete Popup alignment.

For <input> elements with fractional x value, the autocomplete popup
location is mismatched with <input> element. To fix this, we:
1. Add Element::boundsInViewportFloat() to compute a more precise
  bounds for the <input> element, to be passed to popup rendering.
2. Fix popup rendering to round the received bounds in a way that's
  consistent with <input> element's location.

BUG=529867
==========

to

==========
[Element / Autofill] Add boundsInViewportFloat() to fix <input> popup
misalignment.

For <input> elements with fractional x value, the autocomplete popup
location is mismatched with <input> element. To fix this, we:
1. Add Element::boundsInViewportFloat() to compute a more precise
  bounds for the <input> element, to be passed to popup rendering.
2. Fix popup rendering to round the received bounds in a way that's
  consistent with <input> element's location.

BUG=529867
==========

[huangs, 2015-12-10 00:44:26]

huangs@chromium.org changed reviewers:
+ oshima@chromium.org

[huangs, 2015-12-10 00:46:06]

This is currently a proof-of-concept for the main bug.  I'd like to first know
if the FloatRect piping code is a viable.  Or perhaps we should refactor Blink's
coordinate conversion first, before fixing the bug?  Please take a quick look. 
Thanks!

[bokan, 2015-12-10 14:48:34]

On 2015/12/10 00:46:06, huangs wrote:
> This is currently a proof-of-concept for the main bug.  I'd like to first know
> if the FloatRect piping code is a viable.  Or perhaps we should refactor
Blink's
> coordinate conversion first, before fixing the bug?  Please take a quick look.

> Thanks!

What refactor did you have in mind?

[bokan, 2015-12-10 14:48:39]

bokan@chromium.org changed reviewers:
+ bokan@chromium.org

[bokan, 2015-12-10 14:48:40]

https://codereview.chromium.org/1516723003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1516723003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.cpp:1050: FloatRect
Element::boundsInViewportFloat()
Perhaps we should just make boundsInViewport return a FloatRect, rather then add
a second method, and let call sites truncate to ints

[huangs, 2015-12-10 16:15:40]

On 2015/12/10 14:48:40, bokan wrote:
>
https://codereview.chromium.org/1516723003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/Element.cpp (right):
> 
>
https://codereview.chromium.org/1516723003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/Element.cpp:1050: FloatRect
> Element::boundsInViewportFloat()
> Perhaps we should just make boundsInViewport return a FloatRect, rather then
add
> a second method, and let call sites truncate to ints

Would this impact performance significantly?  I also plan to split the CL to 2
parts:
0. Refactor Widget.h etc., if that's called for.
1. Add boundsInViewportFloat(), also renaming boundsInViewport() =>
boundsInViewportInt() to locate the callers.
2. Update callers, possibly rename boundsInViewportFloat() =>
boundsInViewport(), and delete the Int version.

[huangs, 2015-12-10 16:16:14]

https://codereview.chromium.org/1516723003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1516723003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.cpp:1050: FloatRect
Element::boundsInViewportFloat()
On 2015/12/10 14:48:39, bokan wrote:
> Perhaps we should just make boundsInViewport return a FloatRect, rather then
add
> a second method, and let call sites truncate to ints

Replied in main comment.

[bokan, 2015-12-10 17:12:39]

boundsInViewport already truncates from floats to ints. If we just returned the
float version the call sites could decide if they need that truncation. At
worst, the performance would be the same

[oshima, 2015-12-10 18:13:03]

On 2015/12/10 17:12:39, bokan wrote:
> boundsInViewport already truncates from floats to ints. If we just returned
the
> float version the call sites could decide if they need that truncation. At
> worst, the performance would be the same

+1

[huangs, 2015-12-10 21:19:30]

In principle this works, since unite() and enclosingBoundingBox() commute.  The
problem though is that the "enclosing" code operates on FloatRect/IntRect which
are not in general available at caller's level; the caller only can access
WebFloatRect/WebRect, which are missing the "enclosing" feature (seem to be
purposeful for simplicity?).

I'd prefer to keep the boundsInViewportInt() and boundsInViewportFloat()
interfaces separate -- though internally they'd share boundsInViewportFloat().

[huangs, 2015-12-10 21:54:54]

Updated.  Please note that this CL is only a prototype.  I'm going to go ahead
and do some refactoring first:

1. Implement oshima@'s suggestion to make convertViewportToWindow() use float,
and update all callers.
2. Add boundsInViewportFloat(), Rename boundsInViewport() -->
boundsInViewportInt() and make it call the Float() version, mechanical rename
for callers.
3. (Somehow) deal with coordinate conversion stuff in Widget.h.

Then we'll worry about the other stuff.