Paint local attachment backgrounds into composited scrolling contents layer

Paint local attachment backgrounds into composited scrolling contents layer

This change detects when we can paint the background onto the scrolling contents layer. This will unblock promoting scrollers with supported backgrounds on low DPI devices as we can now maintain subpixel antialiased text.

BUG=381840, 568847
TEST=compositing/overflow/overflow-scroll-with-local-image-background.html, LayoutBoxTest.BackgroundRect, CompositedLayerMappingTest.ShouldPaintBackgroundOntoScrollingContentsLayer
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/05ee9971df08ffd2e57af7e6258a2b3efab9b539
Cr-Commit-Position: refs/heads/master@{#414795}

[Stephen Chennney, 2016-06-20 18:21:44]

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

[commit-bot: I haz the power, 2016-06-20 18:21:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2068723002/1

[commit-bot: I haz the power, 2016-06-20 18:24:51]

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

[commit-bot: I haz the power, 2016-06-20 18:24:52]

Dry run: Try jobs failed on following builders:
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Stephen Chennney, 2016-06-21 20:40:45]

Are you back to working on this? Otherwise I'll start as this is blocking my
most important goal.

[flackr, 2016-06-22 19:48:15]

On 2016/06/21 at 20:40:45, schenney wrote:
> Are you back to working on this? Otherwise I'll start as this is blocking my
most important goal.

I am still working on this, I've just had a busy couple weeks. I don't expect to
get too much done until Friday when I'm done sheriffing. I think the next step
is to add a PaintPhase for something like BackgroundPaintedOnScrollingContents
and replace all of the check's that I have for !PaintingCompositedBackground
with that flag. I also think this can be split up into two patches, one which
handles solid colors and one which extends that with background attachment local
handling. Then, modulo bugs and tests it may be ready to land.

[Stephen Chennney, 2016-06-22 20:19:30]

On 2016/06/22 19:48:15, flackr wrote:
> On 2016/06/21 at 20:40:45, schenney wrote:
> > Are you back to working on this? Otherwise I'll start as this is blocking my
> most important goal.
> 
> I am still working on this, I've just had a busy couple weeks. I don't expect
to
> get too much done until Friday when I'm done sheriffing. I think the next step
> is to add a PaintPhase for something like BackgroundPaintedOnScrollingContents
> and replace all of the check's that I have for !PaintingCompositedBackground
> with that flag. I also think this can be split up into two patches, one which
> handles solid colors and one which extends that with background attachment
local
> handling. Then, modulo bugs and tests it may be ready to land.

No problem. I'll keep looking at it anyway because I'm hoping to learn how this
all works. My version (differs only in being rebased) is at
https://codereview.chromium.org/2082153002

[Stephen Chennney, 2016-06-23 16:04:00]

Real failures:
compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html:
The focus outline is not anti-aliased or otherwise incorrectly drawn in the
inside edge. Probably being chopped by background painting.
compositing/overflow/update-widget-positions-on-nested-frames-and-scrollers.html:
iframe border not being drawn. Probably being drawn over by background
compositing/scrollbars/custom-composited-different-track-parts.html: We are not
painting the custom border.
css3/blending/background-blend-mode-overlapping-accelerated-elements.html: Looks
like we have some bleeding
paint/invalidation/caret-with-composited-scroll.html: Focus painting again
scrollbars/basic-scrollbar.html: Scrollbar border painting
scrollbars/disabled-scrollbar.html: Scrollbar border painting
scrollbars/listbox-scrollbar-combinations.html: Scrollbar border painting
scrollbars/scrollbar-buttons.html: Scrollbar border painting
scrollbars/scrollbar-orientation.html: Scrollbar border painting
scrollbars/scrollbars-on-positioned-content.html: Scrollbar border painting

[bokan, 2016-07-26 14:09:42]

Description was changed from

==========
WIP - Paint solid color and local attachment backgrounds into scrolling contents
layer to get composited LCD text layers.

This change detects when we can paint the background onto the scrolling contents
layer. This will unblock promoting scrollers with supported backgrounds on low
DPI devices as we can now maintain subpixel antialiased text.

BUG=381840,568847
TEST=TBD
==========

to

==========
WIP - Paint solid color and local attachment backgrounds into scrolling contents
layer to get composited LCD text layers.

This change detects when we can paint the background onto the scrolling contents
layer. This will unblock promoting scrollers with supported backgrounds on low
DPI devices as we can now maintain subpixel antialiased text.

BUG=381840,568847
TEST=TBD
==========

[flackr, 2016-07-27 21:49:14]

I found and fixed the bug with the border painting - turns out it was just
non-composited background painting that was problematic. I'm running a
linux_chromium_rel_ng tryjob to see what other webkit layout tests are failing.

[Stephen Chennney, 2016-07-27 22:33:36]

Some nits. It seems like a lot of these should be separate changes if they are
fixing existing problems rather than implementing the new functionality.

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

https://codereview.chromium.org/2068723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1833: // If this box has a
transform or contains paint, it acts as a fixed position container for fixed
descendants,
The previous method has the comment outside the block, and this one has it
inside. Let's make them consistent while we're modifying them.

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

https://codereview.chromium.org/2068723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:102: if
(!paintBackgroundOnly
You can have 1 if and put both of the remaining blocks inside it.

https://codereview.chromium.org/2068723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:108: if
(!paintBackgroundOnly
i.e unify this if with the one above.

https://codereview.chromium.org/2068723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:133: if
(!paintBackgroundOnly)
And here too.

[flackr, 2016-07-28 14:39:20]

On 2016/07/27 at 22:33:36, schenney wrote:
> Some nits. It seems like a lot of these should be separate changes if they are
fixing existing problems rather than implementing the new functionality.

Oh definitely. I have always planned on splitting this up into many patches as
I've already started to do (e.g. https://codereview.chromium.org/2127663002).
I'm just using this "mega-patch" to spot check the direction and ensure that
with all of the changes we get the end result of lcd text in composited
scrollers. We can nit on style in the split out patches.

On testing, I think I can currently test most of these painting changes by
creating ref tests which put a non-composited scroller next to a composited one
(i.e. will-change: transform) to verify that they draw the same (I think tests
can even manually turn on lcd text), but once we automatically promote we'll
have to be careful that the tests don't become meaningless - maybe by ensuring
that the cases are equivalent to a transparent background.

The recent LayoutTests run looks promising. Have you seen errors like the themed
scrollbars not being used before? It doesn't reproduce when I run-webkit-tests
locally. I.e. the test fast/block/margin-collapse/103.html at
https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...

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

https://codereview.chromium.org/2068723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1833: // If this box has a
transform or contains paint, it acts as a fixed position container for fixed
descendants,
On 2016/07/27 at 22:33:36, Stephen Chennney wrote:
> The previous method has the comment outside the block, and this one has it
inside. Let's make them consistent while we're modifying them.

Done.

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

https://codereview.chromium.org/2068723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:102: if
(!paintBackgroundOnly
On 2016/07/27 at 22:33:36, Stephen Chennney wrote:
> You can have 1 if and put both of the remaining blocks inside it.

Done.

https://codereview.chromium.org/2068723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:108: if
(!paintBackgroundOnly
On 2016/07/27 at 22:33:36, Stephen Chennney wrote:
> i.e unify this if with the one above.

Done.

https://codereview.chromium.org/2068723002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:133: if
(!paintBackgroundOnly)
On 2016/07/27 at 22:33:36, Stephen Chennney wrote:
> And here too.

Done.

[Stephen Chennney, 2016-07-28 15:31:25]

On 2016/07/28 14:39:20, flackr wrote:
> The recent LayoutTests run looks promising. Have you seen errors like the
themed
> scrollbars not being used before? It doesn't reproduce when I run-webkit-tests
> locally. I.e. the test fast/block/margin-collapse/103.html at
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...

I haven't seen that kind of thing before. It looks like the code that asks the
theme to paint and gets back a "yes, I painted" or "no, I didn't" result is
getting a different answer from the theme. But that doesn't explain why you see
it on bots but not locally. Note that I kicked off a second try run, so we have
2 sets of results to compare, and in at least one of them those scrollbar theme
failures don't happen. It might be that there was or is a problem with the bots.

[flackr, 2016-07-29 02:39:17]

On 2016/07/28 at 15:31:25, schenney wrote:
> On 2016/07/28 14:39:20, flackr wrote:
> > The recent LayoutTests run looks promising. Have you seen errors like the
themed
> > scrollbars not being used before? It doesn't reproduce when I
run-webkit-tests
> > locally. I.e. the test fast/block/margin-collapse/103.html at
> >
https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...
> 
> I haven't seen that kind of thing before. It looks like the code that asks the
theme to paint and gets back a "yes, I painted" or "no, I didn't" result is
getting a different answer from the theme. But that doesn't explain why you see
it on bots but not locally. Note that I kicked off a second try run, so we have
2 sets of results to compare, and in at least one of them those scrollbar theme
failures don't happen. It might be that there was or is a problem with the bots.

As you pointed out this seems to be bot flake. I've filed
https://crbug.com/632369 to track it.

FYI I split out the minimum necessary portion of this code to support solid
color backgrounds: https://codereview.chromium.org/2196583002. This should be
ready to land and then we can automatically promote when we have an opaque
scrolling contents layer.

[flackr, 2016-08-10 17:30:45]

Description was changed from

==========
WIP - Paint solid color and local attachment backgrounds into scrolling contents
layer to get composited LCD text layers.

This change detects when we can paint the background onto the scrolling contents
layer. This will unblock promoting scrollers with supported backgrounds on low
DPI devices as we can now maintain subpixel antialiased text.

BUG=381840,568847
TEST=TBD
==========

to

==========
WIP - Paint solid color and local attachment backgrounds into scrolling contents
layer to get composited LCD text layers.

This change detects when we can paint the background onto the scrolling contents
layer. This will unblock promoting scrollers with supported backgrounds on low
DPI devices as we can now maintain subpixel antialiased text.

BUG=381840,568847
TEST=TBD
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[flackr, 2016-08-11 17:06:32]

Description was changed from

==========
WIP - Paint solid color and local attachment backgrounds into scrolling contents
layer to get composited LCD text layers.

This change detects when we can paint the background onto the scrolling contents
layer. This will unblock promoting scrollers with supported backgrounds on low
DPI devices as we can now maintain subpixel antialiased text.

BUG=381840,568847
TEST=TBD
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Paint local attachment backgrounds into composited scrolling contents layer

This change detects when we can paint the background onto the scrolling contents
layer. This will unblock promoting scrollers with supported backgrounds on low
DPI devices as we can now maintain subpixel antialiased text.

BUG=381840,568847
TEST=compositing/overflow/overflow-scroll-with-local-background.html,
LayoutBoxTest.BackgroundRect,
CompositedLayerMappingTest.ShouldPaintBackgroundOntoScrollingContentsLayer
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[flackr, 2016-08-11 17:06:32]

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

[flackr, 2016-08-11 17:07:43]

Okay, I've merged this with the landed portion of the code and updated the
appropriate methods so that we can also paint on the scrolling contents layer
when we effectively have entirely locally attached backgrounds. This fixes a
longstanding bug we've had with composited scrolling using a local attachment
background. PTAL, thanks.

[chrishtr, 2016-08-12 17:03:00]

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

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-background.html:5:
// Double rAF to ensure content is painted before scroll.
Add another test rather than modifying this one.

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (left):

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:714: if
(!style()->isOverflowVisible() && style()->backgroundLayers().attachment() ==
LocalBackgroundAttachment)
Why can you remove this?

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:717: continue;
Indentation.

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.h:48: enum BackgroundRectOpacity
{ TranslucentBackgroundRect, OpaqueBackgroundRect };
The meaning of these enum values is not that clear. Rename or add a comment. I
thin they mean

TranslucentBackgroundRect: consider all fill layers
OpaqueBackgroundRect: consider only opaque fill layers

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBoxTest.cpp (right):

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxTest.cpp:27:
TEST_F(LayoutBoxTest, BackgroundRect)
Add more tests for other cases.

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp
(right):

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:660:
// Fourth scroller cannot paint background into scrolling contents layer because
the background image is not locally attached.
You said "cannot" but have EXPECT_TRUE below.

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1209: static bool
allLayersAreLocal(const FillLayer& layer)
Seems to be cleaner to inline this in hasEntirelyLocalBackground, by just
declaring


const FillLayer& layer = backgroundLayers();

at the beginning.

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1215: &&
(currLayer->image() || currLayer->clip() != PaddingFillBox))
I don't understand the PaddingFillBox parts, could you explain it to me?

[chrishtr, 2016-08-12 23:17:00]

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

[chrishtr, 2016-08-12 23:17:00]

+trchen as additional reviewer since I am about to be on vacation for a week.

[trchen, 2016-08-13 00:06:42]

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (left):

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:714: if
(!style()->isOverflowVisible() && style()->backgroundLayers().attachment() ==
LocalBackgroundAttachment)
On 2016/08/12 17:03:00, chrishtr - OOO wrote:
> Why can you remove this?

The matching logic is in line 730 on the right side.

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:722: } else {
}
if (!current->next()) {

1. A layer with translucent image can still have opaque color below.
2. Background color is only applicable on bottom layer.

Also I recall schenney@ may have wrote a helper function to determine whether a
fill layer is opaque. We should reuse it if it's there.

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:732: backgroundBox =
enclosingFillBox(backgroundBox, currentClip);
I'm worried about this line, because the unscrolled content box and the local
content box are different boxes.

http://jsbin.com/dujuzazeva/ demonstrates the difference.

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1215: &&
(currLayer->image() || currLayer->clip() != PaddingFillBox))
On 2016/08/12 17:03:00, chrishtr - OOO wrote:
> I don't understand the PaddingFillBox parts, could you explain it to me?

I think he is making an exception for solid color layers here. Even if a layer
is not locally attached, but if it is solid color filling the whole padding box,
it behaves no differently from a local solid color layer.

I think it would be clearer to write it this way:
for (const FillLayer* currLayer = &layer; currLayer; currLayer =
currLayer->next()) {
    if (currLayer->attachment() == LocalBackgroundAttachment)
        continue;
    if (!currLayer->image() && currLayer->clip() == PaddingFillBox)
        continue;
    return false;
}
Human brains are designed to reason DNF form easier. :)

[trchen, 2016-08-13 00:14:08]

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:707: LayoutRect
LayoutBox::backgroundRect(BackgroundRectOpacity rectOpacity) const
I'm worried about the semantic of this function being vague.

Does it return a cull rect so it is guaranteed nothing outside of this rect will
paint (i.e. allow false positive, no false negative)? However in the case of
OpaqueBackgroundRect it reports a rect that's absolutely opaque, but there can
still be opaque pixels outside of the rect (i.e. allow false negative, no false
positive).

[flackr, 2016-08-16 17:52:39]

Description was changed from

==========
Paint local attachment backgrounds into composited scrolling contents layer

This change detects when we can paint the background onto the scrolling contents
layer. This will unblock promoting scrollers with supported backgrounds on low
DPI devices as we can now maintain subpixel antialiased text.

BUG=381840,568847
TEST=compositing/overflow/overflow-scroll-with-local-background.html,
LayoutBoxTest.BackgroundRect,
CompositedLayerMappingTest.ShouldPaintBackgroundOntoScrollingContentsLayer
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Paint local attachment backgrounds into composited scrolling contents layer

This change detects when we can paint the background onto the scrolling contents
layer. This will unblock promoting scrollers with supported backgrounds on low
DPI devices as we can now maintain subpixel antialiased text.

BUG=381840,568847
TEST=compositing/overflow/overflow-scroll-with-local-image-background.html,
LayoutBoxTest.BackgroundRect,
CompositedLayerMappingTest.ShouldPaintBackgroundOntoScrollingContentsLayer
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[flackr, 2016-08-16 17:52:51]

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

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-background.html:5:
// Double rAF to ensure content is painted before scroll.
On 2016/08/12 at 17:03:00, chrishtr - OOO wrote:
> Add another test rather than modifying this one.

Done.

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:707: LayoutRect
LayoutBox::backgroundRect(BackgroundRectOpacity rectOpacity) const
On 2016/08/13 at 00:14:08, trchen wrote:
> I'm worried about the semantic of this function being vague.
> 
> Does it return a cull rect so it is guaranteed nothing outside of this rect
will paint (i.e. allow false positive, no false negative)? However in the case
of OpaqueBackgroundRect it reports a rect that's absolutely opaque, but there
can still be opaque pixels outside of the rect (i.e. allow false negative, no
false positive).

When called with OpaqueBackgroundRect from backgroundIsOpaqueInRect we are okay
with false negatives, as this will not result in visual artifacts. If we have
false positives though it would result in creating an opaque layer and seeing
unpainted parts of the picture.

When called with TranlucentBackgroundRect from compositedLayerMapping we want
the cull / clip rect such that nothing outside this rect will paint.

Perhaps I should rephrase the values in the enum to reflect this, what do you
think of enum BackgroundRectType { BackgroundClipRect, BackgroundKnownOpaqueRect
};?

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:717: continue;
On 2016/08/12 at 17:03:00, chrishtr - OOO wrote:
> Indentation.

Done.

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:722: } else {
On 2016/08/13 at 00:06:42, trchen wrote:
> }
> if (!current->next()) {
> 
> 1. A layer with translucent image can still have opaque color below.

Yes, that is why we continue to the next layer if we encounter a translucent
image.

> 2. Background color is only applicable on bottom layer.

Hmm, I had mistakenly assumed that !cur->image() implies the background color,
but at least "none" can be specified in a background list which has no image -
I've added a test for this..
> 
> Also I recall schenney@ may have wrote a helper function to determine whether
a fill layer is opaque. We should reuse it if it's there.

The only related patch I can find recently is
https://codereview.chromium.org/1826013002 which only checked the background
color property. I've checked FillLayer but it doesn't seem to have any such
property, but perhaps the contents of this loop should become a helper on
FillLayer to this end.

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:732: backgroundBox =
enclosingFillBox(backgroundBox, currentClip);
On 2016/08/13 at 00:06:42, trchen wrote:
> I'm worried about this line, because the unscrolled content box and the local
content box are different boxes.
> 
> http://jsbin.com/dujuzazeva/ demonstrates the difference.

Right, I think we should treat background-attachment: local content-box as the
padding box when we're asking for the "translucent" / clip / cull rect (i.e.
over-estimate is okay) but the content-box when we're asking for the opaque
background rect (i.e. under-estimate is okay). Perhaps this will be more clear
with the new enum values.

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBoxTest.cpp (right):

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxTest.cpp:27:
TEST_F(LayoutBoxTest, BackgroundRect)
On 2016/08/12 at 17:03:00, chrishtr - OOO wrote:
> Add more tests for other cases.

I added tests which I think cover all of the special cases - let me know if you
think I missed any.

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp
(right):

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:660:
// Fourth scroller cannot paint background into scrolling contents layer because
the background image is not locally attached.
On 2016/08/12 at 17:03:00, chrishtr - OOO wrote:
> You said "cannot" but have EXPECT_TRUE below.

Oops, fixed.

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1209: static bool
allLayersAreLocal(const FillLayer& layer)
On 2016/08/12 at 17:03:00, chrishtr - OOO wrote:
> Seems to be cleaner to inline this in hasEntirelyLocalBackground, by just
declaring
> 
> 
> const FillLayer& layer = backgroundLayers();
> 
> at the beginning.

Done.

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1215: &&
(currLayer->image() || currLayer->clip() != PaddingFillBox))
On 2016/08/13 at 00:06:42, trchen wrote:
> On 2016/08/12 17:03:00, chrishtr - OOO wrote:
> > I don't understand the PaddingFillBox parts, could you explain it to me?
> 
> I think he is making an exception for solid color layers here. Even if a layer
is not locally attached, but if it is solid color filling the whole padding box,
it behaves no differently from a local solid color layer.
> 
> I think it would be clearer to write it this way:
> for (const FillLayer* currLayer = &layer; currLayer; currLayer =
currLayer->next()) {
>     if (currLayer->attachment() == LocalBackgroundAttachment)
>         continue;
>     if (!currLayer->image() && currLayer->clip() == PaddingFillBox)
>         continue;
>     return false;
> }
> Human brains are designed to reason DNF form easier. :)

