Paint local background colors onto foreground layer.

Paint local background colors onto foreground layer.

This patch adds support for drawing backgrounds into the scrolling content
layer. This allows us to render text with sub-pixel anti-aliasing when an
opaque background color is used meaning that we can now promote in these
cases, and correctly supports content-box clips on locally attached
backgrounds (see overflow-scroll-with-local-background.html test).

BUG=381840
TEST=compositing/overflow/overflow-scroll-with-local-background.html,compositing/overflow/overflow-scroll-with-transparent-background.html,compositing/overflow/overflow-scroll-with-opaque-background.html

Committed: https://crrev.com/9a2cdc8d96d94c736eb5547a7c8c541e58e5759f
Cr-Commit-Position: refs/heads/master@{#410797}

[chrishtr, 2016-07-29 18:20:19]

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

[chrishtr, 2016-07-29 18:20:20]

Is this CL ready for review?

[flackr, 2016-07-29 18:21:29]

On 2016/07/29 at 18:20:20, chrishtr wrote:
> Is this CL ready for review?

I'm just fixing the last few failing tests, but yes.

[bokan, 2016-07-29 19:22:53]

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

[bokan, 2016-07-29 19:22:54]

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

https://codereview.chromium.org/2196583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:56:
paintRect.move(-m_layoutBox.scrolledContentOffset());
I hit the ASSERTs in LayoutBox::scrolledContentOffset here on
http://bokan.ca/rootscroller.html (while painting a #greenbox). The reason is
that we end up in here even though the box being painted doesn't have overflow.
Doing this inside an if(m_layoutBox.hasOverflowClip()) seems to work but I think
the root problem is that we got here at all;  the box has background-attachment:
scroll but paintFlags has "BackgroundOntoForeground" set which I think shouldn't
happen in this patch.

I'm guessing that we're not clearing the flag in paintInfo properly as we paint
the children of the scrolling layer. OTOH, since the LayoutObjects in the
scrolling contents layer aren't getting painted into their own layer, perhaps
this is ok and the 'if' is all we need. I'm just starting to dabble in this area
so hopefully this made sense...

[flackr, 2016-07-29 19:28:17]

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

https://codereview.chromium.org/2196583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:56:
paintRect.move(-m_layoutBox.scrolledContentOffset());
On 2016/07/29 at 19:22:54, bokan wrote:
> I hit the ASSERTs in LayoutBox::scrolledContentOffset here on
http://bokan.ca/rootscroller.html (while painting a #greenbox). The reason is
that we end up in here even though the box being painted doesn't have overflow.
Doing this inside an if(m_layoutBox.hasOverflowClip()) seems to work but I think
the root problem is that we got here at all;  the box has background-attachment:
scroll but paintFlags has "BackgroundOntoForeground" set which I think shouldn't
happen in this patch.
> 
> I'm guessing that we're not clearing the flag in paintInfo properly as we
paint the children of the scrolling layer. OTOH, since the LayoutObjects in the
scrolling contents layer aren't getting painted into their own layer, perhaps
this is ok and the 'if' is all we need. I'm just starting to dabble in this area
so hopefully this made sense...

I think this is already fixed in the latest patch. The problem was that this
patch still propagates PaintLayerPaintingBackgroundOntoForeground to children
but shouldn't.

[bokan, 2016-07-29 19:29:00]

On 2016/07/29 19:28:17, flackr wrote:
>
https://codereview.chromium.org/2196583002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):
> 
>
https://codereview.chromium.org/2196583002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/BoxPainter.cpp:56:
> paintRect.move(-m_layoutBox.scrolledContentOffset());
> On 2016/07/29 at 19:22:54, bokan wrote:
> > I hit the ASSERTs in LayoutBox::scrolledContentOffset here on
> http://bokan.ca/rootscroller.html (while painting a #greenbox). The reason is
> that we end up in here even though the box being painted doesn't have
overflow.
> Doing this inside an if(m_layoutBox.hasOverflowClip()) seems to work but I
think
> the root problem is that we got here at all;  the box has
background-attachment:
> scroll but paintFlags has "BackgroundOntoForeground" set which I think
shouldn't
> happen in this patch.
> > 
> > I'm guessing that we're not clearing the flag in paintInfo properly as we
> paint the children of the scrolling layer. OTOH, since the LayoutObjects in
the
> scrolling contents layer aren't getting painted into their own layer, perhaps
> this is ok and the 'if' is all we need. I'm just starting to dabble in this
area
> so hopefully this made sense...
> 
> I think this is already fixed in the latest patch. The problem was that this
> patch still propagates PaintLayerPaintingBackgroundOntoForeground to children
> but shouldn't.

Ah, yes it's from the previous patch, you're way ahead of me :)

