Ensure that mouse events on Windows reposted by the menu_controller are converted to pixel.

Ensure that mouse events on Windows reposted by the menu_controller are converted to pixel.

On HiDPI windows these coordinates are in DIPs. While posting them via the PostMessage API we
need to convert them back to pixel using the correct scale factor.

This caused bookmarks to get incorrectly saved at times.

Changes in this patch are as below:-
1. menu_controller.cc. Convert screen_loc to pixels before   invoking Win32 API's as they expect values in pixels.

2. screen_win.cc. Fix the GetWindowAtScreenPoint function   to convert the point to pixels before invoking  
   the WindowFromPoint API.

BUG=381605
R=sky

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

[sky, 2014-06-18 13:15:59]

https://codereview.chromium.org/341763002/diff/40001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/341763002/diff/40001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller.cc:2053: gfx::NativeWindow window =
screen->GetWindowAtScreenPoint(screen_loc);
Move this so that we only call GetWindowAtScreenPoint once. Also, does your code
do the right thing in metro mode?

[ananta, 2014-06-18 18:53:41]

https://codereview.chromium.org/341763002/diff/40001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/341763002/diff/40001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller.cc:2053: gfx::NativeWindow window =
screen->GetWindowAtScreenPoint(screen_loc);
On 2014/06/18 13:15:59, sky wrote:
> Move this so that we only call GetWindowAtScreenPoint once. Also, does your
code
> do the right thing in metro mode?

Added a function PlatformGetWindowAtLocation in the anonymous namespace. This
function returns the window and the screen location in the correct scale.

It works correctly on metro.

[sky, 2014-06-18 22:20:26]

+oshima

https://codereview.chromium.org/341763002/diff/60001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/341763002/diff/60001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller.cc:109: *window =
screen->GetWindowAtScreenPoint(screen_loc);
Something seems wrong here. Why does GetWindowAtScreenPoint want pixel points?
It seems wrong that ConvertPointToScreen returns DIPs yet screen functions want
pixels. Is that right?

[oshima, 2014-06-18 22:27:51]

https://codereview.chromium.org/341763002/diff/60001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/341763002/diff/60001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller.cc:109: *window =
screen->GetWindowAtScreenPoint(screen_loc);
On 2014/06/18 22:20:26, sky wrote:
> Something seems wrong here. Why does GetWindowAtScreenPoint want pixel points?
> It seems wrong that ConvertPointToScreen returns DIPs yet screen functions
want
> pixels. Is that right?

Yes, both must be in DIP.

[ananta, 2014-06-18 22:57:11]

https://codereview.chromium.org/341763002/diff/60001/ui/views/controls/menu/m...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/341763002/diff/60001/ui/views/controls/menu/m...
ui/views/controls/menu/menu_controller.cc:109: *window =
screen->GetWindowAtScreenPoint(screen_loc);
On 2014/06/18 22:20:26, sky wrote:
> Something seems wrong here. Why does GetWindowAtScreenPoint want pixel points?
> It seems wrong that ConvertPointToScreen returns DIPs yet screen functions
want
> pixels. Is that right?

I updated the patch to fix ScreenWin::GetWindowAtScreenPoint to convert to
pixels
before calling WindowFromPoint. The other change is in menu_controller.cc to
convert screen_loc to pixels in the #if defined(OS_WIN) block in RepostEvent.
Added comments as to why that is needed.

PTAL.

[sky, 2014-06-19 00:55:12]

Did you look into other call sites to make sure the screen_win change isn't
going to bite anyone?

https://codereview.chromium.org/341763002/diff/100001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/341763002/diff/100001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:2058: screen_loc =
gfx::win::DIPToScreenPoint(screen_loc);
Don't reuse screen_loc here, it's too easy to get wrong. Maybe name this
screen_loc_pixels or something and move into if block as it seems like that is
the only place that needs it.

[ananta, 2014-06-19 01:44:18]

I looked over the usage of GetWindowAtScreenPoint in the code base. The
ScreenWin change should not break anything. In any case what the code is doing
today is wrong.

https://codereview.chromium.org/341763002/diff/100001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/341763002/diff/100001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:2058: screen_loc =
gfx::win::DIPToScreenPoint(screen_loc);
On 2014/06/19 00:55:12, sky wrote:
> Don't reuse screen_loc here, it's too easy to get wrong. Maybe name this
> screen_loc_pixels or something and move into if block as it seems like that is
> the only place that needs it.

Done.

[sky, 2014-06-19 15:33:32]

LGTM

https://codereview.chromium.org/341763002/diff/120001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/341763002/diff/120001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:2058: gfx::Point screen_loc_pixels =
gfx::win::DIPToScreenPoint(screen_loc);
nit: move into conditional block below, it's only needed there.

[ananta, 2014-06-19 18:45:26]

https://codereview.chromium.org/341763002/diff/120001/ui/views/controls/menu/...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/341763002/diff/120001/ui/views/controls/menu/...
ui/views/controls/menu/menu_controller.cc:2058: gfx::Point screen_loc_pixels =
gfx::win::DIPToScreenPoint(screen_loc);
On 2014/06/19 15:33:32, sky wrote:
> nit: move into conditional block below, it's only needed there.

Done.

[ananta, 2014-06-19 19:30:46]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2014-06-19 19:32:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ananta@chromium.org/341763002/140001

[commit-bot: I haz the power, 2014-06-19 23:59:37]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[commit-bot: I haz the power, 2014-06-20 00:50:26]

Change committed as 278559