Done.

[flackr, 2016-08-16 18:30:26]

I think this could be split up into painting the local backgrounds and then a
follow on properly marking them opaque when they should be if you would prefer.

[trchen, 2016-08-17 01:22:55]

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:707: LayoutRect
LayoutBox::backgroundRect(BackgroundRectOpacity rectOpacity) const
On 2016/08/16 17:52:51, flackr wrote:
> On 2016/08/13 at 00:14:08, trchen wrote:
> > I'm worried about the semantic of this function being vague.
> > 
> > Does it return a cull rect so it is guaranteed nothing outside of this rect
> will paint (i.e. allow false positive, no false negative)? However in the case
> of OpaqueBackgroundRect it reports a rect that's absolutely opaque, but there
> can still be opaque pixels outside of the rect (i.e. allow false negative, no
> false positive).
> 
> When called with OpaqueBackgroundRect from backgroundIsOpaqueInRect we are
okay
> with false negatives, as this will not result in visual artifacts. If we have
> false positives though it would result in creating an opaque layer and seeing
> unpainted parts of the picture.
> 
> When called with TranlucentBackgroundRect from compositedLayerMapping we want
> the cull / clip rect such that nothing outside this rect will paint.
> 
> Perhaps I should rephrase the values in the enum to reflect this, what do you
> think of enum BackgroundRectType { BackgroundClipRect,
BackgroundKnownOpaqueRect
> };?