[chrishtr, 2016-07-29 21:04:34]

https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1794: if
(style()->canContainFixedPositionObjects() && !isFixedPos)
Spurious changes here?

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

https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:313:
// are background attachment local, otherwise background is painted by the
primary graphics layer.
Even with this CL, doesn't m_graphicsLayer still paint its background?

https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:54: // Need to include
overflow when drawing into the scrolling contents layer.
What about boxes that are scrolling but are not composited? They don't need
this.

https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintInfo.h (right):

https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInfo.h:83: result.m_paintFlags &=
~PaintLayerPaintingBackgroundOntoForeground;
Can you just reuse paintRootBackgroundOnly() instead? That appears to be solving
a similar use case that can be generalized.

[flackr, 2016-08-02 16:58:14]

https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1794: if
(style()->canContainFixedPositionObjects() && !isFixedPos)
On 2016/07/29 at 21:04:34, chrishtr wrote:
> Spurious changes here?

schenney suggested I move the comment in ... while I was touching LayoutBox.cpp
to be consistent with the comment in mapLocalToAncestor. Although I guess
currently I'm not otherwise touching LayoutBox.cpp, removed.

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

https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:313:
// are background attachment local, otherwise background is painted by the
primary graphics layer.
On 2016/07/29 at 21:04:34, chrishtr wrote:
> Even with this CL, doesn't m_graphicsLayer still paint its background?

No, it's either painted into m_scrollingContentsLayer or m_graphicsLayer (the
condition in shouldPaintBackground excludes it if we're painting the background
layer with PaintsBackgroundOntoForeground set). m_graphicsLayer will however
always paint the border. We'll probably want to have exceptions to this (like
when you have an opaque color non-local border-box background with alpha
transparent border) however until this case is handled I didn't think it was
worth risking the cost of double-painting the background which will never be
seen.

https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:54: // Need to include
overflow when drawing into the scrolling contents layer.
On 2016/07/29 at 21:04:34, chrishtr wrote:
> What about boxes that are scrolling but are not composited? They don't need
this.

PaintLayerPaintingBackgroundOntoForeground only gets applied for painting
composited scrollers (See CompositedLayerMapping.cpp:2437). The name is already
quite long but we could add "Compositing" to it :-)

https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintInfo.h (right):

https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInfo.h:83: result.m_paintFlags &=
~PaintLayerPaintingBackgroundOntoForeground;
On 2016/07/29 at 21:04:34, chrishtr wrote:
> Can you just reuse paintRootBackgroundOnly() instead? That appears to be
solving a similar use case that can be generalized.

Done.

[flackr, 2016-08-02 17:02:20]

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

[commit-bot: I haz the power, 2016-08-02 17:03:10]

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-08-02 18:32:51]

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

[commit-bot: I haz the power, 2016-08-02 18:32:52]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[flackr, 2016-08-02 18:37:03]

https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintInfo.h (right):

https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInfo.h:83: result.m_paintFlags &=
~PaintLayerPaintingBackgroundOntoForeground;
On 2016/08/02 at 16:58:14, flackr wrote:
> On 2016/07/29 at 21:04:34, chrishtr wrote:
> > Can you just reuse paintRootBackgroundOnly() instead? That appears to be
solving a similar use case that can be generalized.
> 
> Done.

Hmm, painting with RootBackgroundOnly and then again with SkipRootBackground
seems to be crashing: see failure in
compositing/overflow/overflow-scroll-with-local-background.html. I'm
investigating, will let you know when I find the cause and/or fix it.

[flackr, 2016-08-02 19:29:30]

On 2016/08/02 at 18:37:03, flackr wrote:
>
https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintInfo.h (right):
> 
>
https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintInfo.h:83: result.m_paintFlags &=
~PaintLayerPaintingBackgroundOntoForeground;
> On 2016/08/02 at 16:58:14, flackr wrote:
> > On 2016/07/29 at 21:04:34, chrishtr wrote:
> > > Can you just reuse paintRootBackgroundOnly() instead? That appears to be
solving a similar use case that can be generalized.
> > 
> > Done.
> 
> Hmm, painting with RootBackgroundOnly and then again with SkipRootBackground
seems to be crashing: see failure in
compositing/overflow/overflow-scroll-with-local-background.html. I'm
investigating, will let you know when I find the cause and/or fix it.

So it looks like RootBackgroundOnly refers only to the root layer's background
(i.e. the LayoutView). If you'd like I can try to repurpose this to mean the top
layer being currently painted and children layers but this may break other code
which is depending on it referring only to the root layer.

