Only paint the background onto the scrolling contents layer if we have a scrolling contents layer.

Only paint the background onto the scrolling contents layer if we have a scrolling contents layer.

BUG=647169, 645949
TEST=CompositedLayerMappingTest.BackgroundPaintedIntoGraphicsLayerIfNotCompositedScrolling
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/dd59f478410b76764e227fe24642bae6f7b06b26
Cr-Commit-Position: refs/heads/master@{#419064}

[flackr, 2016-09-15 17:50:44]

Description was changed from

==========
Only paint the background onto the scrolling contents layer if we have a
scrolling contents layer.

BUG=647169
TEST=Manual
==========

to

==========
Only paint the background onto the scrolling contents layer if we have a
scrolling contents layer.

BUG=647169
TEST=Manual
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[flackr, 2016-09-15 17:58:25]

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

[flackr, 2016-09-15 17:58:26]

Hey,

I've discovered the problem. It's that we can composite only the root layer of a
scrolling element when it is promoted due to overlap, but we don't actually
enable composited scrolling. In this case, the code decides that the background
can be painted into the scrolling contents layer but none exists so we never
paint the background.

We would need this fix even with
--enable-blink-features=CompositeOpaqueScrollers for the cases where we wouldn't
auto-promote because despite being able to paint the background into the
scrolling contents layer it may not be opaque.

I'll try to come up with a simple test for this, just need to get a better
understanding of how to cause the promotion of just the overflow scroller as the
root.

[flackr, 2016-09-15 18:05:30]

On 2016/09/15 at 17:58:26, flackr wrote:
> Hey,
> 
> I've discovered the problem. It's that we can composite only the root layer of
a scrolling element when it is promoted due to overlap, but we don't actually
enable composited scrolling. In this case, the code decides that the background
can be painted into the scrolling contents layer but none exists so we never
paint the background.
> 
> We would need this fix even with
--enable-blink-features=CompositeOpaqueScrollers for the cases where we wouldn't
auto-promote because despite being able to paint the background into the
scrolling contents layer it may not be opaque.
> 
> I'll try to come up with a simple test for this, just need to get a better
understanding of how to cause the promotion of just the overflow scroller as the
root.

Also, I realize it may make more sense to rename
PaintLayer::shouldPaintBackgroundOntoScrollingContentsLayer to
PaintLayer::canPaintBackgroundOntoScrollingContentsLayer since we only actually
do so if we also happen to composite the scrolling contents layer, but this
would make the merge back more complicated.

[flackr, 2016-09-15 18:15:09]

Description was changed from

==========
Only paint the background onto the scrolling contents layer if we have a
scrolling contents layer.

BUG=647169
TEST=Manual
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Only paint the background onto the scrolling contents layer if we have a
scrolling contents layer.

BUG=647169,645949
TEST=Manual
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[flackr, 2016-09-15 18:16:32]

So I did a little more digging and it turns out this was the root cause of the
issue https://crbug.com/645949. We weren't creating a scrolling contents layer
for border radius because it requires a big clipping layer. I tested the espn.in
site and the problem is still fixed.

I renamed it to canPaint..., it wasn't as many instances as I thought it would
be.

[flackr, 2016-09-15 20:05:47]

Description was changed from

==========
Only paint the background onto the scrolling contents layer if we have a
scrolling contents layer.

BUG=647169,645949
TEST=Manual
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Only paint the background onto the scrolling contents layer if we have a
scrolling contents layer.

BUG=647169,645949
TEST=CompositedLayerMappingTest.BackgroundPaintedIntoGraphicsLayerIfNotCompositedScrolling
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Stephen Chennney, 2016-09-15 20:08:37]

schenney@chromium.org changed reviewers:
+ schenney@chromium.org

[Stephen Chennney, 2016-09-15 20:08:38]

Makes sense. LGTM

https://codereview.chromium.org/2346883003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2346883003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:304:
&& m_owningLayer.needsCompositedScrolling();
Indent.

And add a comment here about why composited scrolling is needed. Basically what
you have in the review comment below.

[chrishtr, 2016-09-15 20:15:47]

https://codereview.chromium.org/2346883003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2346883003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:304:
&& m_owningLayer.needsCompositedScrolling();
It's PaintLayerScrollableArea::usesCompositedScrolling that is the most direct
and foolproof. It's also used in some other places. FYI
I made the same mistake elsewhere as you are fixing in this CL, see:

https://codereview.chromium.org/2343963002

