Bug 92499 – [chromium] Convert screen space scroll gestures to layer space

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Sami Kyöstilä]

[chromium] Convert screen space scroll gestures to layer space

[Sami Kyöstilä]

Created attachment 154925 [details]
Patch

Work-in-progress. Still trying to improve the diagonal scroll behavior

[WebKit Review Bot]

Comment on attachment 154925 [details]
Patch

Attachment 154925 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13373427

New failing tests:
CCLayerTreeHostImplTest.scrollRotatedLayer

[WebKit Review Bot]

Created attachment 154950 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Shawn Singh]

Comment on attachment 154925 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154925&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:919
> +    FloatPoint actualScreenSpaceEndPoint = CCMathUtil::projectPoint(layerImpl.screenSpaceTransform(), actualLocalEndPoint, endClipped);

shouldn't this be layerImpl.screenSpaceTransform().inverse() ??

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:921
> +    if (endClipped)
> +        return FloatSize();

I think this is a reasonable thing to do for production code, but I personally don't ever expect this to happen.   If you find this case happening (after resolving the previous comment), could you please send me some information about the screenSpacePoint, screenSpaceDelta, and all the transforms?  I think this would be an important scenario to understand if it happens, since it shouldn't happen.  On the other hand, I'm not certain enough that it would be wrong, so an ASSERT is not appropriate until we check when this case might occur.

[Sami Kyöstilä]

(In reply to comment #4)
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:919
> > +    FloatPoint actualScreenSpaceEndPoint = CCMathUtil::projectPoint(layerImpl.screenSpaceTransform(), actualLocalEndPoint, endClipped);
> 
> shouldn't this be layerImpl.screenSpaceTransform().inverse() ??

How so? We're going from local layer coordinates to screen coordinates, so as far as I can tell the forward screen space transform is what we need, right?

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:921
> > +    if (endClipped)
> > +        return FloatSize();
> 
> I think this is a reasonable thing to do for production code, but I personally don't ever expect this to happen.   If you find this case happening (after resolving the previous comment), could you please send me some information about the screenSpacePoint, screenSpaceDelta, and all the transforms?  I think this would be an important scenario to understand if it happens, since it shouldn't happen.  On the other hand, I'm not certain enough that it would be wrong, so an ASSERT is not appropriate until we check when this case might occur.

This was just a safeguard -- I haven't actually seen this happening. Sounds like it would be useful to catch these cases, so I'll add a debug assertion here (and retain the early-out to ensure things keep working on release mode).

[Dana Jansens]

(In reply to comment #5)
> (In reply to comment #4)
> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:919
> > > +    FloatPoint actualScreenSpaceEndPoint = CCMathUtil::projectPoint(layerImpl.screenSpaceTransform(), actualLocalEndPoint, endClipped);
> > 
> > shouldn't this be layerImpl.screenSpaceTransform().inverse() ??
> 
> How so? We're going from local layer coordinates to screen coordinates, so as far as I can tell the forward screen space transform is what we need, right?

If so, then you wan mapPoint not projectPoint I believe.

[Sami Kyöstilä]

(In reply to comment #6)
> If so, then you wan mapPoint not projectPoint I believe.

Ah, that makes much more sense, thanks!

[Sami Kyöstilä]

Created attachment 158796 [details]
Patch

[Hans Wennborg]

Comment on attachment 158796 [details]
Patch

I can't really contribute anything here, but here are some drive-by nits anyway :)

View in context: https://bugs.webkit.org/attachment.cgi?id=158796&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCInputHandler.h:58
> +    // Scroll the layer selected with scrollBegin() starting from the given

ultra ultra nit: i'd go for "the selected layer" over "the layer selected"

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:899
> +static FloatSize scrollLayerWithScreenSpaceDelta(CCLayerImpl& layerImpl,

nit: no need to break the line here, i think

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:162
> +        layer->setPosition(FloatPoint(0, 0));

maybe just FloatPoint() ?

[Sami Kyöstilä]

(In reply to comment #9)
> I can't really contribute anything here, but here are some drive-by nits anyway :)

Any and all nits are welcome :)

> View in context: https://bugs.webkit.org/attachment.cgi?id=158796&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCInputHandler.h:58
> > +    // Scroll the layer selected with scrollBegin() starting from the given
> 
> ultra ultra nit: i'd go for "the selected layer" over "the layer selected"

Yeah, that's a little awkward. Rephrasing.
 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:899
> > +static FloatSize scrollLayerWithScreenSpaceDelta(CCLayerImpl& layerImpl,
> 
> nit: no need to break the line here, i think

Done.

> > Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:162
> > +        layer->setPosition(FloatPoint(0, 0));
> 
> maybe just FloatPoint() ?

True. I think I'll just drop this since the position is implicitly at (0, 0).

[Sami Kyöstilä]

Created attachment 158810 [details]
Patch

[WebKit Review Bot]

Comment on attachment 158810 [details]
Patch

Rejecting attachment 158810 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
eded at 1149 (offset 20 lines).
Hunk #22 succeeded at 1161 (offset 20 lines).
Hunk #23 succeeded at 1175 (offset 20 lines).
patching file Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp
patching file Source/WebKit/chromium/tests/CCMathUtilTest.cpp
patching file Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'James Robi..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/13537465

[Sami Kyöstilä]

Created attachment 159377 [details]
Patch

Rebased.

[WebKit Review Bot]

Comment on attachment 159377 [details]
Patch

Rejecting attachment 159377 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13542266

[Sami Kyöstilä]

Created attachment 159382 [details]
Patch

Also update the second ChangeLog.

[WebKit Review Bot]

Comment on attachment 159382 [details]
Patch

Clearing flags on attachment: 159382

Committed r126018: <http://trac.webkit.org/changeset/126018>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.