Adding the page and DPI scale adjustment for Autofill Popups.

Adding the page and DPI scale adjustment for Autofill Popups.

Switching from using Rect to RectF in Autofill. This is to ensure that no
round of values are lost when DPI scale is applied. These changes were required
after the coordinate system change that came with Impl-side painting.

BUG=172409
TEST=Get an Autofill popup to show up on Chrome Android

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180913

[aurimas (slooooooooow), 2013-02-04 22:17:49]

Hey Ilya,

Could you take a look at this CL?

Thanks!
Aurimas

[Ilya Sherman, 2013-02-04 23:23:27]

https://codereview.chromium.org/12188020/diff/1/chrome/android/java/src/org/c...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopup.java
(right):

https://codereview.chromium.org/12188020/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopup.java:155:
mAnchorRect = new Rect((int) Math.round(x * scale), (int) Math.round(y * scale),
nit: Do you still need the type casts to int now that you're calling
Math.round()?

https://codereview.chromium.org/12188020/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):

https://codereview.chromium.org/12188020/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:558: gfx::Point
top_left_corner_of_popup = round(element_bounds().origin()) +
Rounding a gfx::Point doesn't compile.

https://codereview.chromium.org/12188020/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:565: gfx::Point
bottom_right_corner_of_popup = round(element_bounds().origin()) +
Ditto.

https://codereview.chromium.org/12188020/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:639: int
bottom_growth_start = std::max(topmost_display_y,
nit: Since you're now wrapping one of the arguments to the next line, please
wrap both.

[aurimas (slooooooooow), 2013-02-04 23:56:26]

https://codereview.chromium.org/12188020/diff/1/chrome/android/java/src/org/c...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopup.java
(right):

https://codereview.chromium.org/12188020/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopup.java:155:
mAnchorRect = new Rect((int) Math.round(x * scale), (int) Math.round(y * scale),
On 2013/02/04 23:23:28, Ilya Sherman wrote:
> nit: Do you still need the type casts to int now that you're calling
> Math.round()?

It was returning a long since the input was Double. I will case scale to float
instead so I don't need the extra casts.

[Ilya Sherman, 2013-02-05 02:27:53]

LGTM with nits addressed and green bots.  Thanks.

https://chromiumcodereview.appspot.com/12188020/diff/8002/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_popup_controller.h (right):

https://chromiumcodereview.appspot.com/12188020/diff/8002/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_popup_controller.h:66: virtual const
gfx::Rect rounded_element_bounds() const = 0;
I think this can be a private method on AutofillPopupControllerImpl, rather than
exposing it as part of the public API.

https://chromiumcodereview.appspot.com/12188020/diff/8002/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12188020/diff/8002/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:295: const
gfx::Rect AutofillPopupControllerImpl::rounded_element_bounds() const {
nit: This method is not a trivial member variable access, so it should be named
RoundedElementBounds() rather than using hacker_case.

https://chromiumcodereview.appspot.com/12188020/diff/8002/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right):

https://chromiumcodereview.appspot.com/12188020/diff/8002/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:283: gfx::Rect
bounds(0, 0, 1, 2);
This should be a gfx::RectF, right?

[aurimas (slooooooooow), 2013-02-06 00:05:27]

https://chromiumcodereview.appspot.com/12188020/diff/8002/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12188020/diff/8002/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:295: const
gfx::Rect AutofillPopupControllerImpl::rounded_element_bounds() const {
On 2013/02/05 02:27:53, Ilya Sherman wrote:
> nit: This method is not a trivial member variable access, so it should be
named
> RoundedElementBounds() rather than using hacker_case.

Done.

https://chromiumcodereview.appspot.com/12188020/diff/8002/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right):

https://chromiumcodereview.appspot.com/12188020/diff/8002/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:283: gfx::Rect
bounds(0, 0, 1, 2);
On 2013/02/05 02:27:53, Ilya Sherman wrote:
> This should be a gfx::RectF, right?

I only changed for element_bounds to use RectF. Popup still uses Rect as popups
can only really be displayed in whole pixel coordinates.

[Ilya Sherman, 2013-02-06 00:07:40]

https://chromiumcodereview.appspot.com/12188020/diff/8002/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right):

https://chromiumcodereview.appspot.com/12188020/diff/8002/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:283: gfx::Rect
bounds(0, 0, 1, 2);
On 2013/02/06 00:05:27, aurimas wrote:
> On 2013/02/05 02:27:53, Ilya Sherman wrote:
> > This should be a gfx::RectF, right?
> 
> I only changed for element_bounds to use RectF. Popup still uses Rect as
popups
> can only really be displayed in whole pixel coordinates.

These bounds *are* element bounds, and you updated GetOrCreate() to take in a
RectF.  What am I missing?

[aurimas (slooooooooow), 2013-02-06 00:12:42]

Hey Chris,

Could you take a look at chrome/common/autofill_messages.h changes?

Thanks,
Aurimas

[aurimas (slooooooooow), 2013-02-06 00:13:32]

Hey Ted,

Could you take a look at chrome/android changes?

Thanks,
Aurimas

[Ted C, 2013-02-06 00:35:24]

lgtm w/ nits

https://chromiumcodereview.appspot.com/12188020/diff/23015/chrome/browser/aut...
File chrome/browser/autofill/test_autofill_external_delegate.cc (right):

https://chromiumcodereview.appspot.com/12188020/diff/23015/chrome/browser/aut...
chrome/browser/autofill/test_autofill_external_delegate.cc:18: gfx::RectF
bounds(100, 100);
100.f, 100.f

https://chromiumcodereview.appspot.com/12188020/diff/23015/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right):

https://chromiumcodereview.appspot.com/12188020/diff/23015/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:283: gfx::RectF
bounds(0, 0, 1, 2);
0.f, etc...

[palmer, 2013-02-06 00:35:39]

lgtm

[aurimas (slooooooooow), 2013-02-06 00:50:45]

https://chromiumcodereview.appspot.com/12188020/diff/23015/chrome/browser/aut...
File chrome/browser/autofill/test_autofill_external_delegate.cc (right):

https://chromiumcodereview.appspot.com/12188020/diff/23015/chrome/browser/aut...
chrome/browser/autofill/test_autofill_external_delegate.cc:18: gfx::RectF
bounds(100, 100);
On 2013/02/06 00:35:24, Ted C wrote:
> 100.f, 100.f

Done.

https://chromiumcodereview.appspot.com/12188020/diff/23015/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right):

https://chromiumcodereview.appspot.com/12188020/diff/23015/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:283: gfx::RectF
bounds(0, 0, 1, 2);
On 2013/02/06 00:35:24, Ted C wrote:
> 0.f, etc...

Done.

[commit-bot: I haz the power, 2013-02-06 00:51:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/12188020/15024

[commit-bot: I haz the power, 2013-02-06 04:54:57]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-02-06 05:28:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/12188020/15024

[commit-bot: I haz the power, 2013-02-06 07:02:08]

Change committed as 180913