Add background attachment fixed main thread scrolling reason [spv2]

Add background attachment fixed main thread scrolling reason [spv2]

This patch adds main thread scrolling reasons to blink's scroll nodes
and plumbs them to the compositor.

The approach taken in this patch is to match the semantics of cc's
scroll_tree so that main thread scrolling reasons are propagated up
the scroll tree.

Incremental updates are not yet possible but TODOs and tests have been
updated to ensure the main thread scroll reasons are recomputed.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
BUG=644514
Committed: https://crrev.com/6da0810c8c2362fc73d911750400ccd5c33cd6ca
Cr-Commit-Position: refs/heads/master@{#419614}

[pdr., 2016-09-17 05:00:19]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org, trchen@chromium.org

[pdr., 2016-09-17 05:00:29]

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

[ajuma, 2016-09-19 14:38:02]

https://codereview.chromium.org/2345403003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):

https://codereview.chromium.org/2345403003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2048:
EXPECT_FALSE(overflowB->layoutObject()->objectPaintProperties()->scroll()->hasBackgroundAttachmentFixedMainThreadScrollingReason());
Should overflowB also have a main-thread scrolling reason?

[chrishtr, 2016-09-19 17:00:12]

https://codereview.chromium.org/2345403003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.h
(right):

https://codereview.chromium.org/2345403003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.h:78:
void setBackgroundAttachmentFixedMainThreadScrollingReason()
How about:

void addMainThreadScrollingReason(MainThreadScrollingReason reason)
{
  m_mainThreadScrollingReasons |= reason;
}

and

bool hasSpecifiedMainThreadScrollingReason(MainThreadScrollingReason reason)
{
  rreturn m_mainThreadScrollingReasons & reason;
}

This would generalize beyond background-attachment: fixed

[pdr., 2016-09-19 20:25:50]

https://codereview.chromium.org/2345403003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):

https://codereview.chromium.org/2345403003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2048:
EXPECT_FALSE(overflowB->layoutObject()->objectPaintProperties()->scroll()->hasBackgroundAttachmentFixedMainThreadScrollingReason());
On 2016/09/19 at 14:38:02, ajuma wrote:
> Should overflowB also have a main-thread scrolling reason?

Good question! The background of overflowB is painted using the parent's
(overflowA) scroll node and scroll offset transform because they are not
affected by overflowB's scroll position. For this reason, we actually don't set
the main thread reason on overflowB, just the parent space where the background
is painted.

Another way of looking at this is through the transform tree for overflowB:
[ paintOffsetTranslation ]
|
+---[ transform ] < background painted here
    |
    +---[ perspective ]
        |
        +---[ scrollTranslation ] < scroll context setup here

https://codereview.chromium.org/2345403003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.h
(right):

https://codereview.chromium.org/2345403003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.h:78:
void setBackgroundAttachmentFixedMainThreadScrollingReason()
On 2016/09/19 at 17:00:12, chrishtr wrote:
> How about:
> 
> void addMainThreadScrollingReason(MainThreadScrollingReason reason)
> {
>   m_mainThreadScrollingReasons |= reason;
> }
> 
> and
> 
> bool hasSpecifiedMainThreadScrollingReason(MainThreadScrollingReason reason)
> {
>   rreturn m_mainThreadScrollingReasons & reason;
> }
> 
> This would generalize beyond background-attachment: fixed

+1, was going in this direction in the followup patch. Updated to do it here. To
keep the names short and reduce casts, I used the following names:
bool hasMainThreadScrollingReasons(MainThreadScrollingReasons reasons) const
void addMainThreadScrollingReasons(MainThreadScrollingReasons reasons)

[pdr., 2016-09-19 20:25:57]

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

[commit-bot: I haz the power, 2016-09-19 20:26:36]

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

[ajuma, 2016-09-19 20:31:06]

On 2016/09/19 20:25:50, pdr. wrote:
>
https://codereview.chromium.org/2345403003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
> (right):
> 
>
https://codereview.chromium.org/2345403003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2048:
>
EXPECT_FALSE(overflowB->layoutObject()->objectPaintProperties()->scroll()->hasBackgroundAttachmentFixedMainThreadScrollingReason());
> On 2016/09/19 at 14:38:02, ajuma wrote:
> > Should overflowB also have a main-thread scrolling reason?
> 
> Good question! The background of overflowB is painted using the parent's
> (overflowA) scroll node and scroll offset transform because they are not
> affected by overflowB's scroll position. For this reason, we actually don't
set
> the main thread reason on overflowB, just the parent space where the
background
> is painted.
> 
> Another way of looking at this is through the transform tree for overflowB:
> [ paintOffsetTranslation ]
> |
> +---[ transform ] < background painted here
>     |
>     +---[ perspective ]
>         |
>         +---[ scrollTranslation ] < scroll context setup here

Thanks! lgtm

[chrishtr, 2016-09-19 20:32:53]

lgtm

[pdr., 2016-09-19 20:39:34]

The CQ bit was unchecked by pdr@chromium.org

[pdr., 2016-09-19 20:39:37]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2016-09-19 20:39:57]

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, 2016-09-19 21:21:03]

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

[commit-bot: I haz the power, 2016-09-19 21:21:05]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pdr., 2016-09-19 21:36:45]

https://codereview.chromium.org/2345403003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp
(right):

https://codereview.chromium.org/2345403003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp:621:
// The main thread scrolling bits from scrollNodeB should have been propagated
to scrollNodeA.
The CQ failed because of this test. This was written for a previous version of
this patch where PaintartifactCompositor was responsible for propagating main
thread scrolling bits up the tree. This is now handled in
PaintPropertyTreeBuilder so this test has been updated to only check that the
main thread scrolling bits are propagated to the compositor.

[pdr., 2016-09-19 21:37:03]

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

[commit-bot: I haz the power, 2016-09-19 21:37:54]

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, 2016-09-20 00:41:54]

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

[commit-bot: I haz the power, 2016-09-20 00:41:55]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-09-20 00:43:59]

The CQ bit was checked by pdr@chromium.org

[pdr., 2016-09-20 00:43:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2345403003/#ps60001
(title: "Update test that was written for PaintArtifactCompositor's reasons
update")

[commit-bot: I haz the power, 2016-09-20 00:44:39]

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, 2016-09-20 00:51:13]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-20 00:55:13]

Description was changed from

==========
Add background attachment fixed main thread scrolling reason [spv2]

This patch adds main thread scrolling reasons to blink's scroll nodes
and plumbs them to the compositor.

The approach taken in this patch is to match the semantics of cc's
scroll_tree so that main thread scrolling reasons are propagated up
the scroll tree.

Incremental updates are not yet possible but TODOs and tests have been
updated to ensure the main thread scroll reasons are recomputed.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
BUG=644514
==========

to

==========
Add background attachment fixed main thread scrolling reason [spv2]

This patch adds main thread scrolling reasons to blink's scroll nodes
and plumbs them to the compositor.

The approach taken in this patch is to match the semantics of cc's
scroll_tree so that main thread scrolling reasons are propagated up
the scroll tree.

Incremental updates are not yet possible but TODOs and tests have been
updated to ensure the main thread scroll reasons are recomputed.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
BUG=644514

Committed: https://crrev.com/6da0810c8c2362fc73d911750400ccd5c33cd6ca
Cr-Commit-Position: refs/heads/master@{#419614}
==========

[commit-bot: I haz the power, 2016-09-20 00:55:14]

Patchset 4 (id:??) landed as
https://crrev.com/6da0810c8c2362fc73d911750400ccd5c33cd6ca
Cr-Commit-Position: refs/heads/master@{#419614}