Rewrite blink->cc scroll offset logic for SPV2

Use blink's scroll offset to update cc's transform scroll offset for SPV2

Blink and cc use slightly different transform nodes for scroll offset.
Blink creates a transform node just for scroll offset whereas cc has
a special field (scroll_offset) embedded in the transform node. To
account for this, PropertyTreeManager::updateScrollOffset adjusted the
cc transform node.

updateScrollOffset had a bug where the compositor node's scroll offset would
be set from the compositor node's transform, then the transform would
be zero'd. This was broken when updateScrollOffset was called a second
time because the newly-zero'd transform would be used to overwrite the
scroll offset.

Fixing this bug exposed a second bug where setting both the transform
node's scroll_offset and calling scrollTree().SetScrollOffset(...) would
lead to missed compositor-side scroll offset updates (this is tested in
fast/scrolling/fixed-position.html). The SetScrollOffset call has been
temporarily replaced by a TODO for a followup patch.

With this patch google.com is usable with SPV2 \o/

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

Review-Url: https://codereview.chromium.org/2643203002
Cr-Commit-Position: refs/heads/master@{#445399}
Committed: https://chromium.googlesource.com/chromium/src/+/de4408730000072669b7f4d27d0e5e8066988724

[pdr., 2017-01-20 06:14:57]

Description was changed from

==========
Rewrite blink->cc scroll offset logic for SPV2

Blink and cc use slightly different transform nodes for scroll offset.
Blink creates a transform node just for scroll offset whereas cc has
a special field (scroll_offset) embedded in the transform node. To
account for this, PropertyTreeManager::updateScrollOffset adjusted the
cc transform node.

updateScrollOffset had a bug where the compositor node's scroll offset would
be set from the compositor node's transform, then the transform would
be zero'd. This was broken when updateScrollOffset was called a second
time because the newly-zero'd transform would be used to overwrite the
scroll offset.

Fixing this bug exposed a second bug where setting both the transform
node's scroll_offset and calling scrollTree().SetScrollOffset(...) would
lead to missed compositor-side scroll offset updates (this is tested in
fast/scrolling/fixed-position.html).

With this patch google.com is usable with SPV2 \o/

BUG=667946
==========

to

==========
Rewrite blink->cc scroll offset logic for SPV2

Blink and cc use slightly different transform nodes for scroll offset.
Blink creates a transform node just for scroll offset whereas cc has
a special field (scroll_offset) embedded in the transform node. To
account for this, PropertyTreeManager::updateScrollOffset adjusted the
cc transform node.

updateScrollOffset had a bug where the compositor node's scroll offset would
be set from the compositor node's transform, then the transform would
be zero'd. This was broken when updateScrollOffset was called a second
time because the newly-zero'd transform would be used to overwrite the
scroll offset.

Fixing this bug exposed a second bug where setting both the transform
node's scroll_offset and calling scrollTree().SetScrollOffset(...) would
lead to missed compositor-side scroll offset updates (this is tested in
fast/scrolling/fixed-position.html).

With this patch google.com is usable with SPV2 \o/

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

[pdr., 2017-01-20 06:15:43]

Description was changed from

==========
Rewrite blink->cc scroll offset logic for SPV2

Blink and cc use slightly different transform nodes for scroll offset.
Blink creates a transform node just for scroll offset whereas cc has
a special field (scroll_offset) embedded in the transform node. To
account for this, PropertyTreeManager::updateScrollOffset adjusted the
cc transform node.

updateScrollOffset had a bug where the compositor node's scroll offset would
be set from the compositor node's transform, then the transform would
be zero'd. This was broken when updateScrollOffset was called a second
time because the newly-zero'd transform would be used to overwrite the
scroll offset.

Fixing this bug exposed a second bug where setting both the transform
node's scroll_offset and calling scrollTree().SetScrollOffset(...) would
lead to missed compositor-side scroll offset updates (this is tested in
fast/scrolling/fixed-position.html).

With this patch google.com is usable with SPV2 \o/

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

to

==========
Rewrite blink->cc scroll offset logic for SPV2

Blink and cc use slightly different transform nodes for scroll offset.
Blink creates a transform node just for scroll offset whereas cc has
a special field (scroll_offset) embedded in the transform node. To
account for this, PropertyTreeManager::updateScrollOffset adjusted the
cc transform node.

updateScrollOffset had a bug where the compositor node's scroll offset would
be set from the compositor node's transform, then the transform would
be zero'd. This was broken when updateScrollOffset was called a second
time because the newly-zero'd transform would be used to overwrite the
scroll offset.

Fixing this bug exposed a second bug where setting both the transform
node's scroll_offset and calling scrollTree().SetScrollOffset(...) would
lead to missed compositor-side scroll offset updates (this is tested in
fast/scrolling/fixed-position.html).

With this patch google.com is usable with SPV2 \o/

BUG=667946

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

[pdr., 2017-01-20 06:15:43]

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

[pdr., 2017-01-20 06:15:47]

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

[commit-bot: I haz the power, 2017-01-20 06:16:07]

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-01-20 08:33:45]

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