Great idea. It totally sounds clearer!

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:722: } else {
On 2016/08/16 17:52:51, flackr wrote:
> On 2016/08/13 at 00:06:42, trchen wrote:
> > }
> > if (!current->next()) {
> > 
> > 1. A layer with translucent image can still have opaque color below.
> 
> Yes, that is why we continue to the next layer if we encounter a translucent
> image.
> 
> > 2. Background color is only applicable on bottom layer.
> 
> Hmm, I had mistakenly assumed that !cur->image() implies the background color,
> but at least "none" can be specified in a background list which has no image -
> I've added a test for this..

The background color is always applicable even if the bottom layer has an image,
so this line should be 'if' instead of 'else if'. See
BoxPainter::paintFillLayer().

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:732: backgroundBox =
enclosingFillBox(backgroundBox, currentClip);
On 2016/08/16 17:52:51, flackr wrote:
> On 2016/08/13 at 00:06:42, trchen wrote:
> > I'm worried about this line, because the unscrolled content box and the
local
> content box are different boxes.
> > 
> > http://jsbin.com/dujuzazeva/ demonstrates the difference.
> 
> Right, I think we should treat background-attachment: local content-box as the
> padding box when we're asking for the "translucent" / clip / cull rect (i.e.
> over-estimate is okay) but the content-box when we're asking for the opaque
> background rect (i.e. under-estimate is okay). Perhaps this will be more clear
> with the new enum values.

