Fix frame coordinate translation issue with scroll views.

Fix frame coordinate translation issue with scroll views.

The absolute position was being used (which is in content coordinates)
but we were passing the root frame coordinates in. Translate the root
frame coordinates into content coordinates before applying the transform
from the LayoutObject. This matches the implementation of
RootFrameToLocalPoint methods.

BUG=710165
Review-Url: https://codereview.chromium.org/2814473003
Cr-Commit-Position: refs/heads/master@{#464170}
Committed: https://chromium.googlesource.com/chromium/src/+/b452deb2b1802b68bf9662308074e4b10904f709

[dtapuska, 2017-04-10 21:10:23]

dtapuska@chromium.org changed reviewers:
+ mustaq@chromium.org

[dtapuska, 2017-04-10 21:10:23]

Will write a test on Wednesday when I return

[mustaq, 2017-04-11 15:20:09]

https://codereview.chromium.org/2814473003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/2814473003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebInputEventConversion.cpp:200: // Translate the
absolute position to content coordinates.
This particular change is not related to the bug because it is about mouse
events, right?

I have the following concern here, mostly about the original code. So I suggest
deferring the mouse event change, adding just a TODO in this CL.

I couldn't explain the difference of the original code here (the use of
PositionInRootFrame() in particular) with
WebPluginContainerImpl::HandleTouchEvent which doesn't do this. Also, the
transform in HandleTouchEvent should be moved here for sake of better
readability/consistency too.

[dtapuska, 2017-04-12 15:13:34]

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

[commit-bot: I haz the power, 2017-04-12 15:14:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dtapuska, 2017-04-12 15:14:21]

dtapuska@chromium.org changed reviewers:
+ bokan@chromium.org

[dtapuska, 2017-04-12 15:14:21]

PTAL

[mustaq, 2017-04-12 15:38:38]

LGTM % a nit.

https://codereview.chromium.org/2814473003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/2814473003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInputEventConversion.cpp:198: WebFloatPoint
absolute_root_frame_location = PositionInRootFrame();
Nit: after flattening, s/PositionInRootFrame()/PositionInWidget()/ is better?

[commit-bot: I haz the power, 2017-04-12 16:37:04]

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

[commit-bot: I haz the power, 2017-04-12 16:37:05]

Dry run: This issue passed the CQ dry run.

[bokan, 2017-04-12 17:28:44]

lgtm

https://codereview.chromium.org/2814473003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/2814473003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInputEventConversion.cpp:198: WebFloatPoint
absolute_root_frame_location = PositionInRootFrame();
On 2017/04/12 15:38:38, mustaq wrote:
> Nit: after flattening, s/PositionInRootFrame()/PositionInWidget()/ is better?

Isn't the transform what gets us from Widget to RootFrame? i.e. the pinch scale
and pan. In which case it's currently correct...