[flackr, 2016-08-02 19:41:44]

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

[commit-bot: I haz the power, 2016-08-02 19:42:22]

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

[chrishtr, 2016-08-02 20:13:34]

On 2016/08/02 at 19:29:30, flackr wrote:
> On 2016/08/02 at 18:37:03, flackr wrote:
> >
https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/paint/PaintInfo.h (right):
> > 
> >
https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/paint/PaintInfo.h:83: result.m_paintFlags &=
~PaintLayerPaintingBackgroundOntoForeground;
> > On 2016/08/02 at 16:58:14, flackr wrote:
> > > On 2016/07/29 at 21:04:34, chrishtr wrote:
> > > > Can you just reuse paintRootBackgroundOnly() instead? That appears to be
solving a similar use case that can be generalized.
> > > 
> > > Done.
> > 
> > Hmm, painting with RootBackgroundOnly and then again with SkipRootBackground
seems to be crashing: see failure in
compositing/overflow/overflow-scroll-with-local-background.html. I'm
investigating, will let you know when I find the cause and/or fix it.
> 
> So it looks like RootBackgroundOnly refers only to the root layer's background
(i.e. the LayoutView).

Correct, that is the current meaning.

> If you'd like I can try to repurpose this to mean the top layer being
currently painted and children layers but this may break other code which is
depending on it referring only to the root layer.

Yes, please try that. I think it may lead to cleaner overall code, rather than
two flags that have similar meaning.

[commit-bot: I haz the power, 2016-08-02 23:34:35]

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

[commit-bot: I haz the power, 2016-08-02 23:34:36]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[flackr, 2016-08-03 15:33:14]

https://codereview.chromium.org/2196583002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/2196583002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:752:
paintForegroundForFragmentsWithPhase(PaintPhaseDescendantBlockBackgroundsOnly,
layerFragments, context, localPaintingInfo, paintFlags &
~PaintLayerPaintingBackgroundOntoForeground, clipState);
I missed earlier that we don't descend into children from the above paint call,
we paint descendant backgrounds in a separate phase here. We can just remove
PaintLayerPaintingBackgroundOntoForeground here. WDYT?

[chrishtr, 2016-08-03 16:44:48]

https://codereview.chromium.org/2196583002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/2196583002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:752:
paintForegroundForFragmentsWithPhase(PaintPhaseDescendantBlockBackgroundsOnly,
layerFragments, context, localPaintingInfo, paintFlags &
~PaintLayerPaintingBackgroundOntoForeground, clipState);
On 2016/08/03 at 15:33:13, flackr wrote:
> I missed earlier that we don't descend into children from the above paint
call, we paint descendant backgrounds in a separate phase here. We can just
remove PaintLayerPaintingBackgroundOntoForeground here. WDYT?

Rename to PaintLayerPaintingRootBackgroundOntoForeground then?

[flackr, 2016-08-03 17:47:10]

https://codereview.chromium.org/2196583002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/2196583002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:752:
paintForegroundForFragmentsWithPhase(PaintPhaseDescendantBlockBackgroundsOnly,
layerFragments, context, localPaintingInfo, paintFlags &
~PaintLayerPaintingBackgroundOntoForeground, clipState);
On 2016/08/03 at 16:44:48, chrishtr wrote:
> On 2016/08/03 at 15:33:13, flackr wrote:
> > I missed earlier that we don't descend into children from the above paint
call, we paint descendant backgrounds in a separate phase here. We can just
remove PaintLayerPaintingBackgroundOntoForeground here. WDYT?
> 
> Rename to PaintLayerPaintingRootBackgroundOntoForeground then?

Done.

[flackr, 2016-08-03 18:38:59]

On 2016/08/03 at 16:44:48, chrishtr wrote:
>
https://codereview.chromium.org/2196583002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):
> 
>
https://codereview.chromium.org/2196583002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:752:
paintForegroundForFragmentsWithPhase(PaintPhaseDescendantBlockBackgroundsOnly,
layerFragments, context, localPaintingInfo, paintFlags &
~PaintLayerPaintingBackgroundOntoForeground, clipState);
> On 2016/08/03 at 15:33:13, flackr wrote:
> > I missed earlier that we don't descend into children from the above paint
call, we paint descendant backgrounds in a separate phase here. We can just
remove PaintLayerPaintingBackgroundOntoForeground here. WDYT?
> 
> Rename to PaintLayerPaintingRootBackgroundOntoForeground then?

Actually, since we only paint the local background in that one path we can just
set the flag specifically when we are in that case (see latest patch) rather
than propagate this flag for all of the paint calls.