Agreed. We can do that plus a comment to explain.

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1209: static bool
allLayersAreLocal(const FillLayer& layer)
On 2016/08/16 17:52:51, flackr wrote:
> On 2016/08/12 at 17:03:00, chrishtr - OOO wrote:
> > Seems to be cleaner to inline this in hasEntirelyLocalBackground, by just
> declaring
> > 
> > 
> > const FillLayer& layer = backgroundLayers();
> > 
> > at the beginning.
> 
> Done.

Did you forget to git add?

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

https://codereview.chromium.org/2068723002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-image-background.html:34:
</div>
Wow, this test actually failed without your CL. I tried toggling the background
image with devtools, it is definitely an under-invalidation bug. Could you
please file a bug and +cc wangxianzhu@ ? Thanks!

https://codereview.chromium.org/2068723002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2068723002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.h:352: // The enclosing
rectangle of the background with given opacity requirement.
I think this comment needs to be revised too, as for BackgroundKnownOpaqueRect
it is in fact an enclosed rect.

https://codereview.chromium.org/2068723002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBoxTest.cpp (right):

https://codereview.chromium.org/2068723002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxTest.cpp:33: "#target4 {
background: url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUg) content-box, green
border-box multiply;}"
The shorthand form doesn't seem to support background-blend-mode. You may have
to write "background-blend-mode:normal,multiply;".