https://codereview.chromium.org/2814473003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInputEventConversion.cpp:201: if
(plugin_parent) {
There seems to be only one call site and plugin_parent should never be null.
Could you add a TODO to remove the if (better not to do here since we're
merging). Also, change absolute_root_frame_location -> absolute_location or
content_location.

https://codereview.chromium.org/2814473003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/2814473003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:770: WebFloatPoint
absolute_root_frame_location =
This (and below) should now be called absolute_location or content_location,
right?

https://codereview.chromium.org/2814473003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebPluginContainerTest.cpp (right):

https://codereview.chromium.org/2814473003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebPluginContainerTest.cpp:577:
TEST_F(WebPluginContainerTest, TouchEventScrolled) {
Perhaps add a test with some pinch-zoom to ensure the transform gets correctly
applied? Though I'm not even sure if that works today, I recall some issues with
plugins and pinch-zoom...

[mustaq, 2017-04-12 17:52:04]

https://codereview.chromium.org/2814473003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/2814473003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInputEventConversion.cpp:198: WebFloatPoint
absolute_root_frame_location = PositionInRootFrame();
On 2017/04/12 17:28:44, bokan wrote:
> On 2017/04/12 15:38:38, mustaq wrote:
> > Nit: after flattening, s/PositionInRootFrame()/PositionInWidget()/ is
better?
> 
> Isn't the transform what gets us from Widget to RootFrame? i.e. the pinch
scale
> and pan. In which case it's currently correct...

|PositionInRootFrame| is correct but I thought |PositionInWidget| is a faster
way to get the same coordinates after the FlattenTransform() above (which makes
the transform stored in |*this| is a no-op)---if I am not missing something else
in between.

[dtapuska, 2017-04-12 19:02:56]

https://codereview.chromium.org/2814473003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/2814473003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInputEventConversion.cpp:198: WebFloatPoint
absolute_root_frame_location = PositionInRootFrame();
On 2017/04/12 17:52:04, mustaq wrote:
> On 2017/04/12 17:28:44, bokan wrote:
> > On 2017/04/12 15:38:38, mustaq wrote:
> > > Nit: after flattening, s/PositionInRootFrame()/PositionInWidget()/ is
> better?
> > 
> > Isn't the transform what gets us from Widget to RootFrame? i.e. the pinch
> scale
> > and pan. In which case it's currently correct...
> 
> |PositionInRootFrame| is correct but I thought |PositionInWidget| is a faster
> way to get the same coordinates after the FlattenTransform() above (which
makes
> the transform stored in |*this| is a no-op)---if I am not missing something
else
> in between.

Ya it is the same. But for readability sake we probably should keep it with the
slower version.

https://codereview.chromium.org/2814473003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebInputEventConversion.cpp:201: if
(plugin_parent) {
On 2017/04/12 17:28:44, bokan wrote:
> There seems to be only one call site and plugin_parent should never be null.
> Could you add a TODO to remove the if (better not to do here since we're
> merging). Also, change absolute_root_frame_location -> absolute_location or
> content_location.

Done.

https://codereview.chromium.org/2814473003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/2814473003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:770: WebFloatPoint
absolute_root_frame_location =
On 2017/04/12 17:28:44, bokan wrote:
> This (and below) should now be called absolute_location or content_location,
> right?

Done.

https://codereview.chromium.org/2814473003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebPluginContainerTest.cpp (right):

https://codereview.chromium.org/2814473003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebPluginContainerTest.cpp:577:
TEST_F(WebPluginContainerTest, TouchEventScrolled) {
On 2017/04/12 17:28:44, bokan wrote:
> Perhaps add a test with some pinch-zoom to ensure the transform gets correctly
> applied? Though I'm not even sure if that works today, I recall some issues
with
> plugins and pinch-zoom...

Done.

[dtapuska, 2017-04-12 19:03:34]

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

[commit-bot: I haz the power, 2017-04-12 19:05:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bokan, 2017-04-12 19:35:53]

thanks, lgtm

[dtapuska, 2017-04-12 20:55:08]

The CQ bit was unchecked by dtapuska@chromium.org

[dtapuska, 2017-04-12 20:55:14]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2017-04-12 20:55:15]

The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org
Link to the patchset: https://codereview.chromium.org/2814473003/#ps60001
(title: "Add more zoom tests")

[commit-bot: I haz the power, 2017-04-12 20:55:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 22:13:28]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492030514769510,
"parent_rev": "9b11e822fff411d0d9ee23727d18e820b3d07fb7", "commit_rev":
"b452deb2b1802b68bf9662308074e4b10904f709"}

[commit-bot: I haz the power, 2017-04-12 22:14:26]

Description was changed from

==========
Fix frame coordinate translation issue with scroll views.

The absolute position was being used (which is in content coordinates)
but we were passing the root frame coordinates in. Translate the root
frame coordinates into content coordinates before applying the transform
from the LayoutObject. This matches the implementation of
RootFrameToLocalPoint methods.

BUG=710165
==========

to

==========
Fix frame coordinate translation issue with scroll views.

The absolute position was being used (which is in content coordinates)
but we were passing the root frame coordinates in. Translate the root
frame coordinates into content coordinates before applying the transform
from the LayoutObject. This matches the implementation of
RootFrameToLocalPoint methods.

BUG=710165

Review-Url: https://codereview.chromium.org/2814473003
Cr-Commit-Position: refs/heads/master@{#464170}
Committed:
https://chromium.googlesource.com/chromium/src/+/b452deb2b1802b68bf9662308074...
==========

[commit-bot: I haz the power, 2017-04-12 22:14:27]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b452deb2b1802b68bf9662308074...