[chrishtr, 2016-08-03 18:58:01]

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

https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:313:
// are background attachment local, otherwise background is painted by the
primary graphics layer.
On 2016/08/02 at 16:58:14, flackr wrote:
> On 2016/07/29 at 21:04:34, chrishtr wrote:
> > Even with this CL, doesn't m_graphicsLayer still paint its background?
> 
> No, it's either painted into m_scrollingContentsLayer or m_graphicsLayer (the
condition in shouldPaintBackground excludes it if we're painting the background
layer with PaintsBackgroundOntoForeground set). m_graphicsLayer will however
always paint the border. We'll probably want to have exceptions to this (like
when you have an opaque color non-local border-box background with alpha
transparent border) however until this case is handled I didn't think it was
worth risking the cost of double-painting the background which will never be
seen.

1. I don't see this exclusion in the current patch. The logic is:

 bool shouldPaintBackground = shouldPaintContent && !selectionOnly
            && (shouldPaintBackgroundOntoForeground ||
isPaintingCompositedBackground);

Where does this exclude? It is true only more often than before.

2. If the background is not painted onto the background or m_graphicsLayer
layers, then won't their opaqueness bit be invalid?

https://codereview.chromium.org/2196583002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/2196583002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:54: // Need to include
overflow when drawing into the scrolling contents layer.
Explicitly say in a comment that this code is specifically for the case of
painting the background of a composited
scroller into the scrolling contents layer.

https://codereview.chromium.org/2196583002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:75: (paintInfo.paintFlags()
& PaintLayerPaintingCompositingBackgroundPhase
This conditional duplicates lines 50-51. Factor into a helper.

https://codereview.chromium.org/2196583002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:110:
GraphicsContextStateSaver stateSaver(paintInfo.context, false);
Why do you need this?

https://codereview.chromium.org/2196583002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayer.h (right):

https://codereview.chromium.org/2196583002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayer.h:250: bool
shouldPaintBackgroundOntoForeground() const;
This needs a comment.

https://codereview.chromium.org/2196583002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerPaintingInfo.h (right):

https://codereview.chromium.org/2196583002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerPaintingInfo.h:73:
PaintLayerPaintingRootBackgroundOntoForeground = 1 << 14,
How about PaintLayerPaintingRootBackgroundOntoScrollingContentsLayer?

[flackr, 2016-08-03 20:58:53]

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

https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:313:
// are background attachment local, otherwise background is painted by the
primary graphics layer.
On 2016/08/03 at 18:58:00, chrishtr wrote:
> On 2016/08/02 at 16:58:14, flackr wrote:
> > On 2016/07/29 at 21:04:34, chrishtr wrote:
> > > Even with this CL, doesn't m_graphicsLayer still paint its background?
> > 
> > No, it's either painted into m_scrollingContentsLayer or m_graphicsLayer
(the condition in shouldPaintBackground excludes it if we're painting the
background layer with PaintsBackgroundOntoForeground set). m_graphicsLayer will
however always paint the border. We'll probably want to have exceptions to this
(like when you have an opaque color non-local border-box background with alpha
transparent border) however until this case is handled I didn't think it was
worth risking the cost of double-painting the background which will never be
seen.
> 
> 1. I don't see this exclusion in the current patch. The logic is:
> 
>  bool shouldPaintBackground = shouldPaintContent && !selectionOnly
>             && (shouldPaintBackgroundOntoForeground ||
isPaintingCompositedBackground);
> 
> Where does this exclude? It is true only more often than before.

We always paint the border and shadows into the m_graphicsLayer, but in
BoxPainter.cpp these lines should only paint the actual background into one of
the layers:
bool paintBackgroundOnly = !(paintInfo.paintFlags() &
PaintLayerPaintingCompositingBackgroundPhase)
        && (paintInfo.paintFlags() &
PaintLayerPaintingRootBackgroundOntoForeground);
bool shouldPaintBackground = !themePainted
    && (paintBackgroundOnly || !(paintInfo.paintFlags() &
PaintLayerPaintingRootBackgroundOntoForeground));

Basically paintBackgroundOnly should be true when we're painting onto the
composited foreground, and the painting into background layer only happens when
we are not going to paint into the composited foreground.

> 
> 2. If the background is not painted onto the background or m_graphicsLayer
layers, then won't their opaqueness bit be invalid?

Yes, it's a bit of a lie but only for a region which is completely obscured.
I.e. we don't paint the background region but it should be covered up completely
by the opaque scrolling contents layer.