https://codereview.chromium.org/2068723002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxTest.cpp:58: // #target4's
background color has a blend mode so it isn't opaque.
Note: CSS backgrounds always create an isolated group, so blending should not
affect background opaqueness (as a whole) in any way. It is okay to be
conservative though.

https://codereview.chromium.org/2068723002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxTest.cpp:67:
EXPECT_EQ(LayoutRect(0, 0, 140, 140),
layoutBox->backgroundRect(BackgroundClipRect));
This is surprising to me. I think it relies on the fact that
FillLayer::cullEmptyLayers() cannot delete the top layer. If you insert one
layer in the beginning like: "background: none content-box, none border-box,
green content-box;", the result probably will be different.

It is not your fault, but a comment here can save a lot of head scratching. :)

[trchen, 2016-08-17 01:32:52]

On 2016/08/17 01:22:55, trchen wrote:
>
https://codereview.chromium.org/2068723002/diff/160001/third_party/WebKit/Lay...
> File
>
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-image-background.html
> (right):
> 
>
https://codereview.chromium.org/2068723002/diff/160001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-local-image-background.html:34:
> </div>
> Wow, this test actually failed without your CL. I tried toggling the
background
> image with devtools, it is definitely an under-invalidation bug. Could you
> please file a bug and +cc wangxianzhu@ ? Thanks!
> 

