Convert MouseEvents from DIPS to Pixels before passing it to surfaces for hittesting.

Convert MouseEvents from DIPS to Pixels before passing it to surfaces for hittesting.

BUG=529906
Committed: https://crrev.com/52bac907b88927112477d3a2700c6e6fdb1a8804
Cr-Commit-Position: refs/heads/master@{#358344}

[lfg, 2015-09-10 17:28:17]

lfg@chromium.org changed reviewers:
+ sadrul@chromium.org

[lfg, 2015-09-10 17:28:17]

Hi Sadrul, can you take a look? Thanks!

[sadrul, 2015-09-11 17:48:11]

https://codereview.chromium.org/1308553007/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1308553007/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2172:
host->GetRootTransform().TransformPoint(&point_in_pixels);
Does this do the right thing when the display is rotated on a chromeos device?
Because the root-transform possibly includes the rotation transforms that you
don't want to use?

Also, can you add tests?

[lfg, 2015-09-11 19:19:10]

On 2015/09/11 17:48:11, sadrul wrote:
>
https://codereview.chromium.org/1308553007/diff/20001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/1308553007/diff/20001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_view_aura.cc:2172:
> host->GetRootTransform().TransformPoint(&point_in_pixels);
> Does this do the right thing when the display is rotated on a chromeos device?
> Because the root-transform possibly includes the rotation transforms that you
> don't want to use?

Is there any way to simulate a rotated device so I can test this?

[sadrul, 2015-09-14 20:23:54]

On 2015/09/11 19:19:10, lfg wrote:
> On 2015/09/11 17:48:11, sadrul wrote:
> >
>
https://codereview.chromium.org/1308553007/diff/20001/content/browser/rendere...
> > File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> > 
> >
>
https://codereview.chromium.org/1308553007/diff/20001/content/browser/rendere...
> > content/browser/renderer_host/render_widget_host_view_aura.cc:2172:
> > host->GetRootTransform().TransformPoint(&point_in_pixels);
> > Does this do the right thing when the display is rotated on a chromeos
device?
> > Because the root-transform possibly includes the rotation transforms that
you
> > don't want to use?
> 
> Is there any way to simulate a rotated device so I can test this?

I believe for cros, we apply a rotation transform on the root-window? (I am
looking at AshRootWindowTransformer)

[lfg, 2015-10-19 21:21:00]

Hey, sorry for the delay, I had some higher priority stuff coming in, but
finally got some time to look at this. Can you take another look?

[sadrul, 2015-10-21 21:46:21]

The test failure on mac seem unrelated to crbug.com/545547. Mind taking a look
at that?

https://codereview.chromium.org/1308553007/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1308553007/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2171: const
gfx::Display display = window_ ?
When can |window_| be null here?

https://codereview.chromium.org/1308553007/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2174: float
scale_factor = display.device_scale_factor();
How often do we have to do this? Would it make sense to cache the scale-factor
in RWHVA (and update in OnDeviceScaleFactorChanged() override)?

[lfg, 2015-10-22 18:35:44]

Patchset #5 (id:80001) has been deleted

[lfg, 2015-10-22 20:24:48]

On 2015/10/21 21:46:21, sadrul wrote:
> The test failure on mac seem unrelated to crbug.com/545547. Mind taking a look
> at that?

I believe it's the same issue I fixed here:
https://codereview.chromium.org/1416173002/ .

>
https://codereview.chromium.org/1308553007/diff/60001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/1308553007/diff/60001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_view_aura.cc:2171: const
> gfx::Display display = window_ ?
> When can |window_| be null here?

After looking at it, I don't think it can be null. I used the same code from
another place, and was being too careful around that.

>
https://codereview.chromium.org/1308553007/diff/60001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_view_aura.cc:2174: float
> scale_factor = display.device_scale_factor();
> How often do we have to do this? Would it make sense to cache the scale-factor
> in RWHVA (and update in OnDeviceScaleFactorChanged() override)?

That's a good point. It's used for every mouse event (i.e. mouse move is the
most common one). So it should be quite frequent, but not that much (~once per
frame). It's still probably a good idea to cache it, so I've updated the code
(Mac is different, and there's already an accessor for it).

[lfg, 2015-11-02 19:03:15]

Hey Sadrul, please take another look. Mac should be working now.

[sadrul, 2015-11-03 18:00:00]

Could you add a mac owner for the mac change (looks reasonable to me)?

https://codereview.chromium.org/1308553007/diff/180001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1308553007/diff/180001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:509:
device_scale_factor_ = display.device_scale_factor();
Note that at this point, |window_| hasn't been attached to a window-tree yet. So
GetDisplayNearestWindow() wouldn't necessarily give the right result. The better
place for this would be in the three InitAs* methods, where |window_| is
attached. In InitAsChild(), you should get the Display nearest |parent_view|. In
InitAsPopup() and InitAsFullscreen(), you should get the Display nearest
|window_| after the calls to ParentWindowWithContext(). That way, you start with
the correct DSF. In the ctor initialize-list, you should initialize
|device_scale_factor_| to some default value

https://codereview.chromium.org/1308553007/diff/180001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2188:
gfx::ConvertPointToDIP(device_scale_factor_, *transformed_point);
Add a comment here to clarify that surface-hittest happens in device-pixels.

https://codereview.chromium.org/1308553007/diff/180001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1308553007/diff/180001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:103:
RenderWidgetHostMouseEventMonitor(RenderWidgetHost* host)
explicit

https://codereview.chromium.org/1308553007/diff/180001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:114: bool EventWasReceived() {
return event_received; }
const

https://codereview.chromium.org/1308553007/diff/180001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:125: bool event_received;
event_received_

https://codereview.chromium.org/1308553007/diff/180001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:127: };
DISALLOW_COPY_...

(I realize that you are just moving RenderWidgetHostMouseEventMonitor from
existing code, but since you are here, might as well fix some nits. thanks!)

https://codereview.chromium.org/1308553007/diff/180001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:177: }
This is still unfortunate. I have left a comment in the bug.

https://codereview.chromium.org/1308553007/diff/180001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:216: }
eugh ... this too! We really need to find a better solution for this sort of
thing. (I understand that this too is existing code ... still, unfortunate that
we are not fixing)

https://codereview.chromium.org/1308553007/diff/180001/content/browser/site_p...
File content/browser/site_per_process_browsertest.h (right):

https://codereview.chromium.org/1308553007/diff/180001/content/browser/site_p...
content/browser/site_per_process_browsertest.h:39: };
This doesn't need to be in the header, right?