https://codereview.chromium.org/2196583002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/2196583002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:54: // Need to include
overflow when drawing into the scrolling contents layer.
On 2016/08/03 at 18:58:00, chrishtr wrote:
> Explicitly say in a comment that this code is specifically for the case of
painting the background of a composited
> scroller into the scrolling contents layer.

Done.

https://codereview.chromium.org/2196583002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:75: (paintInfo.paintFlags()
& PaintLayerPaintingCompositingBackgroundPhase
On 2016/08/03 at 18:58:00, chrishtr wrote:
> This conditional duplicates lines 50-51. Factor into a helper.

Done.

https://codereview.chromium.org/2196583002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:110:
GraphicsContextStateSaver stateSaver(paintInfo.context, false);
On 2016/08/03 at 18:58:00, chrishtr wrote:
> Why do you need this?

It's moved from line 92. Putting it inside the condition incorrectly scopes its
lifetime to be destructed before the end of this method.

https://codereview.chromium.org/2196583002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayer.h (right):

https://codereview.chromium.org/2196583002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayer.h:250: bool
shouldPaintBackgroundOntoForeground() const;
On 2016/08/03 at 18:58:00, chrishtr wrote:
> This needs a comment.

Done.

https://codereview.chromium.org/2196583002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerPaintingInfo.h (right):

https://codereview.chromium.org/2196583002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerPaintingInfo.h:73:
PaintLayerPaintingRootBackgroundOntoForeground = 1 << 14,
On 2016/08/03 at 18:58:00, chrishtr wrote:
> How about PaintLayerPaintingRootBackgroundOntoScrollingContentsLayer?

Sure, should I replace foreground elsewhere with scrolling contents (layer)? as
well?

[chrishtr, 2016-08-04 05:20:52]

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

https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:313:
// are background attachment local, otherwise background is painted by the
primary graphics layer.
On 2016/08/03 at 20:58:52, flackr wrote:
> On 2016/08/03 at 18:58:00, chrishtr wrote:
> > On 2016/08/02 at 16:58:14, flackr wrote:
> > > On 2016/07/29 at 21:04:34, chrishtr wrote:
> > > > Even with this CL, doesn't m_graphicsLayer still paint its background?
> > > 
> > > No, it's either painted into m_scrollingContentsLayer or m_graphicsLayer
(the condition in shouldPaintBackground excludes it if we're painting the
background layer with PaintsBackgroundOntoForeground set). m_graphicsLayer will
however always paint the border. We'll probably want to have exceptions to this
(like when you have an opaque color non-local border-box background with alpha
transparent border) however until this case is handled I didn't think it was
worth risking the cost of double-painting the background which will never be
seen.
> > 
> > 1. I don't see this exclusion in the current patch. The logic is:
> > 
> >  bool shouldPaintBackground = shouldPaintContent && !selectionOnly
> >             && (shouldPaintBackgroundOntoForeground ||
isPaintingCompositedBackground);
> > 
> > Where does this exclude? It is true only more often than before.
> 
> We always paint the border and shadows into the m_graphicsLayer, but in
BoxPainter.cpp these lines should only paint the actual background into one of
the layers:
> bool paintBackgroundOnly = !(paintInfo.paintFlags() &
PaintLayerPaintingCompositingBackgroundPhase)
>         && (paintInfo.paintFlags() &
PaintLayerPaintingRootBackgroundOntoForeground);
> bool shouldPaintBackground = !themePainted
>     && (paintBackgroundOnly || !(paintInfo.paintFlags() &
PaintLayerPaintingRootBackgroundOntoForeground));
> 
> Basically paintBackgroundOnly should be true when we're painting onto the
composited foreground, and the painting into background layer only happens when
we are not going to paint into the composited foreground.

I still don't see the logic that leads to this...

but in any case, I don't see the harm in painting the background twice. In fact,
it avoids the danger of under-painting layers
which must be opaque. Is there a downside?

> 
> > 
> > 2. If the background is not painted onto the background or m_graphicsLayer
layers, then won't their opaqueness bit be invalid?
> 
> Yes, it's a bit of a lie but only for a region which is completely obscured.
I.e. we don't paint the background region but it should be covered up completely
by the opaque scrolling contents layer.

[flackr, 2016-08-04 13:03:30]

