Use the window coordinate for the autofill bounds.

Use the window coordinate for the autofill bounds.

Viewport and Window coordinates will differ when Zoom is used to implement DSF behavior.

BUG=485650
TEST=RenderViewImplScaleFactorTest.*, plus manually tested.

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

[oshima, 2015-11-18 19:53:41]

Description was changed from

==========
Convert Input To Viewport in InputRouter

patch from issue 1440923002 at patchset 100001
(http://crrev.com/1440923002#ps100001)

BUG=
==========

to

==========
Autofill

BUG=
==========

[oshima, 2015-12-03 04:47:36]

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

[commit-bot: I haz the power, 2015-12-03 04:48:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1455143004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1455143004/80001

[commit-bot: I haz the power, 2015-12-03 04:58:41]

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

[commit-bot: I haz the power, 2015-12-03 04:58:42]

Dry run: Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[oshima, 2015-12-03 08:10:52]

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

[oshima, 2015-12-03 08:11:01]

Patchset #1 (id:1) has been deleted

[oshima, 2015-12-03 08:11:12]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2015-12-03 08:12:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1455143004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1455143004/140001

[oshima, 2015-12-03 08:13:17]

Patchset #5 (id:120001) has been deleted

[oshima, 2015-12-03 08:13:24]

Patchset #3 (id:80001) has been deleted

[oshima, 2015-12-03 08:13:26]

Patchset #2 (id:60001) has been deleted

[oshima, 2015-12-03 08:13:30]

Patchset #1 (id:40001) has been deleted

[oshima, 2015-12-03 08:13:40]

Patchset #1 (id:100001) has been deleted

[commit-bot: I haz the power, 2015-12-03 09:20:56]

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

[commit-bot: I haz the power, 2015-12-03 09:20:57]

Dry run: This issue passed the CQ dry run.

[oshima, 2015-12-03 19:04:57]

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

[commit-bot: I haz the power, 2015-12-03 19:05:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1455143004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1455143004/160001

[oshima, 2015-12-03 19:07:41]

Description was changed from

==========
Autofill

BUG=
==========

to

==========
Use the window coordinate for the autofill bounds.

Viewport and Window coordinates will differ when Zoom is used to implement DSF
behavior.

BUG=485650
TEST=RenderViewImplScaleFactorTest.*, plus manually tested.
==========

[oshima, 2015-12-03 19:07:41]

oshima@chromium.org changed reviewers:
+ bokan@chromium.org, vabr@chromium.org

[oshima, 2015-12-03 19:10:48]

bokan -> please review all as this involves coordinate change.
vabr -> components/autofill owner

[oshima, 2015-12-03 19:16:59]

Patchset #1 (id:140001) has been deleted

[vabr (Chromium), 2015-12-03 19:29:02]

components/autofill owner LGTM

When you say "manually tested", does that involve what I was asking for in
https://codereview.chromium.org/1463723003#msg16 ?

Cheers,
Vaclav

[oshima, 2015-12-03 19:31:59]

On 2015/12/03 19:29:02, vabr (Chromium) wrote:
> components/autofill owner LGTM
> 
> When you say "manually tested", does that involve what I was asking for in
> https://codereview.chromium.org/1463723003#msg16 ?
> 
> Cheers,
> Vaclav

Yes, I tested using that site.

[bokan, 2015-12-03 20:49:20]

https://codereview.chromium.org/1455143004/diff/160001/content/public/rendere...
File content/public/renderer/render_view.h (right):

https://codereview.chromium.org/1455143004/diff/160001/content/public/rendere...
content/public/renderer/render_view.h:127: // Converts the |rect| from viewport
coordinates to window coordinates.
Could you add a bit of description here (or elsewhere if it makes more sense) on
what |viewport| and what |window| coordinates are?

https://codereview.chromium.org/1455143004/diff/160001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1455143004/diff/160001/content/renderer/rende...
content/renderer/render_view_impl.cc:1946:
gfx::Rect(element.boundsInViewport()), 1 / device_scale_factor_);
If you change convertViewportToWindow to return a gfx::Rect you can use it here
too.

https://codereview.chromium.org/1455143004/diff/160001/content/renderer/rende...
content/renderer/render_view_impl.cc:2144: void
RenderViewImpl::convertViewportToWindow(blink::WebRect* rect) {
Why not just make this return a gfx::Rect since Window coordinates are consumed
by the renderer which uses gfx::Rect (like in all your call-sites in this
patch)?

[oshima, 2015-12-03 21:58:17]

https://codereview.chromium.org/1455143004/diff/160001/content/public/rendere...
File content/public/renderer/render_view.h (right):

https://codereview.chromium.org/1455143004/diff/160001/content/public/rendere...
content/public/renderer/render_view.h:127: // Converts the |rect| from viewport
coordinates to window coordinates.
On 2015/12/03 20:49:20, bokan wrote:
> Could you add a bit of description here (or elsewhere if it makes more sense)
on
> what |viewport| and what |window| coordinates are?

I updated here for now, but I think we should update the chromium doc and point
to that instead once migration is completed.

https://codereview.chromium.org/1455143004/diff/160001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1455143004/diff/160001/content/renderer/rende...
content/renderer/render_view_impl.cc:1946:
gfx::Rect(element.boundsInViewport()), 1 / device_scale_factor_);
On 2015/12/03 20:49:20, bokan wrote:
> If you change convertViewportToWindow to return a gfx::Rect you can use it
here
> too.

I was going to change this during cleanup but forgot to do so. Thank you for
pointing out. Done.

https://codereview.chromium.org/1455143004/diff/160001/content/renderer/rende...
content/renderer/render_view_impl.cc:2144: void
RenderViewImpl::convertViewportToWindow(blink::WebRect* rect) {
On 2015/12/03 20:49:20, bokan wrote:
> Why not just make this return a gfx::Rect since Window coordinates are
consumed
> by the renderer which uses gfx::Rect (like in all your call-sites in this
> patch)?

This was a part of larger CL, and Blink needs this for popup. See
https://codereview.chromium.org/1456753002/.

Is it ok to use gfx::Rect in WebWidgetClient? If not, we'll end up having two
almost identical but have slightly different signature on the same class.

I used out parameter because it may not have to update anything, but I'm happy
to return WebRect (or gfx::Rect). Please let me know.

[oshima, 2015-12-03 22:57:01]

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

[commit-bot: I haz the power, 2015-12-03 22:57:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1455143004/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1455143004/180001

[commit-bot: I haz the power, 2015-12-03 23:17:06]

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

[commit-bot: I haz the power, 2015-12-03 23:17:07]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bokan, 2015-12-04 00:28:52]

lgtm

https://codereview.chromium.org/1455143004/diff/160001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1455143004/diff/160001/content/renderer/rende...
content/renderer/render_view_impl.cc:2144: void
RenderViewImpl::convertViewportToWindow(blink::WebRect* rect) {
On 2015/12/03 21:58:17, oshima wrote:
> On 2015/12/03 20:49:20, bokan wrote:
> > Why not just make this return a gfx::Rect since Window coordinates are
> consumed
> > by the renderer which uses gfx::Rect (like in all your call-sites in this
> > patch)?
> 
> This was a part of larger CL, and Blink needs this for popup. See
> https://codereview.chromium.org/1456753002/.
> 
> Is it ok to use gfx::Rect in WebWidgetClient? If not, we'll end up having two
> almost identical but have slightly different signature on the same class.
> 
> I used out parameter because it may not have to update anything, but I'm happy
> to return WebRect (or gfx::Rect). Please let me know.

Ah, I see. Yah, right now I think gfx::Rect is still disallowed in Source/web.
But with the repo merge I think it's something we should eliminate (as it is
now, WebRect is an extra intermediate step for no reason). I'll propose the
change on blink-dev. In the mean time, no need to block on that, lgtm as is and
we (I can do this) can change it later.

[oshima, 2015-12-04 01:01:01]

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

[oshima, 2015-12-04 01:01:01]

piman@ -> content owner

[oshima, 2015-12-04 01:01:07]

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

[commit-bot: I haz the power, 2015-12-04 01:05:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1455143004/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1455143004/200001

[commit-bot: I haz the power, 2015-12-04 03:11:40]

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

[commit-bot: I haz the power, 2015-12-04 03:11:41]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  android_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[piman, 2015-12-04 03:25:00]

lgtm

[oshima, 2015-12-04 04:19:51]

The CQ bit was checked by oshima@chromium.org

[oshima, 2015-12-04 04:19:51]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/1455143004/#ps200001
(title: "fix typo")

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

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

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

Description was changed from

==========
Use the window coordinate for the autofill bounds.

Viewport and Window coordinates will differ when Zoom is used to implement DSF
behavior.

BUG=485650
TEST=RenderViewImplScaleFactorTest.*, plus manually tested.
==========

to

==========
Use the window coordinate for the autofill bounds.

Viewport and Window coordinates will differ when Zoom is used to implement DSF
behavior.

BUG=485650
TEST=RenderViewImplScaleFactorTest.*, plus manually tested.
==========

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

Committed patchset #3 (id:200001)

[commit-bot: I haz the power, 2015-12-04 04:28:27]

Description was changed from

==========
Use the window coordinate for the autofill bounds.

Viewport and Window coordinates will differ when Zoom is used to implement DSF
behavior.

BUG=485650
TEST=RenderViewImplScaleFactorTest.*, plus manually tested.
==========

to

==========
Use the window coordinate for the autofill bounds.

Viewport and Window coordinates will differ when Zoom is used to implement DSF
behavior.

BUG=485650
TEST=RenderViewImplScaleFactorTest.*, plus manually tested.

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

[commit-bot: I haz the power, 2015-12-04 04:28:29]

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