Remove GraphicsLayer::didScroll and directly call ScrollableArea::didScroll

Remove GraphicsLayer::didScroll and directly call ScrollableArea::didScroll

This patch removes GraphicsLayer::didScroll, replacing this cc->blink
scroll callback with a direct call to Scrollablearea::didScroll. The
old function on GraphicsLayer was only used to lookup the scroll
offset but we can pass that along in the didScroll callback.

This patch will be useful for SPV2 which does not use GraphicsLayer and
can benefit from avoiding it, but this patch is intended to be useful
for SPV1. This code has been split off of a larger patch for composited
scrolling: https://codereview.chromium.org/2676923002.

BUG=667946
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2680953002
Cr-Commit-Position: refs/heads/master@{#449073}
Committed: https://chromium.googlesource.com/chromium/src/+/27a8cfde871ad4786943a345569f36c9a5ba6e29

[pdr., 2017-02-07 21:59:46]

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

[pdr., 2017-02-07 22:00:19]

pdr@chromium.org changed reviewers:
+ ajuma@chromium.org, chrishtr@chromium.org, wkorman@chromium.org

[commit-bot: I haz the power, 2017-02-07 22:01:31]

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

[wkorman, 2017-02-07 22:18:05]

https://codereview.chromium.org/2680953002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsLayerTest.cpp (right):

https://codereview.chromium.org/2680953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsLayerTest.cpp:212:
m_platformLayer->setScrollPositionDouble(scrollPosition);
Do we still need to do this? It looks like this was set so that it could be
pulled off in GraphicsLayer.cpp:1166 above, but since now we pass it in below...
do we still need to set it here? Maybe there was some other reason for the
indirection?

I see this test is checking it in 215/216 below but with this change the test
case passes but maybe is just no longer relevant and the test case should just
be removed?

Anyway the only other remaining use of setScrollPositionDouble after this
change, outside of tests, looks like it's in
ScrollingCoordinator::scrollableAreaScrollLayerDidChange. And the only
getter-accessor use of it is in GraphicsLayer::didScroll() which you removed
above, so maybe we don't need the scrollPositionDouble field/accessors at all
any more.

https://codereview.chromium.org/2680953002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/2680953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:665: void
ScrollableArea::didScroll(const gfx::ScrollOffset& offset) {
Which unit test covers this? WebFrameTest? Is it checking the offsets to make
sure what was passed stuck? I am not sure I'm seeing that in the test case.

[commit-bot: I haz the power, 2017-02-07 23:43:52]

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

[commit-bot: I haz the power, 2017-02-07 23:43:52]

Dry run: This issue passed the CQ dry run.

[pdr., 2017-02-08 03:53:56]

https://codereview.chromium.org/2680953002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsLayerTest.cpp (right):

https://codereview.chromium.org/2680953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsLayerTest.cpp:212:
m_platformLayer->setScrollPositionDouble(scrollPosition);
On 2017/02/07 at 22:18:05, wkorman wrote:
> Do we still need to do this? It looks like this was set so that it could be
pulled off in GraphicsLayer.cpp:1166 above, but since now we pass it in below...
do we still need to set it here? Maybe there was some other reason for the
indirection?
> 
> I see this test is checking it in 215/216 below but with this change the test
case passes but maybe is just no longer relevant and the test case should just
be removed?

Good catch, these are no longer needed.

> 
> Anyway the only other remaining use of setScrollPositionDouble after this
change, outside of tests, looks like it's in
ScrollingCoordinator::scrollableAreaScrollLayerDidChange. And the only
getter-accessor use of it is in GraphicsLayer::didScroll() which you removed
above, so maybe we don't need the scrollPositionDouble field/accessors at all
any more.

We still need this setter for the ScrollingCoordinator case, as it forwards the
offset to the cc::Layer. The getter is only used in tests but it's for the same
reason. That said, the double-specific getter/setter isn't useful since we have
floats on both sides. I've updated the API to use floats (FloatPoint
scrollPosition(), setScrollPosition(FloatPoint)).

https://codereview.chromium.org/2680953002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/2680953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:665: void
ScrollableArea::didScroll(const gfx::ScrollOffset& offset) {
On 2017/02/07 at 22:18:05, wkorman wrote:
> Which unit test covers this? WebFrameTest? Is it checking the offsets to make
sure what was passed stuck? I am not sure I'm seeing that in the test case.

These are tested but only implicitly. I've added a new test,
ScrollableAreaDidScroll, that checks for sticking and ensures the scroll origin
is included. I've also removed the GraphicsLayerTest.

[pdr., 2017-02-08 03:54:02]

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

[commit-bot: I haz the power, 2017-02-08 03:54:20]

Dry run: 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-02-08 06:03:44]

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

[commit-bot: I haz the power, 2017-02-08 06:03:45]

Dry run: This issue passed the CQ dry run.

[ajuma, 2017-02-08 15:27:29]

lgtm

[pdr., 2017-02-08 15:28:48]

On 2017/02/08 at 15:27:29, ajuma wrote:
> lgtm

Thanks!

@wkorman, can you take one more look to see if your concerns were addressed? If
so, please send this to the commit queue.

[wkorman, 2017-02-08 19:28:34]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2017-02-08 19:28:34]

lgtm

[commit-bot: I haz the power, 2017-02-08 19:29:12]

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-02-08 19:40:59]

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

[commit-bot: I haz the power, 2017-02-08 19:41:00]

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

[pdr., 2017-02-08 19:50:22]

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

[pdr., 2017-02-08 19:52:28]

On 2017/02/08 at 19:41:00, commit-bot wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Nico, could you please review src/ui

[Nico, 2017-02-08 19:53:52]

ui/ lgtm

[pdr., 2017-02-08 20:00:26]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2017-02-08 20:01:07]

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-02-08 20:12:27]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486584026618620,
"parent_rev": "39f049e94f28f0c26dba4aae5fe7d2818d4c570d", "commit_rev":
"27a8cfde871ad4786943a345569f36c9a5ba6e29"}

[commit-bot: I haz the power, 2017-02-08 20:13:00]

Description was changed from

==========
Remove GraphicsLayer::didScroll and directly call ScrollableArea::didScroll

This patch removes GraphicsLayer::didScroll, replacing this cc->blink
scroll callback with a direct call to Scrollablearea::didScroll. The
old function on GraphicsLayer was only used to lookup the scroll
offset but we can pass that along in the didScroll callback.

This patch will be useful for SPV2 which does not use GraphicsLayer and
can benefit from avoiding it, but this patch is intended to be useful
for SPV1. This code has been split off of a larger patch for composited
scrolling: https://codereview.chromium.org/2676923002.

BUG=667946
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Remove GraphicsLayer::didScroll and directly call ScrollableArea::didScroll

This patch removes GraphicsLayer::didScroll, replacing this cc->blink
scroll callback with a direct call to Scrollablearea::didScroll. The
old function on GraphicsLayer was only used to lookup the scroll
offset but we can pass that along in the didScroll callback.

This patch will be useful for SPV2 which does not use GraphicsLayer and
can benefit from avoiding it, but this patch is intended to be useful
for SPV1. This code has been split off of a larger patch for composited
scrolling: https://codereview.chromium.org/2676923002.

BUG=667946
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2680953002
Cr-Commit-Position: refs/heads/master@{#449073}
Committed:
https://chromium.googlesource.com/chromium/src/+/27a8cfde871ad4786943a345569f...
==========

[commit-bot: I haz the power, 2017-02-08 20:13:02]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/27a8cfde871ad4786943a345569f...