[commit-bot: I haz the power, 2017-01-20 08:33:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pdr., 2017-01-20 18:45:28]

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

[wkorman, 2017-01-20 19:33:52]

lgtm

Per discussion see if we can write a unit test to prevent regression of the
scrolling that newly works.

[ajuma, 2017-01-20 21:35:48]

https://codereview.chromium.org/2643203002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp
(left):

https://codereview.chromium.org/2643203002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp:292:
scrollTree().SetScrollOffset(layerId,
With this removed, is the scroll offset getting set on the scroll tree at all?
(If not, I'd expect main-thread-driven scrolling to work, but compositor-driven
scrolling to be broken.)

[pdr., 2017-01-20 23:01:40]

On 2017/01/20 at 21:35:48, ajuma wrote:
>
https://codereview.chromium.org/2643203002/diff/1/third_party/WebKit/Source/p...
> File
third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp
(left):
> 
>
https://codereview.chromium.org/2643203002/diff/1/third_party/WebKit/Source/p...
>
third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp:292:
scrollTree().SetScrollOffset(layerId,
> With this removed, is the scroll offset getting set on the scroll tree at all?
(If not, I'd expect main-thread-driven scrolling to work, but compositor-driven
scrolling to be broken.)

We actually don't have any compositor scrolling working yet :/ I am hoping to
add that next but we have some chicken-and-egg issues. Sadly, I just found that
scrolling is still completely broken on Windows (with and without this patch).

Do you have a suggestion for how this should be architected? Should we call
scrollTree().SetScrollOffset only for compositor-driven scrolling? Or do we need
to set both? Similarly, do you think it makes sense to get main thread scrolling
working first, or do we need to get them wired up together?

[ajuma, 2017-01-20 23:11:27]

On 2017/01/20 23:01:40, pdr. wrote:
> On 2017/01/20 at 21:35:48, ajuma wrote:
> >
>
https://codereview.chromium.org/2643203002/diff/1/third_party/WebKit/Source/p...
> > File
>
third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp
> (left):
> > 
> >
>
https://codereview.chromium.org/2643203002/diff/1/third_party/WebKit/Source/p...
> >
>
third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp:292:
> scrollTree().SetScrollOffset(layerId,
> > With this removed, is the scroll offset getting set on the scroll tree at
all?
> (If not, I'd expect main-thread-driven scrolling to work, but
compositor-driven
> scrolling to be broken.)
> 
> We actually don't have any compositor scrolling working yet :/ I am hoping to
> add that next but we have some chicken-and-egg issues. Sadly, I just found
that
> scrolling is still completely broken on Windows (with and without this patch).
> 
> Do you have a suggestion for how this should be architected? Should we call
> scrollTree().SetScrollOffset only for compositor-driven scrolling? Or do we
need
> to set both?

Setting in for both seems like it might be simpler, just for the sake of having
consistent state in the scroll and transform trees.

> Similarly, do you think it makes sense to get main thread scrolling
> working first, or do we need to get them wired up together?

Making main-thread scroll work first seems totally reasonable :)  But perhaps
expand the comment in ensureCompositorScrollNode to mention that setting the
offset on the scroll node will eventually be needed.

[pdr., 2017-01-23 04:10:18]

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

[commit-bot: I haz the power, 2017-01-23 04:10:26]

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

[pdr., 2017-01-23 04:10:28]

Description was changed from

==========
Rewrite blink->cc scroll offset logic for SPV2

Blink and cc use slightly different transform nodes for scroll offset.
Blink creates a transform node just for scroll offset whereas cc has
a special field (scroll_offset) embedded in the transform node. To
account for this, PropertyTreeManager::updateScrollOffset adjusted the
cc transform node.

updateScrollOffset had a bug where the compositor node's scroll offset would
be set from the compositor node's transform, then the transform would
be zero'd. This was broken when updateScrollOffset was called a second
time because the newly-zero'd transform would be used to overwrite the
scroll offset.

Fixing this bug exposed a second bug where setting both the transform
node's scroll_offset and calling scrollTree().SetScrollOffset(...) would
lead to missed compositor-side scroll offset updates (this is tested in
fast/scrolling/fixed-position.html).

With this patch google.com is usable with SPV2 \o/

BUG=667946

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

to

==========
Use blink's scroll offset to update cc's transform scroll offset for SPV2

Blink and cc use slightly different transform nodes for scroll offset.
Blink creates a transform node just for scroll offset whereas cc has
a special field (scroll_offset) embedded in the transform node. To
account for this, PropertyTreeManager::updateScrollOffset adjusted the
cc transform node.

updateScrollOffset had a bug where the compositor node's scroll offset would
be set from the compositor node's transform, then the transform would
be zero'd. This was broken when updateScrollOffset was called a second
time because the newly-zero'd transform would be used to overwrite the
scroll offset.

Fixing this bug exposed a second bug where setting both the transform
node's scroll_offset and calling scrollTree().SetScrollOffset(...) would
lead to missed compositor-side scroll offset updates (this is tested in
fast/scrolling/fixed-position.html). The SetScrollOffset call has been
temporarily replaced by a TODO for a followup patch.

With this patch google.com is usable with SPV2 \o/

BUG=667946

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

[pdr., 2017-01-23 05:23:16]

On 2017/01/20 at 23:11:27, ajuma wrote:
> On 2017/01/20 23:01:40, pdr. wrote:
> > On 2017/01/20 at 21:35:48, ajuma wrote:
> > >
> >
https://codereview.chromium.org/2643203002/diff/1/third_party/WebKit/Source/p...
> > > File
> >
third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp
> > (left):
> > > 
> > >
> >
https://codereview.chromium.org/2643203002/diff/1/third_party/WebKit/Source/p...
> > >
> >
third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp:292:
> > scrollTree().SetScrollOffset(layerId,
> > > With this removed, is the scroll offset getting set on the scroll tree at
all?
> > (If not, I'd expect main-thread-driven scrolling to work, but
compositor-driven
> > scrolling to be broken.)
> > 
> > We actually don't have any compositor scrolling working yet :/ I am hoping
to
> > add that next but we have some chicken-and-egg issues. Sadly, I just found
that
> > scrolling is still completely broken on Windows (with and without this
patch).
> > 
> > Do you have a suggestion for how this should be architected? Should we call
> > scrollTree().SetScrollOffset only for compositor-driven scrolling? Or do we
need
> > to set both?
> 
> Setting in for both seems like it might be simpler, just for the sake of
having consistent state in the scroll and transform trees.
> 
> > Similarly, do you think it makes sense to get main thread scrolling
> > working first, or do we need to get them wired up together?
> 
> Making main-thread scroll work first seems totally reasonable :)  But perhaps
expand the comment in ensureCompositorScrollNode to mention that setting the
offset on the scroll node will eventually be needed.

+1. I added a TODO in the latest patch about re-adding this back, and refactored
the code back to PropertyTreeManager::updateScrollOffset where this can be done.
I've also updated the change description to reflect that this patch is really
just fixing the bug where multiple updates would result in zero-ing the scroll
offset value.

[commit-bot: I haz the power, 2017-01-23 05:44:44]

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

[commit-bot: I haz the power, 2017-01-23 05:44:45]

Dry run: This issue passed the CQ dry run.

[ajuma, 2017-01-23 14:36:16]

On 2017/01/23 05:23:16, pdr. wrote:
> On 2017/01/20 at 23:11:27, ajuma wrote:
> > On 2017/01/20 23:01:40, pdr. wrote:
> > > On 2017/01/20 at 21:35:48, ajuma wrote:
> > > >
> > >
>
https://codereview.chromium.org/2643203002/diff/1/third_party/WebKit/Source/p...
> > > > File
> > >
>
third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp
> > > (left):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/2643203002/diff/1/third_party/WebKit/Source/p...
> > > >
> > >
>
third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp:292:
> > > scrollTree().SetScrollOffset(layerId,
> > > > With this removed, is the scroll offset getting set on the scroll tree
at
> all?
> > > (If not, I'd expect main-thread-driven scrolling to work, but
> compositor-driven
> > > scrolling to be broken.)
> > > 
> > > We actually don't have any compositor scrolling working yet :/ I am hoping
> to
> > > add that next but we have some chicken-and-egg issues. Sadly, I just found
> that
> > > scrolling is still completely broken on Windows (with and without this
> patch).
> > > 
> > > Do you have a suggestion for how this should be architected? Should we
call
> > > scrollTree().SetScrollOffset only for compositor-driven scrolling? Or do
we
> need
> > > to set both?
> > 
> > Setting in for both seems like it might be simpler, just for the sake of
> having consistent state in the scroll and transform trees.
> > 
> > > Similarly, do you think it makes sense to get main thread scrolling
> > > working first, or do we need to get them wired up together?
> > 
> > Making main-thread scroll work first seems totally reasonable :)  But
perhaps
> expand the comment in ensureCompositorScrollNode to mention that setting the
> offset on the scroll node will eventually be needed.
> 
> +1. I added a TODO in the latest patch about re-adding this back, and
refactored
> the code back to PropertyTreeManager::updateScrollOffset where this can be
done.
> I've also updated the change description to reflect that this patch is really
> just fixing the bug where multiple updates would result in zero-ing the scroll
> offset value.

Thanks! lgtm

[pdr., 2017-01-23 17:32:53]

The CQ bit was checked by pdr@chromium.org

[pdr., 2017-01-23 17:32:53]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2643203002/#ps20001
(title: "Move code back under updateScrollOffset")

[commit-bot: I haz the power, 2017-01-23 17:33:20]

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-01-23 17:38:00]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485192773043060,
"parent_rev": "d8cc9d23b26dc31e050fdc1e2042f639d8a608fa", "commit_rev":
"de4408730000072669b7f4d27d0e5e8066988724"}