Forget it. Your CL desc mentioned issue 568847, and it is me who filed it
(insert shame cube meme here. orz).

[flackr, 2016-08-17 18:20:55]

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:722: } else {
On 2016/08/17 at 01:22:55, trchen wrote:
> On 2016/08/16 17:52:51, flackr wrote:
> > On 2016/08/13 at 00:06:42, trchen wrote:
> > > }
> > > if (!current->next()) {
> > > 
> > > 1. A layer with translucent image can still have opaque color below.
> > 
> > Yes, that is why we continue to the next layer if we encounter a translucent
> > image.
> > 
> > > 2. Background color is only applicable on bottom layer.
> > 
> > Hmm, I had mistakenly assumed that !cur->image() implies the background
color,
> > but at least "none" can be specified in a background list which has no image
-
> > I've added a test for this..
> 
> The background color is always applicable even if the bottom layer has an
image, so this line should be 'if' instead of 'else if'. See
BoxPainter::paintFillLayer().

Thanks for the clarification. Done, but needed to rework this to be a neither
condition. If neither the image or color is opaque, then continue.

https://codereview.chromium.org/2068723002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:732: backgroundBox =
enclosingFillBox(backgroundBox, currentClip);
On 2016/08/17 at 01:22:55, trchen wrote:
> On 2016/08/16 17:52:51, flackr wrote:
> > On 2016/08/13 at 00:06:42, trchen wrote:
> > > I'm worried about this line, because the unscrolled content box and the
local
> > content box are different boxes.
> > > 
> > > http://jsbin.com/dujuzazeva/ demonstrates the difference.
> > 
> > Right, I think we should treat background-attachment: local content-box as
the
> > padding box when we're asking for the "translucent" / clip / cull rect (i.e.
> > over-estimate is okay) but the content-box when we're asking for the opaque
> > background rect (i.e. under-estimate is okay). Perhaps this will be more
clear
> > with the new enum values.
> 
> Agreed. We can do that plus a comment to explain.