On 2016/08/04 at 05:20:52, chrishtr wrote:
>
https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):
> 
>
https://codereview.chromium.org/2196583002/diff/80001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:313:
// are background attachment local, otherwise background is painted by the
primary graphics layer.
> On 2016/08/03 at 20:58:52, flackr wrote:
> > On 2016/08/03 at 18:58:00, chrishtr wrote:
> > > On 2016/08/02 at 16:58:14, flackr wrote:
> > > > On 2016/07/29 at 21:04:34, chrishtr wrote:
> > > > > Even with this CL, doesn't m_graphicsLayer still paint its background?
> > > > 
> > > > No, it's either painted into m_scrollingContentsLayer or m_graphicsLayer
(the condition in shouldPaintBackground excludes it if we're painting the
background layer with PaintsBackgroundOntoForeground set). m_graphicsLayer will
however always paint the border. We'll probably want to have exceptions to this
(like when you have an opaque color non-local border-box background with alpha
transparent border) however until this case is handled I didn't think it was
worth risking the cost of double-painting the background which will never be
seen.
> > > 
> > > 1. I don't see this exclusion in the current patch. The logic is:
> > > 
> > >  bool shouldPaintBackground = shouldPaintContent && !selectionOnly
> > >             && (shouldPaintBackgroundOntoForeground ||
isPaintingCompositedBackground);
> > > 
> > > Where does this exclude? It is true only more often than before.
> > 
> > We always paint the border and shadows into the m_graphicsLayer, but in
BoxPainter.cpp these lines should only paint the actual background into one of
the layers:
> > bool paintBackgroundOnly = !(paintInfo.paintFlags() &
PaintLayerPaintingCompositingBackgroundPhase)
> >         && (paintInfo.paintFlags() &
PaintLayerPaintingRootBackgroundOntoForeground);
> > bool shouldPaintBackground = !themePainted
> >     && (paintBackgroundOnly || !(paintInfo.paintFlags() &
PaintLayerPaintingRootBackgroundOntoForeground));
> > 
> > Basically paintBackgroundOnly should be true when we're painting onto the
composited foreground, and the painting into background layer only happens when
we are not going to paint into the composited foreground.
> 
> I still don't see the logic that leads to this...

So when we have a composited local background scroller we call
BoxPainter::paintBoxDecorationBackgroundWithRect twice
- once for the background with paintFlags =
PaintLayerPaintingCompositingBackgroundPhase |
PaintLayerPaintingRootBackgroundOntoScrollingContentsLayer
- once for the foreground with paintFlags =
PaintLayerPaintingCompositingForegroundPhase |
PaintLayerPaintingRootBackgroundOntoScrollingContentsLayer

In the background call (with values substituted in):
bool paintBackgroundOnly = !(paintInfo.paintFlags() &
PaintLayerPaintingCompositingBackgroundPhase /* true */)
    && (paintInfo.paintFlags() & PaintLayerPaintingRootBackgroundOntoForeground
/* true */);

So paintBackgroundOnly = false;

bool shouldPaintBackground = !(themePainted /* false */)
    && (paintBackgroundOnly /* false */ || !(paintInfo.paintFlags() &
PaintLayerPaintingRootBackgroundOntoForeground /* true */));

Making shouldPaintBackground = false, so we don't get into the actual background
painting code:
if (shouldPaintBackground) {
    paintBackground(paintInfo, paintRect, boxDecorationData.backgroundColor,
boxDecorationData.bleedAvoidance);
    if (boxDecorationData.hasAppearance)
        themePainter.paintDecorations(m_layoutBox, paintInfo, snappedPaintRect);
}

> 
> but in any case, I don't see the harm in painting the background twice. In
fact, it avoids the danger of under-painting layers
> which must be opaque. Is there a downside?

It's mainly a cost thing. I didn't think we should paint something that will
never be seen. It might be negligible with solid color but I suspect the cost
may be noticeable when we start painting computed backgrounds (gradients) or
large images. This also might be important when the background layer doesn't
actually paint anything else (i.e. no border, shadow, etc) and painting the
background into it would cause it to require an actual texture rather than being
a transparent layer.

It's worth noting, there are also cases where we probably don't want to paint
the background onto the scrolling contents layer even if we could. For example,
if the scrollingContentsLayer doesn't actually have any drawn content then it
would be better to keep it that way so that we don't create a texture for it.
I'd like to start with this simple version first and worry about more complex
rules in follow-on patches.

> 
> > 
> > > 
> > > 2. If the background is not painted onto the background or m_graphicsLayer
layers, then won't their opaqueness bit be invalid?
> > 
> > Yes, it's a bit of a lie but only for a region which is completely obscured.
I.e. we don't paint the background region but it should be covered up completely
by the opaque scrolling contents layer.

[flackr, 2016-08-04 21:42:36]

