Second attempt at fixing pointer lock issues with Windows HiDPI.

Second attempt at fixing pointer lock issues with Windows HiDPI.

The earlier attempt https://codereview.chromium.org/973123003/ was incorrect. Reverted parts that patch
which changed the WebMouseEventBuilder::Build function to convert from DIP to pixels and back. The initial
conversion from DIP to pixels was not needed as the values are picked up from the Windows message which gives
us these coordinates in pixels.

Changes in this patch are as below:-
1. The WebMouseEventBuilder::Build function has been changed to convert the WebMouseEvent::globalX and globalY values
   to DIP. The other changes in the earlier patch as mentioned above have been reverted.

2. The RenderWidgetHostViewAura::UpdateMouseLockRegion function which executes on Windows was clipping the windows cursor
   to a rectangle in DIPs. We need to convert this rectangle to pixels before calling the ClipCursor API. This was the
   main reason for the lock operation not working correctly.

3. I was seeing a DCHECK in the renderer process in the WebViewImpl::pointerLockMouseEvent function as we were sending
   the mouse leave message. Added code in the CanRendererHandleEvent function in the render_widget_host_view_aura.cc file
   to not send this message if we are in the mouse locked state.

BUG=411634
TEST= The main test case in bug 411634

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

[ananta, 2015-03-06 23:45:55]

ananta@chromium.org changed reviewers:
+ scheib@chromium.org

[ananta, 2015-03-07 00:03:11]

ananta@chromium.org changed reviewers:
+ cpu@chromium.org

[scheib, 2015-03-09 04:40:38]

LGTM

[cpu_(ooo_6.6-7.5), 2015-03-09 20:50:07]

cpu@chromium.org changed reviewers:
+ scottmg@chromium.org
- cpu@chromium.org

[cpu_(ooo_6.6-7.5), 2015-03-09 20:50:07]

seems we are going in circles. I might not be the best to review the code since
the last one looked fine, so this one as well as the previous-previous one.

I'll rubberstamp once scott has looked at it or feel free to nominate anybody
who has a clue.

[scottmg, 2015-03-09 21:33:18]

Sorry, I can't really reason about this either, but with a test that we don't
regress/to enforce which is expected, it generally lgtm.

I think the only way to make this stuff sane is to replace gfx::Rect with one of
gfx::Rect<Pixels> or gfx::Rect<Dips> without implicit conversions between them.
Some day.

https://codereview.chromium.org/981393002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/web_input_event_builders_win.cc
(right):

https://codereview.chromium.org/981393002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/web_input_event_builders_win.cc:191: DWORD
time_ms) {
I doesn't seem particularly clear whether this should be dips or pixels. Would
it be possible to add a test maybe near
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/i...
somewhere?

[ananta, 2015-03-09 22:55:59]

https://codereview.chromium.org/981393002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/web_input_event_builders_win.cc
(right):

https://codereview.chromium.org/981393002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/web_input_event_builders_win.cc:191: DWORD
time_ms) {
On 2015/03/09 21:33:18, scottmg wrote:
> I doesn't seem particularly clear whether this should be dips or pixels. Would
> it be possible to add a test maybe near
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/i...
> somewhere?

Added a content_unittest WebInputEventBuilderTest.TestMouseEventScale

[scottmg, 2015-03-09 23:10:09]

lgtm

[cpu_(ooo_6.6-7.5), 2015-03-09 23:50:57]

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

[cpu_(ooo_6.6-7.5), 2015-03-09 23:50:58]

lgtm

[cpu_(ooo_6.6-7.5), 2015-03-09 23:51:02]

The CQ bit was checked by cpu@chromium.org

[cpu_(ooo_6.6-7.5), 2015-03-09 23:51:02]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org
Link to the patchset: https://codereview.chromium.org/981393002/#ps40001 (title:
"Added the files for the test")

[commit-bot: I haz the power, 2015-03-09 23:51:45]

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

