[chromium] Convert screen space scroll gestures to layer space
Created attachment 154925 [details] Patch Work-in-progress. Still trying to improve the diagonal scroll behavior
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
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
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.
(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).
(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.
(In reply to comment #6) > If so, then you wan mapPoint not projectPoint I believe. Ah, that makes much more sense, thanks!
Created attachment 158796 [details] Patch
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() ?
(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).
Created attachment 158810 [details] Patch
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
Created attachment 159377 [details] Patch Rebased.
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
Created attachment 159382 [details] Patch Also update the second ChangeLog.
Comment on attachment 159382 [details] Patch Clearing flags on attachment: 159382 Committed r126018: <http://trac.webkit.org/changeset/126018>
All reviewed patches have been landed. Closing bug.