[Stephen Chennney, 2016-09-15 20:21:27]

On 2016/09/15 20:15:47, chrishtr wrote:
>
https://codereview.chromium.org/2346883003/diff/60001/third_party/WebKit/Sour...
> File
> third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
> (right):
> 
>
https://codereview.chromium.org/2346883003/diff/60001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:304:
> && m_owningLayer.needsCompositedScrolling();
> It's PaintLayerScrollableArea::usesCompositedScrolling that is the most direct
> and foolproof. It's also used in some other places. FYI
> I made the same mistake elsewhere as you are fixing in this CL, see:
> 
> https://codereview.chromium.org/2343963002

Ahh, I should have caught that. We should add a comment to
PaintLayer::needsCompositedScrolling indicating that it does not guarantee a
scrolling contents layer and pointing to the right method to call.

[flackr, 2016-09-15 20:28:37]

https://codereview.chromium.org/2346883003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2346883003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:304:
&& m_owningLayer.needsCompositedScrolling();
On 2016/09/15 at 20:08:37, Stephen Chennney wrote:
> Indent.

I'm confused, isn't that the correct indent for two lines that are part of the
same expression? Should I not wrap the first line?

> 
> And add a comment here about why composited scrolling is needed. Basically
what you have in the review comment below.

Done.

https://codereview.chromium.org/2346883003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2346883003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:304:
&& m_owningLayer.needsCompositedScrolling();
On 2016/09/15 at 20:15:47, chrishtr wrote:
> It's PaintLayerScrollableArea::usesCompositedScrolling that is the most direct
and foolproof. It's also used in some other places. FYI
> I made the same mistake elsewhere as you are fixing in this CL, see:
> 
> https://codereview.chromium.org/2343963002

Done. I can add it, but I shouldn't need to check if we have a scrollable area
given canPaintBackgroundOntoScrollingContentsLayer must have returned true
before we evaluate this.

[Stephen Chennney, 2016-09-15 20:32:43]

On 2016/09/15 20:28:37, flackr wrote:
>
https://codereview.chromium.org/2346883003/diff/40001/third_party/WebKit/Sour...
> File
> third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
> (right):
> 
>
https://codereview.chromium.org/2346883003/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:304:
> && m_owningLayer.needsCompositedScrolling();
> On 2016/09/15 at 20:08:37, Stephen Chennney wrote:
> > Indent.
> 
> I'm confused, isn't that the correct indent for two lines that are part of the
> same expression? Should I not wrap the first line?

You're right, I'm wrong. I failed to parse the line correctly when I wrote the
comment, then didn't correct it when I did parse the code correctly.

[chrishtr, 2016-09-15 20:33:50]

lgtm

[flackr, 2016-09-15 20:38:20]

The CQ bit was checked by flackr@chromium.org

[flackr, 2016-09-15 20:38:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
schenney@chromium.org
Link to the patchset: https://codereview.chromium.org/2346883003/#ps100001
(title: "Use PaintLayerScrollableArea::usesCompositedScrolling")

[commit-bot: I haz the power, 2016-09-15 20:38: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-15 20:43:04]

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

[commit-bot: I haz the power, 2016-09-15 20:43:05]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[flackr, 2016-09-15 21:11:39]

The CQ bit was checked by flackr@chromium.org

[flackr, 2016-09-15 21:11:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, schenney@chromium.org
Link to the patchset: https://codereview.chromium.org/2346883003/#ps120001
(title: "Merge with master")

[commit-bot: I haz the power, 2016-09-15 21:12:43]

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-16 01:09:41]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-16 01:11:16]

Description was changed from

==========
Only paint the background onto the scrolling contents layer if we have a
scrolling contents layer.

BUG=647169,645949
TEST=CompositedLayerMappingTest.BackgroundPaintedIntoGraphicsLayerIfNotCompositedScrolling
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Only paint the background onto the scrolling contents layer if we have a
scrolling contents layer.

BUG=647169,645949
TEST=CompositedLayerMappingTest.BackgroundPaintedIntoGraphicsLayerIfNotCompositedScrolling
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/dd59f478410b76764e227fe24642bae6f7b06b26
Cr-Commit-Position: refs/heads/master@{#419064}
==========

[commit-bot: I haz the power, 2016-09-16 01:11:17]

Patchset 7 (id:??) landed as
https://crrev.com/dd59f478410b76764e227fe24642bae6f7b06b26
Cr-Commit-Position: refs/heads/master@{#419064}