[commit-bot: I haz the power, 2015-03-10 00:24:05]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-10 00:24:52]

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

[Nico, 2015-03-10 15:42:37]

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

[Nico, 2015-03-10 15:42:38]

This fails on the DrMemory bots:
http://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20%28...
http://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20%28...

Looks like the expectations are off by a factor of 2 there. Maybe something is
racy and the slowdown from the drmemory instrumentation exposes that?

[scottmg, 2015-03-10 16:59:13]

On 2015/03/10 15:42:38, Nico wrote:
> This fails on the DrMemory bots:
>
http://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20%28...
>
http://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20%28...
> 
> Looks like the expectations are off by a factor of 2 there. Maybe something is
> racy and the slowdown from the drmemory instrumentation exposes that?

It modifies the commandline, maybe that's not allowed/working in drmemory?

I do see a number of other places that do similar things, one difference being
that they don't bother restoring it to "1" at the end of the test. Not sure why
that would be relevant though.

[scottmg, 2015-03-10 17:14:47]

On 2015/03/10 16:59:13, scottmg wrote:
> On 2015/03/10 15:42:38, Nico wrote:
> > This fails on the DrMemory bots:
> >
>
http://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20%28...
> >
>
http://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20%28...
> > 
> > Looks like the expectations are off by a factor of 2 there. Maybe something
is
> > racy and the slowdown from the drmemory instrumentation exposes that?
> 
> It modifies the commandline, maybe that's not allowed/working in drmemory?
> 
> I do see a number of other places that do similar things, one difference being
> that they don't bother restoring it to "1" at the end of the test. Not sure
why
> that would be relevant though.

Hm, or maybe it's because only drmemory runs in a single process. It looks like
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/display.cc&...
puts the value in a static, so if some other test requests the scale factor
earlier it'll be stuck at the original value.

[Nico, 2015-03-10 17:41:09]

On 2015/03/10 17:14:47, scottmg wrote:
> On 2015/03/10 16:59:13, scottmg wrote:
> > On 2015/03/10 15:42:38, Nico wrote:
> > > This fails on the DrMemory bots:
> > >
> >
>
http://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20%28...
> > >
> >
>
http://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20%28...
> > > 
> > > Looks like the expectations are off by a factor of 2 there. Maybe
something
> is
> > > racy and the slowdown from the drmemory instrumentation exposes that?
> > 
> > It modifies the commandline, maybe that's not allowed/working in drmemory?
> > 
> > I do see a number of other places that do similar things, one difference
being
> > that they don't bother restoring it to "1" at the end of the test. Not sure
> why
> > that would be relevant though.
> 
> Hm, or maybe it's because only drmemory runs in a single process. It looks
like
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/display.cc&...
> puts the value in a static, so if some other test requests the scale factor
> earlier it'll be stuck at the original value.

That looks like the cause. One fix might be to have an explicit
"ComputedScaleFactor" bool in that file, and a ResetScaleFactorCacheForTests()
that sets that bool to false again, instead of the static.

[Nico, 2015-03-10 18:11:55]

looks like ananta isn't around today, so I suppose we should just revert for
now?

[chromium-reviews, 2015-03-10 18:21:46]

I will be in around 11:30. Will take a look once I get in

Thanks
Ananta
On Mar 10, 2015 11:11 AM, <thakis@chromium.org> wrote:

> looks like ananta isn't around today, so I suppose we should just revert
> for
> now?
>
> https://codereview.chromium.org/981393002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2015-03-10 18:22:55]

Sounds good, thanks :-)

On Tue, Mar 10, 2015 at 11:21 AM, Anantanarayanan Iyengar <
iyengar@google.com> wrote:

> I will be in around 11:30. Will take a look once I get in
>
> Thanks
> Ananta
> On Mar 10, 2015 11:11 AM, <thakis@chromium.org> wrote:
>
>> looks like ananta isn't around today, so I suppose we should just revert
>> for
>> now?
>>
>> https://codereview.chromium.org/981393002/
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.