Background attachment behavior update for rootlayer with root-layer-scrolls enabled

With root layer scrolling we should paint fixed background attachment into GraphicsLayer and local/scroll attachment into ScrollingContentslayer.

BUG=682246
TEST=All/BoxPaintInvalidatorTest.CompositedLayoutViewResize/1; All/BoxPaintInvalidatorTest.CompositedLayoutViewGradientResize/1
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2655023003
Cr-Commit-Position: refs/heads/master@{#450764}
Committed: https://chromium.googlesource.com/chromium/src/+/97f1bc8106b41307d0b02eae771daa5ef266ef4f

[yigu, 2017-01-25 20:36:35]

Description was changed from

==========
With root layer scrolling we should paint fixed background attachment into
GraphicsLayer and local/scroll attachment into ScrollingContentslayer.

BUG=682246
==========

to

==========
With root layer scrolling we should paint fixed background attachment into
GraphicsLayer and local/scroll attachment into ScrollingContentslayer.

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

[yigu, 2017-01-25 20:37:13]

Description was changed from

==========
With root layer scrolling we should paint fixed background attachment into
GraphicsLayer and local/scroll attachment into ScrollingContentslayer.

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

to

==========
With root layer scrolling we should paint fixed background attachment into
GraphicsLayer and local/scroll attachment into ScrollingContentslayer.

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

[yigu, 2017-01-25 22:05:14]

yigu@chromium.org changed reviewers:
+ chrishtr@chromium.org, flackr@chromium.org

[yigu, 2017-01-25 22:05:14]

yigu@chromium.org changed required reviewers:
+ chrishtr@chromium.org

[yigu, 2017-01-25 22:05:15]

Hi Chris,
PTAL. Not sure whether this is the correct way of doing so.
Thanks!

[flackr, 2017-01-26 00:03:16]

For the root layer, I'm a little confused about falling back to painting the
background to the GraphicsLayer when most of the code today (i.e. without root
layer scrolling) assumes that the background is painted into what is effectively
the scrolling contents layer and forces scroll on main and repainting if we have
fixed attachment backgrounds. I think we want to change backgroundPaintLocation
to reflect this (i.e. preferring the scrolling contents layer).

https://codereview.chromium.org/2655023003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2655023003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:2629: if
(RuntimeEnabledFeatures::rootLayerScrollingEnabled() || !isRootLayer()) {
I wonder if this check is really necessary? The scrollsOverflow() should be
false on the root layer if we don't have root layer scrolling right?

https://codereview.chromium.org/2655023003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:2636: location =
BackgroundPaintInGraphicsLayer;
This seems wrong, do we actually create a layer behind the scrolling contents
layer for negative z index layers on the root scroller?

If we do, and we paint a scrolling / local background into a non-scrolling layer
then we'd have to repaint on scroll.

[chrishtr, 2017-02-03 01:46:30]

What's the latest on this patch?

[yigu, 2017-02-03 02:08:35]

On 2017/02/03 01:46:30, chrishtr wrote:
> What's the latest on this patch?

There is no update since flackr's feedback. I'm still trying to understand the
machinery here. It looks like the current code breaks some "invalidation"
related test.

[yigu, 2017-02-13 01:36:13]

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

[commit-bot: I haz the power, 2017-02-13 01:36:17]

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-13 02:47:16]

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

[commit-bot: I haz the power, 2017-02-13 02:47:16]

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_...)

[yigu, 2017-02-15 01:59:40]

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

[commit-bot: I haz the power, 2017-02-15 02:01:06]

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

[yigu, 2017-02-15 02:02:38]

Hi Chris, thanks for the investigation! I made some changes to
BoxPaintInvalidatorTest based on the purpose of this patch. PTAL.

[yigu, 2017-02-15 02:03:57]

Description was changed from

==========
With root layer scrolling we should paint fixed background attachment into
GraphicsLayer and local/scroll attachment into ScrollingContentslayer.

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

to

==========
With root layer scrolling we should paint fixed background attachment into
GraphicsLayer and local/scroll attachment into ScrollingContentslayer.