Done, and added a test for this case.

[trchen, 2016-08-18 21:24:23]

https://codereview.chromium.org/2068723002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBoxTest.cpp (right):

https://codereview.chromium.org/2068723002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxTest.cpp:66: // covering the
border box.
As I commented earlier, this is quite surprising and relies on the limitation of
FillLayer::cullEmptyLayers(). Can we add a line of comment explaining why 'none'
background is not getting culled?

https://codereview.chromium.org/2068723002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/2068723002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1209: static bool
allLayersAreLocal(const FillLayer* layer)
Can we merge this with hasEntirelyLocalBackground() below? Or do you plan to
reuse this helper in the future?

[flackr, 2016-08-19 21:59:56]

https://codereview.chromium.org/2068723002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBoxTest.cpp (right):

https://codereview.chromium.org/2068723002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxTest.cpp:66: // covering the
border box.
On 2016/08/18 at 21:24:22, trchen wrote:
> As I commented earlier, this is quite surprising and relies on the limitation
of FillLayer::cullEmptyLayers(). Can we add a line of comment explaining why
'none' background is not getting culled?

My apologies for missing this earlier comment. Do you have an explanation?
Should we be culling it? I wasn't sure I understood all of the ways FillLayer
can be used so I assumed it was better to leave it as is for now. This would
also show up on the ComputedStyle right? So I suppose it may be required to keep
the layer listed.

https://codereview.chromium.org/2068723002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/2068723002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1209: static bool
allLayersAreLocal(const FillLayer* layer)
On 2016/08/18 at 21:24:23, trchen wrote:
> Can we merge this with hasEntirelyLocalBackground() below? Or do you plan to
reuse this helper in the future?

Of course, I actually have a follow-on patch in which I merge this
(https://codereview.chromium.org/2264663002) but I may as well do this now.

[trchen, 2016-08-19 22:49:15]

lgtm except one error in LayoutBoxTest #target4.

https://codereview.chromium.org/2068723002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBoxTest.cpp (right):

https://codereview.chromium.org/2068723002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxTest.cpp:66: // covering the
border box.
On 2016/08/19 21:59:56, flackr wrote:
> On 2016/08/18 at 21:24:22, trchen wrote:
> > As I commented earlier, this is quite surprising and relies on the
limitation
> of FillLayer::cullEmptyLayers(). Can we add a line of comment explaining why
> 'none' background is not getting culled?
> 
> My apologies for missing this earlier comment. Do you have an explanation?
> Should we be culling it? I wasn't sure I understood all of the ways FillLayer
> can be used so I assumed it was better to leave it as is for now. This would
> also show up on the ComputedStyle right? So I suppose it may be required to
keep
> the layer listed.

Actually... Apologies from me. I realized FillLayer::cullEmptyLayers() is a
different thing than what I thought it is. Your test totally made sense.

---- Explanation (you may TL;DR) ----

I thought FillLayer::cullEmptyLayers() means removing layers that don't have an
image because those layers are provably no-op. It is not what it does.

In fact it is a part of style resolving. Basically we grow the FillLayer list to
the length of the longest background property. However CSS spec says
background-image dictates the number of background layers. Other background
properties that exceeded the length of background-image are simply ignored.
FillLayer::cullEmptyLayers() trims down the list to the same length as
background-image.

For example:
background-clipping: border-box padding-box content-box;
background-color: green;
Is equivalent to
background: none border-box green;

background-clipping: border-box, padding-box, content-box;
background-image: none, none;
background-color: green;
Is equvalent to
background: none border-box, none padding-box green;

https://codereview.chromium.org/2068723002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/2068723002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1209: static bool
allLayersAreLocal(const FillLayer* layer)
On 2016/08/19 21:59:56, flackr wrote:
> On 2016/08/18 at 21:24:23, trchen wrote:
> > Can we merge this with hasEntirelyLocalBackground() below? Or do you plan to
> reuse this helper in the future?
> 
> Of course, I actually have a follow-on patch in which I merge this
> (https://codereview.chromium.org/2264663002) but I may as well do this now.

Cool.

https://codereview.chromium.org/2068723002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBoxTest.cpp (right):

https://codereview.chromium.org/2068723002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxTest.cpp:33: "#target4 {
background: url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUg) content-box, green
border-box multiply;}"
The shorthand form doesn't support background-blend-mode. According to the spec,
the use of shorthand form overwrites background-blend-mode with initial value.