As per our discussion I've done the following:
1. CLM explicitly sets the bit about whether we paint the root background.
Additionally, since this looks very similar to the
PaintLayerPaintingSkipRootBackground flag I've re-used it as you were suggesting
earlier.
2. Opaqueness of m_graphicsLayer is set to false when we paint into the
scrolling contents layer.
3. I've used the same primary flag as shouldRespectOverflowClip
(PaintLayerPaintingOverflowContents) but because we only want to apply the
special rules when painting the root layer I had to add this additional check.

[chrishtr, 2016-08-05 00:40:09]

https://codereview.chromium.org/2196583002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2196583002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2453:
bool paintRootBackgroundIntoScrollingContentsLayer =
m_owningLayer.shouldPaintBackgroundOntoScrollingContentsLayer();
Add:

DCHECK(!paintRootBackgroundIntoScrollingContentsLayer || (!m_backgroundLayer &&
!m_foregroundLayer))

https://codereview.chromium.org/2196583002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/2196583002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:40: bool
isPaintingScrollingContentsLayerBackground(const LayoutBoxModelObject* obj,
const PaintInfo& paintInfo)
isPaintingBackgroundOfPaintContainerIntoScrollingContentsLayer. Kind of long but
the current name is not quite right..

https://codereview.chromium.org/2196583002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayer.h (right):

https://codereview.chromium.org/2196583002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayer.h:255: bool
shouldPaintBackgroundOntoScrollingContentsLayer() const;
Move this method to CompositedLayerMapping.

https://codereview.chromium.org/2196583002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/2196583002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:386: &&
(isPaintingCompositedBackground || (isPaintingRootLayer && !(paintFlags &
PaintLayerPaintingSkipRootBackground)));
Do you need isPaintingCompositedBackground now?

[flackr, 2016-08-08 18:01:30]

https://codereview.chromium.org/2196583002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2196583002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2453:
bool paintRootBackgroundIntoScrollingContentsLayer =
m_owningLayer.shouldPaintBackgroundOntoScrollingContentsLayer();
On 2016/08/05 at 00:40:08, chrishtr wrote:
> Add:
> 
> DCHECK(!paintRootBackgroundIntoScrollingContentsLayer || (!m_backgroundLayer
&& !m_foregroundLayer))

Done.

https://codereview.chromium.org/2196583002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/2196583002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:40: bool
isPaintingScrollingContentsLayerBackground(const LayoutBoxModelObject* obj,
const PaintInfo& paintInfo)
On 2016/08/05 at 00:40:08, chrishtr wrote:
> isPaintingBackgroundOfPaintContainerIntoScrollingContentsLayer. Kind of long
but the current name is not quite right..

Done.

https://codereview.chromium.org/2196583002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayer.h (right):

https://codereview.chromium.org/2196583002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayer.h:255: bool
shouldPaintBackgroundOntoScrollingContentsLayer() const;
On 2016/08/05 at 00:40:08, chrishtr wrote:
> Move this method to CompositedLayerMapping.

Done.

https://codereview.chromium.org/2196583002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/2196583002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:386: &&
(isPaintingCompositedBackground || (isPaintingRootLayer && !(paintFlags &
PaintLayerPaintingSkipRootBackground)));
On 2016/08/05 at 00:40:08, chrishtr wrote:
> Do you need isPaintingCompositedBackground now?

Yes, we still need to call paintBackgroundForFragments on
isPaintingCompositedBackground whether or not we are going to paint the actual
background in order to paint the borders, box shadow, etc.