[lfg, 2015-11-03 20:49:16]

https://codereview.chromium.org/1308553007/diff/180001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1308553007/diff/180001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:509:
device_scale_factor_ = display.device_scale_factor();
On 2015/11/03 18:00:00, sadrul wrote:
> Note that at this point, |window_| hasn't been attached to a window-tree yet.
So
> GetDisplayNearestWindow() wouldn't necessarily give the right result. The
better
> place for this would be in the three InitAs* methods, where |window_| is
> attached. In InitAsChild(), you should get the Display nearest |parent_view|.
In
> InitAsPopup() and InitAsFullscreen(), you should get the Display nearest
> |window_| after the calls to ParentWindowWithContext(). That way, you start
with
> the correct DSF. In the ctor initialize-list, you should initialize
> |device_scale_factor_| to some default value

Done. I've also added a DCHECK to make sure it's initialized before we use it.

https://codereview.chromium.org/1308553007/diff/180001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2188:
gfx::ConvertPointToDIP(device_scale_factor_, *transformed_point);
On 2015/11/03 18:00:00, sadrul wrote:
> Add a comment here to clarify that surface-hittest happens in device-pixels.

Done.

https://codereview.chromium.org/1308553007/diff/180001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1308553007/diff/180001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:103:
RenderWidgetHostMouseEventMonitor(RenderWidgetHost* host)
On 2015/11/03 18:00:00, sadrul wrote:
> explicit

Done.

https://codereview.chromium.org/1308553007/diff/180001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:114: bool EventWasReceived() {
return event_received; }
On 2015/11/03 18:00:00, sadrul wrote:
> const

Done.

https://codereview.chromium.org/1308553007/diff/180001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:125: bool event_received;
On 2015/11/03 18:00:00, sadrul wrote:
> event_received_

Done.

https://codereview.chromium.org/1308553007/diff/180001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:127: };
On 2015/11/03 18:00:00, sadrul wrote:
> DISALLOW_COPY_...
> 
> (I realize that you are just moving RenderWidgetHostMouseEventMonitor from
> existing code, but since you are here, might as well fix some nits. thanks!)

Done.