You'll have to write this:
background-image: url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUg), none;
background-clip: content-box, border-box;
background-blend-mode: normal, multiply;
background-color: green;

[flackr, 2016-08-22 15:12:39]

https://codereview.chromium.org/2068723002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBoxTest.cpp (right):

https://codereview.chromium.org/2068723002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxTest.cpp:33: "#target4 {
background: url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUg) content-box, green
border-box multiply;}"
On 2016/08/19 at 22:49:14, trchen wrote:
> The shorthand form doesn't support background-blend-mode. According to the
spec, the use of shorthand form overwrites background-blend-mode with initial
value.
> 
> You'll have to write this:
> background-image: url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUg), none;
> background-clip: content-box, border-box;
> background-blend-mode: normal, multiply;
> background-color: green;

Done.

[flackr, 2016-08-22 15:14:01]

The CQ bit was checked by flackr@chromium.org

[flackr, 2016-08-22 15:14:01]

The patchset sent to the CQ was uploaded after l-g-t-m from trchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2068723002/#ps220001
(title: "Use full property names to specify background blend mode.")

[commit-bot: I haz the power, 2016-08-22 15:14:23]

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-22 15:22:17]

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

[commit-bot: I haz the power, 2016-08-22 15:22:18]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[flackr, 2016-08-22 15:28:20]

Chris would you be willing to provide OWNERS review?

[chrishtr, 2016-08-22 20:09:54]

lgtm

[flackr, 2016-08-23 13:37:32]

The CQ bit was checked by flackr@chromium.org

[flackr, 2016-08-23 13:37:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, trchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2068723002/#ps240001
(title: "Rebaseline tests")

[commit-bot: I haz the power, 2016-08-23 13:37:51]

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-23 14:46:32]

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

[commit-bot: I haz the power, 2016-08-23 14:46:34]

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

[flackr, 2016-08-23 21:22:55]

It looks like my new tests have turned up a pre-existing memory leak. I filed
https://crbug.com/640344 to track but is it reasonable to add a suppression so
as not to block this?

Also, I made a slight change to not consider non-repeating (or repeating with
gaps) backgrounds opaque (as they may not fill the clip so we'd need more
complex logic to figure out what bounds they do fill). I don't have a test for
this as after struggling for a while with it I'm still not sure how to get
RenderingTest to wait for images (even data URL's) to load but I manually
tested.

Last but not least, I noticed when rebasing the tests that we used to mark some
of the blended background layers opaque in a few of them, I wonder if we can
remove the blend mode restriction. I'd prefer to do this in a later patch
though.

[flackr, 2016-08-26 13:00:52]

The CQ bit was checked by flackr@chromium.org

[flackr, 2016-08-26 13:00:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, trchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2068723002/#ps260001
(title: "Only consider an image opaque if its guaranteed to fill the clip
(repeats with no gaps)")

[commit-bot: I haz the power, 2016-08-26 13: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, 2016-08-26 14:48:48]

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

[commit-bot: I haz the power, 2016-08-26 14:48:50]

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-26 14:57:29]

The CQ bit was checked by flackr@chromium.org

[flackr, 2016-08-26 14:57:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, trchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2068723002/#ps280001
(title: "Merge and fix test expectations.")

[commit-bot: I haz the power, 2016-08-26 14:57:48]

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-26 16:26:49]

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

[commit-bot: I haz the power, 2016-08-26 16:26:51]

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-26 18:45:05]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2016-08-26 18:46: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, 2016-08-26 20:26:27]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-08-26 20:27:53]

Description was changed from

==========
Paint local attachment backgrounds into composited scrolling contents layer

This change detects when we can paint the background onto the scrolling contents
layer. This will unblock promoting scrollers with supported backgrounds on low
DPI devices as we can now maintain subpixel antialiased text.

BUG=381840,568847
TEST=compositing/overflow/overflow-scroll-with-local-image-background.html,
LayoutBoxTest.BackgroundRect,
CompositedLayerMappingTest.ShouldPaintBackgroundOntoScrollingContentsLayer
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Paint local attachment backgrounds into composited scrolling contents layer

This change detects when we can paint the background onto the scrolling contents
layer. This will unblock promoting scrollers with supported backgrounds on low
DPI devices as we can now maintain subpixel antialiased text.

BUG=381840,568847
TEST=compositing/overflow/overflow-scroll-with-local-image-background.html,
LayoutBoxTest.BackgroundRect,
CompositedLayerMappingTest.ShouldPaintBackgroundOntoScrollingContentsLayer
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/05ee9971df08ffd2e57af7e6258a2b3efab9b539
Cr-Commit-Position: refs/heads/master@{#414795}
==========

[commit-bot: I haz the power, 2016-08-26 20:27:54]

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