[commit-bot: I haz the power, 2017-01-23 17:39:21]

Description was changed from

==========
Use blink's scroll offset to update cc's transform scroll offset for SPV2

Blink and cc use slightly different transform nodes for scroll offset.
Blink creates a transform node just for scroll offset whereas cc has
a special field (scroll_offset) embedded in the transform node. To
account for this, PropertyTreeManager::updateScrollOffset adjusted the
cc transform node.

updateScrollOffset had a bug where the compositor node's scroll offset would
be set from the compositor node's transform, then the transform would
be zero'd. This was broken when updateScrollOffset was called a second
time because the newly-zero'd transform would be used to overwrite the
scroll offset.

Fixing this bug exposed a second bug where setting both the transform
node's scroll_offset and calling scrollTree().SetScrollOffset(...) would
lead to missed compositor-side scroll offset updates (this is tested in
fast/scrolling/fixed-position.html). The SetScrollOffset call has been
temporarily replaced by a TODO for a followup patch.

With this patch google.com is usable with SPV2 \o/

BUG=667946

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

to

==========
Use blink's scroll offset to update cc's transform scroll offset for SPV2

Blink and cc use slightly different transform nodes for scroll offset.
Blink creates a transform node just for scroll offset whereas cc has
a special field (scroll_offset) embedded in the transform node. To
account for this, PropertyTreeManager::updateScrollOffset adjusted the
cc transform node.

updateScrollOffset had a bug where the compositor node's scroll offset would
be set from the compositor node's transform, then the transform would
be zero'd. This was broken when updateScrollOffset was called a second
time because the newly-zero'd transform would be used to overwrite the
scroll offset.

Fixing this bug exposed a second bug where setting both the transform
node's scroll_offset and calling scrollTree().SetScrollOffset(...) would
lead to missed compositor-side scroll offset updates (this is tested in
fast/scrolling/fixed-position.html). The SetScrollOffset call has been
temporarily replaced by a TODO for a followup patch.

With this patch google.com is usable with SPV2 \o/

BUG=667946

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[commit-bot: I haz the power, 2017-01-23 17:39:26]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/de4408730000072669b7f4d27d0e...