https://codereview.chromium.org/1308553007/diff/180001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:216: }
On 2015/11/03 18:00:00, sadrul wrote:
> eugh ... this too! We really need to find a better solution for this sort of
> thing. (I understand that this too is existing code ... still, unfortunate
that
> we are not fixing)

I discussed an alternative to this with kenrb@, which would be to parse the
delegated frame data and look for the surface reference, but he pointed that
this would be duplicating a lot of the code we want to test, which is true.

The current assumption of the test is only that we'll eventually hit a state
where the hittest reaches the child process directly, without going through the
parent frame process.

https://codereview.chromium.org/1308553007/diff/180001/content/browser/site_p...
File content/browser/site_per_process_browsertest.h (right):

https://codereview.chromium.org/1308553007/diff/180001/content/browser/site_p...
content/browser/site_per_process_browsertest.h:39: };
On 2015/11/03 18:00:00, sadrul wrote:
> This doesn't need to be in the header, right?

Done.

[sadrul, 2015-11-04 19:03:44]

The aura changes lgtm. Please get a review from a mac owner for the mac change.

https://codereview.chromium.org/1308553007/diff/200001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1308553007/diff/200001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:531:
gfx::Screen::GetScreenFor(window_)->GetDisplayNearestWindow(window_);
I don't think |window_| is parented here yet. Better to look at parent_view
instead

[lfg, 2015-11-04 19:39:09]

https://codereview.chromium.org/1308553007/diff/200001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1308553007/diff/200001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:531:
gfx::Screen::GetScreenFor(window_)->GetDisplayNearestWindow(window_);
On 2015/11/04 19:03:44, sadrul wrote:
> I don't think |window_| is parented here yet. Better to look at parent_view
> instead

Done.

[lfg, 2015-11-04 19:48:44]

lfg@chromium.org changed reviewers:
+ avi@chromium.org

[lfg, 2015-11-04 19:48:44]

Hey Avi, can you please take a look at:

- render_widget_host_view_mac.mm
- site_per_process_browsertest.cc


Thanks!

[Avi (use Gerrit), 2015-11-04 20:16:47]

lgtm

https://codereview.chromium.org/1308553007/diff/220001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1308553007/diff/220001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:1600:
.device_scale_factor();
I'm thinking in Cocoa, and would instinctively do

float scale_factor = [[cocoa_view_ window] backingScaleFactor];

asking the window itself what the scale factor was, rather than wandering around
like this. I guess this is the fancy gfx:: way we do it? (shrug)

[lfg, 2015-11-04 21:08:39]

On 2015/11/04 20:16:47, Avi wrote:
> lgtm
> 
>
https://codereview.chromium.org/1308553007/diff/220001/content/browser/render...
> File content/browser/renderer_host/render_widget_host_view_mac.mm (right):
> 
>
https://codereview.chromium.org/1308553007/diff/220001/content/browser/render...
> content/browser/renderer_host/render_widget_host_view_mac.mm:1600:
> .device_scale_factor();
> I'm thinking in Cocoa, and would instinctively do
> 
> float scale_factor = [[cocoa_view_ window] backingScaleFactor];
> 
> asking the window itself what the scale factor was, rather than wandering
around
> like this. I guess this is the fancy gfx:: way we do it? (shrug)

I initially tried using the scale factor from the cocoa view, the issue is that
with --force-device-scale-factor, the native view's scale factor ends up being
different than the gfx scale factor :(

[lfg, 2015-11-05 19:22:31]

The CQ bit was checked by lfg@chromium.org

[lfg, 2015-11-05 19:22:31]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1308553007/#ps220001
(title: "fixing InitAsChild")

[commit-bot: I haz the power, 2015-11-05 19:23:00]

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

[commit-bot: I haz the power, 2015-11-05 19:26:30]

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

[commit-bot: I haz the power, 2015-11-05 19:26:31]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lfg, 2015-11-06 16:29:31]

The CQ bit was checked by lfg@chromium.org

[lfg, 2015-11-06 16:29:31]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1308553007/#ps240001
(title: "rebase")

[commit-bot: I haz the power, 2015-11-06 16:30:02]

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

[commit-bot: I haz the power, 2015-11-06 17:28:00]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2015-11-06 17:28:55]

Patchset 12 (id:??) landed as
https://crrev.com/52bac907b88927112477d3a2700c6e6fdb1a8804
Cr-Commit-Position: refs/heads/master@{#358344}