BUG=682246
TEST=All/BoxPaintInvalidatorTest.CompositedLayoutViewResize/1;
All/BoxPaintInvalidatorTest.CompositedLayoutViewGradientResize/1
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-02-15 02:08:35]

https://codereview.chromium.org/2655023003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BoxPaintInvalidatorTest.cpp (right):

https://codereview.chromium.org/2655023003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPaintInvalidatorTest.cpp:243:
EXPECT_EQ(PaintInvalidationBackgroundOnScrollingContentsLayer,
Isn't this only for cases when --root-layer-scrolls?

[yigu, 2017-02-15 02:16:13]

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

[commit-bot: I haz the power, 2017-02-15 02:17:20]

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

[yigu, 2017-02-15 02:18:51]

On 2017/02/15 02:08:35, chrishtr wrote:
>
https://codereview.chromium.org/2655023003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/BoxPaintInvalidatorTest.cpp (right):
> 
>
https://codereview.chromium.org/2655023003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/BoxPaintInvalidatorTest.cpp:243:
> EXPECT_EQ(PaintInvalidationBackgroundOnScrollingContentsLayer,
> Isn't this only for cases when --root-layer-scrolls?

Yes. New patch uploaded. Same for
BoxPaintInvalidatorTest.CompositedLayoutViewGradientResize. The reasons are
different w, w/o root-layer-scrolls.

[chrishtr, 2017-02-15 02:21:46]

lgtm

[commit-bot: I haz the power, 2017-02-15 04:20:09]

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

[commit-bot: I haz the power, 2017-02-15 04:20:10]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[flackr, 2017-02-15 05:52:19]

Have you tested that fixed position root backgrounds work correctly?

https://codereview.chromium.org/2655023003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2655023003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:2608: location =
layoutObject()->backgroundPaintLocation();
Don't we still need to check scrollsOverflow? Otherwise we could return
BackgroundPaintInScrollingContentsLayer for non scrolling paint layers.

[yigu, 2017-02-15 16:00:38]

Thanks! fixed background seems fine with this patch. The condition check has
been updated.

https://codereview.chromium.org/2655023003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2655023003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:2608: location =
layoutObject()->backgroundPaintLocation();
On 2017/02/15 05:52:19, flackr wrote:
> Don't we still need to check scrollsOverflow? Otherwise we could return
> BackgroundPaintInScrollingContentsLayer for non scrolling paint layers.

Done.

[yigu, 2017-02-15 16:01:00]

The CQ bit was checked by yigu@chromium.org

[yigu, 2017-02-15 16:01:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2655023003/#ps80001
(title: "Update condition check")

[commit-bot: I haz the power, 2017-02-15 16:01:27]

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-15 19:21:07]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487174460746140,
"parent_rev": "59d6f53d51032a8b4b4568e2af635b21bd754f88", "commit_rev":
"97f1bc8106b41307d0b02eae771daa5ef266ef4f"}

[commit-bot: I haz the power, 2017-02-15 19:21:45]

Description was changed from

==========
With root layer scrolling we should paint fixed background attachment into
GraphicsLayer and local/scroll attachment into ScrollingContentslayer.

BUG=682246
TEST=All/BoxPaintInvalidatorTest.CompositedLayoutViewResize/1;
All/BoxPaintInvalidatorTest.CompositedLayoutViewGradientResize/1
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
With root layer scrolling we should paint fixed background attachment into
GraphicsLayer and local/scroll attachment into ScrollingContentslayer.

BUG=682246
TEST=All/BoxPaintInvalidatorTest.CompositedLayoutViewResize/1;
All/BoxPaintInvalidatorTest.CompositedLayoutViewGradientResize/1
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2655023003
Cr-Commit-Position: refs/heads/master@{#450764}
Committed:
https://chromium.googlesource.com/chromium/src/+/97f1bc8106b41307d0b02eae771d...
==========

[commit-bot: I haz the power, 2017-02-15 19:21:47]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/97f1bc8106b41307d0b02eae771d...