[chrishtr, 2016-08-08 21:16:40]

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-background-and-child-expected.html
(right):

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-background-and-child-expected.html:23:
<div id="scroller">
Add a comment explaining what this is testing.

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-background-and-child.html
(right):

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-background-and-child.html:24:
<div id="scroller">
Add a comment explaining what this is testing.

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-background-expected.html
(right):

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-background-expected.html:20:
<div id="scroller">
Add a comment explaining what this is testing.

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-background.html
(right):

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-background.html:6:
requestAnimationFrame(function() {
Do you know why it takes two rAFs in this case?

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-background.html:30:
<div id="scroller">
Add a comment explaining what this is testing.

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-opaque-background.html
(right):

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-opaque-background.html:6:
document.getElementById('layertree').innerText =
window.internals.layerTreeAsText(document);
Please parse this into JSON and check the contentsOpaque value on the scrolling
contents layer explicitly.
Similar example:

https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/compositi...

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-transparent-background.html
(right):

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-transparent-background.html:6:
document.getElementById('layertree').innerText =
window.internals.layerTreeAsText(document);
ditto

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2574:
bool CompositedLayerMapping::shouldPaintBackgroundOntoScrollingContentsLayer()
const
Unittest this method with a few cases. Those cases may obsolete some of your
layout tests...

[chrishtr, 2016-08-08 21:16:51]

Code is looking good, remaining comments are about tests.

[flackr, 2016-08-09 18:35:27]

Thanks for the reviews! The tests look cleaner :-)

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-background-and-child-expected.html
(right):

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-background-and-child-expected.html:23:
<div id="scroller">
On 2016/08/08 at 21:16:39, chrishtr wrote:
> Add a comment explaining what this is testing.

Done.

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-background-and-child.html
(right):

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-background-and-child.html:24:
<div id="scroller">
On 2016/08/08 at 21:16:39, chrishtr wrote:
> Add a comment explaining what this is testing.

Done.

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-background-expected.html
(right):

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-background-expected.html:20:
<div id="scroller">
On 2016/08/08 at 21:16:39, chrishtr wrote:
> Add a comment explaining what this is testing.

Done.

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-background.html
(right):

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-background.html:6:
requestAnimationFrame(function() {
On 2016/08/08 at 21:16:39, chrishtr wrote:
> Do you know why it takes two rAFs in this case?

yes, requestAnimationFrame fires at the beginning of the frame before we have
actually painted anything. Doing it twice ensures that we scroll on the second
frame after having already painted the scroller in its initial position. This
trick has been used in many other tests and I think is also used by external
developers when they want to wait for something to have been painted.

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-background.html:30:
<div id="scroller">
On 2016/08/08 at 21:16:39, chrishtr wrote:
> Add a comment explaining what this is testing.

Done.

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-opaque-background.html
(right):

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-opaque-background.html:6:
document.getElementById('layertree').innerText =
window.internals.layerTreeAsText(document);
On 2016/08/08 at 21:16:39, chrishtr wrote:
> Please parse this into JSON and check the contentsOpaque value on the
scrolling contents layer explicitly.
> Similar example:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/compositi...

Done, thanks for the example.

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-transparent-background.html
(right):

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-transparent-background.html:6:
document.getElementById('layertree').innerText =
window.internals.layerTreeAsText(document);
On 2016/08/08 at 21:16:40, chrishtr wrote:
> ditto

Done.

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2196583002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2574:
bool CompositedLayerMapping::shouldPaintBackgroundOntoScrollingContentsLayer()
const
On 2016/08/08 at 21:16:40, chrishtr wrote:
> Unittest this method with a few cases. Those cases may obsolete some of your
layout tests...

Done, but I think we should keep all of the added layout tests as they are
testing that the actual painting and layer opacities are done correctly.

[flackr, 2016-08-09 18:55:38]

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

[commit-bot: I haz the power, 2016-08-09 18:56:14]

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

[chrishtr, 2016-08-09 20:14:46]

lgtm

[commit-bot: I haz the power, 2016-08-09 20:22:42]

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

[commit-bot: I haz the power, 2016-08-09 20:22:43]

Dry run: This issue passed the CQ dry run.

[flackr, 2016-08-09 20:23:17]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2016-08-09 20:23:54]

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-08-09 20:30:29]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-08-09 20:33:45]

Description was changed from

==========
Paint local background colors onto foreground layer.

This patch adds support for drawing backgrounds into the scrolling content
layer. This allows us to render text with sub-pixel anti-aliasing when an
opaque background color is used meaning that we can now promote in these
cases, and correctly supports content-box clips on locally attached
backgrounds (see overflow-scroll-with-local-background.html test).

BUG=381840
TEST=compositing/overflow/overflow-scroll-with-local-background.html,compositing/overflow/overflow-scroll-with-transparent-background.html,compositing/overflow/overflow-scroll-with-opaque-background.html
==========

to

==========
Paint local background colors onto foreground layer.

This patch adds support for drawing backgrounds into the scrolling content
layer. This allows us to render text with sub-pixel anti-aliasing when an
opaque background color is used meaning that we can now promote in these
cases, and correctly supports content-box clips on locally attached
backgrounds (see overflow-scroll-with-local-background.html test).

BUG=381840
TEST=compositing/overflow/overflow-scroll-with-local-background.html,compositing/overflow/overflow-scroll-with-transparent-background.html,compositing/overflow/overflow-scroll-with-opaque-background.html

Committed: https://crrev.com/9a2cdc8d96d94c736eb5547a7c8c541e58e5759f
Cr-Commit-Position: refs/heads/master@{#410797}
==========

[commit-bot: I haz the power, 2016-08-09 20:33:46]

Patchset 15 (id:??) landed as
https://crrev.com/9a2cdc8d96d94c736eb5547a7c8c541e58e5759f
Cr-Commit-Position: refs/heads/master@{#410797}