Fix the bug that negative outline-offset is covered up by composited

Fix the bug that negative outline-offset is covered up by composited
scrolling contents layer.

This CL adds a decoration layer, child of the graphics layer, painted
on top of other layers. Painting outline in this layer would guarantee
its appearance.

Currently the decoration layer is used only for this purpose. But
in the future more contents will be added into it.

BUG=642866
TEST=CompositedLayerMappingTest.DecorationLayerOnlyCreatedInCompositedScrolling; CompositedLayerMappingTest.DecorationLayerCreatedAndDestroyedInCompositedScrolling; third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-negative-offset-translucent-outline.html;
third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-with-negative-offset-outline.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/4199d1cead13a646b2cf14ad36ef00e61e9bab83
Cr-Commit-Position: refs/heads/master@{#433205}

[yigu, 2016-10-21 17:32:30]

Description was changed from

==========
Fix the bug that negative outline-offset is covered up by composited
scrolling contents layer.

This CL adds a decoration layer, child of the graphics layer, painted
on top of other layers. Painting outline in this layer would guarantee
its appearance.

Currently the decoration layer is used only for this purpose. But
in the future more contents will be added into it.

BUG=642866
==========

to

==========
Fix the bug that negative outline-offset is covered up by composited
scrolling contents layer.

This CL adds a decoration layer, child of the graphics layer, painted
on top of other layers. Painting outline in this layer would guarantee
its appearance.

Currently the decoration layer is used only for this purpose. But
in the future more contents will be added into it.

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

[yigu, 2016-10-21 18:49:14]

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

[commit-bot: I haz the power, 2016-10-21 18:49:55]

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

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

[commit-bot: I haz the power, 2016-10-21 19:39:16]

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

[yigu, 2016-10-24 15:42:02]

Description was changed from

==========
Fix the bug that negative outline-offset is covered up by composited
scrolling contents layer.

This CL adds a decoration layer, child of the graphics layer, painted
on top of other layers. Painting outline in this layer would guarantee
its appearance.

Currently the decoration layer is used only for this purpose. But
in the future more contents will be added into it.

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

to

==========
Fix the bug that negative outline-offset is covered up by composited
scrolling contents layer.

This CL adds a decoration layer, child of the graphics layer, painted
on top of other layers. Painting outline in this layer would guarantee
its appearance.

Currently the decoration layer is used only for this purpose. But
in the future more contents will be added into it.

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

[yigu, 2016-10-24 15:42:02]

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

[flackr, 2016-10-24 17:19:30]

Overall this looks great. There are a couple extra things that I think you still
need to handle.
1. Including the layer in JSON dumps used for tests (this leads into #2). There
are a lot of tests which dump the layer tree as JSON and we need the decoration
layer included. See
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...
which is called on every layer when adding it to the JSON structure. You should
add a test which confirms that the layer is created using this like
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/compositi...
2. Paint invalidations - if you look at
third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-with-local-background-expected.txt
you can see it expects a paint invalidation on the scrolling contents layer when
the background changes. This similarly should cause a paint invalidation on the
decoration layer when the decoration changes. I suspect the decoration will not
update if you for example change its color after it is already composited.

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

https://codereview.chromium.org/2439113003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:527:
if (updateDecorationLayer(layoutObject->style()->hasOutline()))
I don't think this condition is specific enough. We only need the decoration
layer for composited overflow scrollers (i.e.
m_owningLayer.getScrollableArea()->usesCompositedScrolling()) with a negative
outline offset that would be covered up by the composited scrolling contents
layer and scrollbars.

https://codereview.chromium.org/2439113003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerPaintingInfo.h (right):

https://codereview.chromium.org/2439113003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPaintingInfo.h:73:
PaintLayerPaintingCompositingAllPhases =
I think we need to add PaintLayerPaintingCompositingDecorationPhase to the
CompositingAllPhases.

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

https://codereview.chromium.org/2439113003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositingReasons.h:97: const
uint64_t CompositingReasonLayerForDecoration = UINT64_C(1) << 50;
We don't need to add a compositing reason for this - we should never composite
something just because it has a negative-offset outline, it will just create an
extra layer when composited if we need to for  the outline.

[flackr, 2016-10-24 20:07:05]

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

https://codereview.chromium.org/2439113003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1871:
if ((mode & ApplyToDecorationLayer) && mapping->decorationLayer())
I think you need to apply to non scrolling content layers to get the
invalidation in composited scrolling content, e.g. add:
|| (mode & ApplyToNonScrollingContentLayers)

[yigu, 2016-10-24 20:31:36]

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

https://codereview.chromium.org/2439113003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:527:
if (updateDecorationLayer(layoutObject->style()->hasOutline()))
On 2016/10/24 17:19:30, flackr wrote:
> I don't think this condition is specific enough. We only need the decoration
> layer for composited overflow scrollers (i.e.
> m_owningLayer.getScrollableArea()->usesCompositedScrolling()) with a negative
> outline offset that would be covered up by the composited scrolling contents
> layer and scrollbars.

Done.

https://codereview.chromium.org/2439113003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1871:
if ((mode & ApplyToDecorationLayer) && mapping->decorationLayer())
On 2016/10/24 20:07:04, flackr wrote:
> I think you need to apply to non scrolling content layers to get the
> invalidation in composited scrolling content, e.g. add:
> || (mode & ApplyToNonScrollingContentLayers)

Done.

https://codereview.chromium.org/2439113003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerPaintingInfo.h (right):

https://codereview.chromium.org/2439113003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPaintingInfo.h:73:
PaintLayerPaintingCompositingAllPhases =
On 2016/10/24 17:19:30, flackr wrote:
> I think we need to add PaintLayerPaintingCompositingDecorationPhase to the
> CompositingAllPhases.

Done.

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

https://codereview.chromium.org/2439113003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositingReasons.h:97: const
uint64_t CompositingReasonLayerForDecoration = UINT64_C(1) << 50;
On 2016/10/24 17:19:30, flackr wrote:
> We don't need to add a compositing reason for this - we should never composite
> something just because it has a negative-offset outline, it will just create
an
> extra layer when composited if we need to for  the outline.

Will add a compositing reason as discussed.

[yigu, 2016-10-24 20:38:26]

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

[commit-bot: I haz the power, 2016-10-24 20:39:04]

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-10-24 21:11:12]

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

[commit-bot: I haz the power, 2016-10-24 21:11:13]

Dry run: 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...)

[yigu, 2016-10-25 17:13:48]

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

[commit-bot: I haz the power, 2016-10-25 17:14:16]

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-10-25 18:25:20]

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

[commit-bot: I haz the power, 2016-10-25 18:25:20]

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

[flackr, 2016-10-25 19:48:11]

This review seems to be getting quite a few patchsets so I'll wait until it
settles down to comment more - but here's some comments on what I've looked at
so far.

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

https://codereview.chromium.org/2439113003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:565:
if
(updateDecorationLayer(m_owningLayer.getScrollableArea()->usesCompositedScrolling())
&&
Can you move this condition to a boolean similar to needsAncestorClip? Also, it
looks like you're only passing usesCompositedScrolling() for
needsDecorationLayer.

https://codereview.chromium.org/2439113003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:566:
layoutObject->style()->hasOutline() && layoutObject->style()->outlineOffset() <
0)
In fact, now that I think about it this should be less than the minimum negative
border width (i.e. same check that we have in
LayoutBoxModelObject::hasLocalEquivalentBackground). Can you remove the check
for negative outline offset there since with this patch we can now composite it
correctly?

https://codereview.chromium.org/2439113003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:954:
updateDecorationLayerGeometry(contentsSize);
It seems to me that the sizes of these layers will be larger than they need to
be. Consider for example:

outline: 20px solid blue;
outline-offset: -5px;

This should result in only the decoration layer being 15px larger than the
element since the background and foreground layers will no longer draw the
outline. For now, it's probably safe to have the layers be the same size but we
should fix this in the future. Can you leave a TODO to only make the layers as
large as they need to be?

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

https://codereview.chromium.org/2439113003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1602:
m_backgroundLayer->setDrawsContent(hasPaintedContent);
The background should no longer setDrawsContent(true) if we only have an outline
(i.e. if there is nothing actually drawn into the background anymore).

https://codereview.chromium.org/2439113003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1605:
m_decorationLayer->setDrawsContent(hasPaintedContent);
We should only be allocating an m_decorationLayer if it draws content, so this
can just be true.

[yigu, 2016-10-25 20:42:24]

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

[commit-bot: I haz the power, 2016-10-25 20:43:13]

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

[yigu, 2016-10-25 20:58:45]

Thanks Rob! Please see patch 5 for an updated version. I didn't remove the "if
(outlineOverlapsPaddingBox)" check in LayoutBoxModelObject.cpp. Leave it to a
separate CL.

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

https://codereview.chromium.org/2439113003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:565:
if
(updateDecorationLayer(m_owningLayer.getScrollableArea()->usesCompositedScrolling())
&&
On 2016/10/25 at 19:48:10, flackr wrote:
> Can you move this condition to a boolean similar to needsAncestorClip? Also,
it looks like you're only passing usesCompositedScrolling() for
needsDecorationLayer.

This bug has been fixed in patch 4.

https://codereview.chromium.org/2439113003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:566:
layoutObject->style()->hasOutline() && layoutObject->style()->outlineOffset() <
0)
On 2016/10/25 at 19:48:10, flackr wrote:
> In fact, now that I think about it this should be less than the minimum
negative border width (i.e. same check that we have in
LayoutBoxModelObject::hasLocalEquivalentBackground). Can you remove the check
for negative outline offset there since with this patch we can now composite it
correctly?

Done.

https://codereview.chromium.org/2439113003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:954:
updateDecorationLayerGeometry(contentsSize);
On 2016/10/25 at 19:48:10, flackr wrote:
> It seems to me that the sizes of these layers will be larger than they need to
be. Consider for example:
> 
> outline: 20px solid blue;
> outline-offset: -5px;
> 
> This should result in only the decoration layer being 15px larger than the
element since the background and foreground layers will no longer draw the
outline. For now, it's probably safe to have the layers be the same size but we
should fix this in the future. Can you leave a TODO to only make the layers as
large as they need to be?

TODO added.

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

https://codereview.chromium.org/2439113003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1602:
m_backgroundLayer->setDrawsContent(hasPaintedContent);
On 2016/10/25 at 19:48:10, flackr wrote:
> The background should no longer setDrawsContent(true) if we only have an
outline (i.e. if there is nothing actually drawn into the background anymore).

TODO added.

https://codereview.chromium.org/2439113003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1605:
m_decorationLayer->setDrawsContent(hasPaintedContent);
On 2016/10/25 at 19:48:10, flackr wrote:
> We should only be allocating an m_decorationLayer if it draws content, so this
can just be true.

Set to true.

[flackr, 2016-10-25 21:26:31]

Can you verify that the rebaselines of
scrollbars/border-box-rect-clips-scrollbars-expected.png are correct? It seems
like we lost an outline.

Also I think the thai-line-breaks test change is not expected, I have seen this
occasionally from the rebaseline tool but I think it's an incorrect update.

https://codereview.chromium.org/2439113003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-with-negative-offset-outline.html
(right):

https://codereview.chromium.org/2439113003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-with-negative-offset-outline.html:15:
outline: 1px solid blue;
nit: it may be good to start the outline red so that if it fails to update it is
obvious that it is the wrong color.

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

https://codereview.chromium.org/2439113003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:564:
// and scrollbars.
Suggested wording:
If the outline needs to draw over the composited scrolling contents layer
or scrollbar layers it needs to be drawn into a separate layer.

https://codereview.chromium.org/2439113003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:967:
// The actual size of the decoration layer should be calculated separately.
nit: The size of the background layer should be different as well. We need to
check whether we are painting the decoration layer into the background and then
ignore or consider the outline when determining the contentsSize.

https://codereview.chromium.org/2439113003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1613:
// only have an outline (i.e. if there is nothing actually drawn into the
nit: "if we only have an outline and we're drawing the outline into the
decoration layer"

https://codereview.chromium.org/2439113003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2058:
m_backgroundLayer->setPaintingPhase(GraphicsLayerPaintBackground);
I think this function needs to know if we have a decoration layer and if not set
the painting phase to GraphicsLayerPaintBackground |
GraphicsLayerPaintDecoration otherwise it seems like we won't draw the outline
into the background when we don't create the decoration layer.

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

https://codereview.chromium.org/2439113003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:254: // It used to be
painted in either the foreground phase or the background
No need to mention where it used to be painted, we can just say The outline is
painted as part of the decoration phase which paints content that is not
scrolled and should be above scrolled content.

https://codereview.chromium.org/2439113003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsLayerClient.h (right):

https://codereview.chromium.org/2439113003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsLayerClient.h:47:
GraphicsLayerPaintAllWithOverflowClip =
GraphicsLayerPaintDecoration should be part of this.

[commit-bot: I haz the power, 2016-10-25 22:13:28]

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

[commit-bot: I haz the power, 2016-10-25 22:13:29]

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

[yigu, 2016-10-27 19:25:14]

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

[commit-bot: I haz the power, 2016-10-27 19:26:06]

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

[yigu, 2016-10-27 20:21:26]

Hi Rob,
Yesterday we tried
m_backgroundLayer->setPaintingPhase(GraphicsLayerPaintBackground |
GraphicsLayerPaintDecoration) but it didn't work. The reason is that whenever we
call updateBackgroundLayer(bool) and set the phases, the layerChanged will be
set true. Accordingly, updatePaintingPhases() will be called in which they do
m_graphicsLayer->setPaintingPhase(paintingPhaseForPrimaryLayer()). Since we only
have foreground, background and mask layers handled there. The decoration phase
flag will be reset. Therefore I added phase |= GraphicsLayerPaintDecoration to
make sure the decoration phase flag is set regardless the exist of decoration
layer. I tested it using examples with translucent outline and the outline was
painted once. Hope this way works.
The ref test will be added in next patch.
Thanks.

https://codereview.chromium.org/2439113003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-with-negative-offset-outline.html
(right):

https://codereview.chromium.org/2439113003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-with-negative-offset-outline.html:15:
outline: 1px solid blue;
On 2016/10/25 at 21:26:30, flackr wrote:
> nit: it may be good to start the outline red so that if it fails to update it
is obvious that it is the wrong color.

That makes more sense.

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

https://codereview.chromium.org/2439113003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:564:
// and scrollbars.
On 2016/10/25 at 21:26:30, flackr wrote:
> Suggested wording:
> If the outline needs to draw over the composited scrolling contents layer
> or scrollbar layers it needs to be drawn into a separate layer.

Done.

https://codereview.chromium.org/2439113003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:967:
// The actual size of the decoration layer should be calculated separately.
On 2016/10/25 at 21:26:30, flackr wrote:
> nit: The size of the background layer should be different as well. We need to
check whether we are painting the decoration layer into the background and then
ignore or consider the outline when determining the contentsSize.

Added.

https://codereview.chromium.org/2439113003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1613:
// only have an outline (i.e. if there is nothing actually drawn into the
On 2016/10/25 at 21:26:30, flackr wrote:
> nit: "if we only have an outline and we're drawing the outline into the
decoration layer"

Added.

https://codereview.chromium.org/2439113003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2058:
m_backgroundLayer->setPaintingPhase(GraphicsLayerPaintBackground);
On 2016/10/25 21:26:30, flackr wrote:
> I think this function needs to know if we have a decoration layer and if not
set
> the painting phase to GraphicsLayerPaintBackground |
> GraphicsLayerPaintDecoration otherwise it seems like we won't draw the outline
> into the background when we don't create the decoration layer.

Fix it by adding phase |= GraphicsLayerPaintDecoration into
paintingPhaseForPrimaryLayer() which is called by updatePaintingPhases() which
happens whenever there is a layer change.

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

https://codereview.chromium.org/2439113003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:254: // It used to be
painted in either the foreground phase or the background
On 2016/10/25 at 21:26:30, flackr wrote:
> No need to mention where it used to be painted, we can just say The outline is
painted as part of the decoration phase which paints content that is not
scrolled and should be above scrolled content.

Done.

https://codereview.chromium.org/2439113003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsLayerClient.h (right):

https://codereview.chromium.org/2439113003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsLayerClient.h:47:
GraphicsLayerPaintAllWithOverflowClip =
On 2016/10/25 at 21:26:30, flackr wrote:
> GraphicsLayerPaintDecoration should be part of this.

Done.

[commit-bot: I haz the power, 2016-10-27 21:24:05]

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

[commit-bot: I haz the power, 2016-10-27 21:24:06]

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-10-28 17:40:03]

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

https://codereview.chromium.org/2439113003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1481:
updateBottomLayer(m_decorationLayer.get());
What if we don't have a m_decorationLayer?

Also, I think instead of using bottomLayer you should add this explicitly to the
m_graphicsLayer, i.e. m_graphicsLayer->addChild(m_decorationLayer.get());

[yigu, 2016-11-01 17:22:36]

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

[commit-bot: I haz the power, 2016-11-01 17:22:54]

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

[yigu, 2016-11-01 17:30:30]

Hi Rob,
Just added a unit test for the decoration layer and a ref test for the
translucent outline painting. Please take a look. Thanks:)

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

https://codereview.chromium.org/2439113003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1481:
updateBottomLayer(m_decorationLayer.get());
On 2016/10/28 at 17:40:02, flackr wrote:
> What if we don't have a m_decorationLayer?
> 
> Also, I think instead of using bottomLayer you should add this explicitly to
the m_graphicsLayer, i.e. m_graphicsLayer->addChild(m_decorationLayer.get());

That makes total sense. Plus, since addChild doesn't check if we are passing
nullptr, I'll add a check as well.

[commit-bot: I haz the power, 2016-11-01 19:06:32]

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

[commit-bot: I haz the power, 2016-11-01 19:06:33]

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

[yigu, 2016-11-11 15:49:18]

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

[commit-bot: I haz the power, 2016-11-11 15:49:33]

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-11-11 17:05:57]

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

[commit-bot: I haz the power, 2016-11-11 17:05:59]

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

[yigu, 2016-11-11 18:03:01]

Hi Rob,

Please take a look at the tests I added. There are 3 failures left (see below).
For the first two, I had run run-webkit-tests --reset-result for the two html
files in the last patch but haven't run rebaseline-cl since then. For the last
failure, not sure what happened there. 

https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel...

https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...

[yigu, 2016-11-12 04:50:06]

Hi Rob,

The ref test passed but the other one still failed. I did rebaseline but it
complained about cannot find the failed test
'virtual/prefer_compositing_to_lcd_text/compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html'
so it couldn't rebaseline that file. I couldn't find that file under
LayoutTests/ myself..  Meanwhile all those irrelevant png files from
platform/android came up again when doing rebaseline.. :( 

Yi

[yigu, 2016-11-14 18:33:52]

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

[commit-bot: I haz the power, 2016-11-14 18:35:24]

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-11-14 21:30:30]

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

[commit-bot: I haz the power, 2016-11-14 21:30:31]

Dry run: This issue passed the CQ dry run.

[yigu, 2016-11-14 21:35:10]

Hi Rob,

I added the html that couldn't be rebaselined automatically into
TestExpectations. All green finally. Please take a look. Thanks!

Yi

[yigu, 2016-11-15 19:04:06]

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

[yigu, 2016-11-15 19:04:07]

Hi Chris,

To avoid the negative-offset outline being covered up by the composited
scrollbar, we now put the outline into a new layer called decoration layer which
is painted on top of other layers. Please take a look. Thanks!

Yi

[flackr, 2016-11-15 20:40:40]

https://codereview.chromium.org/2439113003/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-negative-offset-translucent-outline.html
(right):

https://codereview.chromium.org/2439113003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-negative-offset-translucent-outline.html:8:
document.getElementById('scroller').scrollTop = 200;
Is the double rAF or scrolling necessary? We already have a separate test that
the outline is not painted into the scrolling contents layer and it should look
the same whether scrolled or not right?

https://codereview.chromium.org/2439113003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-negative-offset-translucent-outline.html:17:
background: gray local content-box;
Why local content-box? I think just having background: local gray; would be
sufficient to make sure the outline is painted above the scrolling content.

https://codereview.chromium.org/2439113003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-negative-offset-translucent-outline.html:21:
padding: 10px;
nit: Remove unnecessary padding.

https://codereview.chromium.org/2439113003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-negative-offset-translucent-outline.html:32:
<!-- #scroller should not be covered up by a negative offset outline. It is
This comment is confusing, the outline should be on top of the #scroller
shouldn't it? Did you mean #scroller outline should not be covered up by
scrolling content background?

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

https://codereview.chromium.org/2439113003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:967:
// The size of the background layer should be differnet as well. We need to
nit:s/differnet/different

[yigu, 2016-11-15 21:35:58]

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

[commit-bot: I haz the power, 2016-11-15 21:36:15]

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-11-15 21:50:39]

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

[commit-bot: I haz the power, 2016-11-15 21:50:40]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[yigu, 2016-11-15 23:04:53]

The CQ bit was checked by yigu@chromium.org

[yigu, 2016-11-15 23:05:13]

The CQ bit was unchecked by yigu@chromium.org

[yigu, 2016-11-15 23:30:14]

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

[yigu, 2016-11-15 23:30:16]

Thanks Rob for the feedback! New patch on.

https://codereview.chromium.org/2439113003/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-negative-offset-translucent-outline.html
(right):

https://codereview.chromium.org/2439113003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-negative-offset-translucent-outline.html:8:
document.getElementById('scroller').scrollTop = 200;
On 2016/11/15 20:40:40, flackr wrote:
> Is the double rAF or scrolling necessary? We already have a separate test that
> the outline is not painted into the scrolling contents layer and it should
look
> the same whether scrolled or not right?

You are right. This is a redundant test.

https://codereview.chromium.org/2439113003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-negative-offset-translucent-outline.html:17:
background: gray local content-box;
On 2016/11/15 20:40:40, flackr wrote:
> Why local content-box? I think just having background: local gray; would be
> sufficient to make sure the outline is painted above the scrolling content.

Done.

https://codereview.chromium.org/2439113003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-negative-offset-translucent-outline.html:21:
padding: 10px;
On 2016/11/15 20:40:40, flackr wrote:
> nit: Remove unnecessary padding.

Done.

https://codereview.chromium.org/2439113003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-negative-offset-translucent-outline.html:32:
<!-- #scroller should not be covered up by a negative offset outline. It is
On 2016/11/15 20:40:40, flackr wrote:
> This comment is confusing, the outline should be on top of the #scroller
> shouldn't it? Did you mean #scroller outline should not be covered up by
> scrolling content background?

Yes that was misleading.

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

https://codereview.chromium.org/2439113003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:967:
// The size of the background layer should be differnet as well. We need to
On 2016/11/15 20:40:40, flackr wrote:
> nit:s/differnet/different

Done.

[flackr, 2016-11-16 21:14:20]

Just a few comments, but overall looks good.

https://codereview.chromium.org/2439113003/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-negative-offset-translucent-outline.html
(right):

https://codereview.chromium.org/2439113003/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-negative-offset-translucent-outline.html:3:
document.getElementById('scroller').scrollTop = 200;
nit: Scrolling is unnecessary for testing this.

https://codereview.chromium.org/2439113003/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-negative-offset-translucent-outline.html:8:
background: local grey;
Thinking about this some more, if we were double painting the outline (which is
the main reason to test a partially translucent outline) we may obscure that by
having a background on the scroller. Can you remove this background?

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

https://codereview.chromium.org/2439113003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:840:
DecorationLayerCreatedAndDestroyedInCompositedScrolling) {
You should probably also test that we don't create a decoration layer if we're
not composited scrolling.

https://codereview.chromium.org/2439113003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:877:
}
nit: Blank line between tests.

[yigu, 2016-11-17 15:50:54]

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

[commit-bot: I haz the power, 2016-11-17 15:51:16]

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

[yigu, 2016-11-17 15:53:15]

Hi Rob, thanks for the feedback! Added a new unit test and removed some
unnecessary element in the ref test. PTAL.
Yi

https://codereview.chromium.org/2439113003/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-negative-offset-translucent-outline.html
(right):

https://codereview.chromium.org/2439113003/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-negative-offset-translucent-outline.html:3:
document.getElementById('scroller').scrollTop = 200;
On 2016/11/16 21:14:20, flackr wrote:
> nit: Scrolling is unnecessary for testing this.

Done.

https://codereview.chromium.org/2439113003/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-negative-offset-translucent-outline.html:8:
background: local grey;
On 2016/11/16 21:14:20, flackr wrote:
> Thinking about this some more, if we were double painting the outline (which
is
> the main reason to test a partially translucent outline) we may obscure that
by
> having a background on the scroller. Can you remove this background?

Done.

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

https://codereview.chromium.org/2439113003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:840:
DecorationLayerCreatedAndDestroyedInCompositedScrolling) {
On 2016/11/16 21:14:20, flackr wrote:
> You should probably also test that we don't create a decoration layer if we're
> not composited scrolling.

Test added.

https://codereview.chromium.org/2439113003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:877:
}
On 2016/11/16 21:14:20, flackr wrote:
> nit: Blank line between tests.

Done.

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

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

[commit-bot: I haz the power, 2016-11-17 16:08:02]

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

[flackr, 2016-11-17 16:10:45]

Please name the specific tests you added for this in a TEST= line next to the
BUG= line in your review description.

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

https://codereview.chromium.org/2439113003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:843:
"will-change: transform;'></div>");
This element should have a negative offset outline such that if it were a
composited scroller it would get a decoration layer. Perhaps the test could
actually verify that it does by switching from overflow: visible to overflow:
scroll.

[flackr, 2016-11-17 17:02:13]

You should be seeing #scroller15 in
PaintLayerScrollableAreaTest.CanPaintBackgroundOntoScrollingContentsLayer being
true now, but I don't see that change in your patch. Can you check why that is?

[yigu, 2016-11-17 18:47:31]

Description was changed from

==========
Fix the bug that negative outline-offset is covered up by composited
scrolling contents layer.

This CL adds a decoration layer, child of the graphics layer, painted
on top of other layers. Painting outline in this layer would guarantee
its appearance.

Currently the decoration layer is used only for this purpose. But
in the future more contents will be added into it.

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

to

==========
Fix the bug that negative outline-offset is covered up by composited
scrolling contents layer.

This CL adds a decoration layer, child of the graphics layer, painted
on top of other layers. Painting outline in this layer would guarantee
its appearance.

Currently the decoration layer is used only for this purpose. But
in the future more contents will be added into it.

BUG=642866
TEST=CompositedLayerMappingTest.DecorationLayerOnlyCreatedInCompositedScrolling;
CompositedLayerMappingTest.DecorationLayerCreatedAndDestroyedInCompositedScrolling;
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-negative-offset-translucent-outline.html;
third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-with-negative-offset-outline.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[yigu, 2016-11-17 19:03:23]

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

[yigu, 2016-11-17 19:04:35]

On 2016/11/17 17:02:13, flackr wrote:
> You should be seeing #scroller15 in
> PaintLayerScrollableAreaTest.CanPaintBackgroundOntoScrollingContentsLayer
being
> true now, but I don't see that change in your patch. Can you check why that
is?

Fixed in patch 13. PTAL.

[commit-bot: I haz the power, 2016-11-17 19:04:47]

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

[flackr, 2016-11-17 19:50:24]

LGTM

[commit-bot: I haz the power, 2016-11-17 20:51:33]

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

[commit-bot: I haz the power, 2016-11-17 20:51:34]

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

[chrishtr, 2016-11-17 21:15:57]

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

https://codereview.chromium.org/2439113003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:283:
(isPaintingCompositedDecoration || !isPaintingScrollingContent) &&
What about cases when there is composited scrolling but no decoration layer?

[yigu, 2016-11-17 22:01:26]

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

[commit-bot: I haz the power, 2016-11-17 22:03:00]

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

[yigu, 2016-11-17 22:03:59]

On 2016/11/17 21:15:57, chrishtr wrote:
>
https://codereview.chromium.org/2439113003/diff/240001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):
> 
>
https://codereview.chromium.org/2439113003/diff/240001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:283:
> (isPaintingCompositedDecoration || !isPaintingScrollingContent) &&
> What about cases when there is composited scrolling but no decoration layer?

In the cases we don't create decoration layer, the decoration phase is still
there regardless. Therefore the outline can still be drawn.

[chrishtr, 2016-11-17 22:40:21]

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

https://codereview.chromium.org/2439113003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:283:
(isPaintingCompositedDecoration || !isPaintingScrollingContent) &&
On 2016/11/17 at 21:15:57, chrishtr wrote:
> What about cases when there is composited scrolling but no decoration layer?

Ah, now I see. Thanks for explaining.

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

https://codereview.chromium.org/2439113003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:610:
// DecorationLayer which includes painting of outline etc.
etc? What does it paint except outline?

[yigu, 2016-11-17 22:45:29]

On 2016/11/17 22:40:21, chrishtr wrote:
>
https://codereview.chromium.org/2439113003/diff/240001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):
> 
>
https://codereview.chromium.org/2439113003/diff/240001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:283:
> (isPaintingCompositedDecoration || !isPaintingScrollingContent) &&
> On 2016/11/17 at 21:15:57, chrishtr wrote:
> > What about cases when there is composited scrolling but no decoration layer?
> 
> Ah, now I see. Thanks for explaining.
> 
>
https://codereview.chromium.org/2439113003/diff/260001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
> (right):
> 
>
https://codereview.chromium.org/2439113003/diff/260001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:610:
> // DecorationLayer which includes painting of outline etc.
> etc? What does it paint except outline?

Np. Currently we only draw outline into the decoration layer. But in the future
maybe more "decoration related stuff" will be done in that layer. Perhaps I
should take out the "etc" for now for clarification. Thanks!

[chrishtr, 2016-11-17 22:48:14]

On 2016/11/17 at 22:45:29, yigu wrote:
> On 2016/11/17 22:40:21, chrishtr wrote:
> >
https://codereview.chromium.org/2439113003/diff/240001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):
> > 
> >
https://codereview.chromium.org/2439113003/diff/240001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:283:
> > (isPaintingCompositedDecoration || !isPaintingScrollingContent) &&
> > On 2016/11/17 at 21:15:57, chrishtr wrote:
> > > What about cases when there is composited scrolling but no decoration
layer?
> > 
> > Ah, now I see. Thanks for explaining.
> > 
> >
https://codereview.chromium.org/2439113003/diff/260001/third_party/WebKit/Sou...
> > File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
> > (right):
> > 
> >
https://codereview.chromium.org/2439113003/diff/260001/third_party/WebKit/Sou...
> >
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:610:
> > // DecorationLayer which includes painting of outline etc.
> > etc? What does it paint except outline?
> 
> Np. Currently we only draw outline into the decoration layer. But in the
future maybe more "decoration related stuff" will be done in that layer. Perhaps
I should take out the "etc" for now for clarification. Thanks!

Yes, please update the comment to match current reality.

[chrishtr, 2016-11-17 22:48:46]

On 2016/11/17 at 22:48:14, chrishtr wrote:
> On 2016/11/17 at 22:45:29, yigu wrote:
> > On 2016/11/17 22:40:21, chrishtr wrote:
> > >
https://codereview.chromium.org/2439113003/diff/240001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):
> > > 
> > >
https://codereview.chromium.org/2439113003/diff/240001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:283:
> > > (isPaintingCompositedDecoration || !isPaintingScrollingContent) &&
> > > On 2016/11/17 at 21:15:57, chrishtr wrote:
> > > > What about cases when there is composited scrolling but no decoration
layer?
> > > 
> > > Ah, now I see. Thanks for explaining.
> > > 
> > >
https://codereview.chromium.org/2439113003/diff/260001/third_party/WebKit/Sou...
> > > File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
> > > (right):
> > > 
> > >
https://codereview.chromium.org/2439113003/diff/260001/third_party/WebKit/Sou...
> > >
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:610:
> > > // DecorationLayer which includes painting of outline etc.
> > > etc? What does it paint except outline?
> > 
> > Np. Currently we only draw outline into the decoration layer. But in the
future maybe more "decoration related stuff" will be done in that layer. Perhaps
I should take out the "etc" for now for clarification. Thanks!
> 
> Yes, please update the comment to match current reality.

Actually, renaming it m_boxDecorationOutline layer would be better, to make that
even more obvious.

[yigu, 2016-11-17 23:04:10]

On 2016/11/17 22:48:46, chrishtr wrote:
> On 2016/11/17 at 22:48:14, chrishtr wrote:
> > On 2016/11/17 at 22:45:29, yigu wrote:
> > > On 2016/11/17 22:40:21, chrishtr wrote:
> > > >
>
https://codereview.chromium.org/2439113003/diff/240001/third_party/WebKit/Sou...
> > > > File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):
> > > > 
> > > >
>
https://codereview.chromium.org/2439113003/diff/240001/third_party/WebKit/Sou...
> > > > third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:283:
> > > > (isPaintingCompositedDecoration || !isPaintingScrollingContent) &&
> > > > On 2016/11/17 at 21:15:57, chrishtr wrote:
> > > > > What about cases when there is composited scrolling but no decoration
> layer?
> > > > 
> > > > Ah, now I see. Thanks for explaining.
> > > > 
> > > >
>
https://codereview.chromium.org/2439113003/diff/260001/third_party/WebKit/Sou...
> > > > File
> third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
> > > > (right):
> > > > 
> > > >
>
https://codereview.chromium.org/2439113003/diff/260001/third_party/WebKit/Sou...
> > > >
>
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:610:
> > > > // DecorationLayer which includes painting of outline etc.
> > > > etc? What does it paint except outline?
> > > 
> > > Np. Currently we only draw outline into the decoration layer. But in the
> future maybe more "decoration related stuff" will be done in that layer.
Perhaps
> I should take out the "etc" for now for clarification. Thanks!
> > 
> > Yes, please update the comment to match current reality.
> 
> Actually, renaming it m_boxDecorationOutline layer would be better, to make
that
> even more obvious.

I can understand m_DecorationOutlineLayer but not sure what the "box" prefix
means..

[chrishtr, 2016-11-17 23:05:00]

lgtm

[chrishtr, 2016-11-17 23:07:08]

On 2016/11/17 at 23:05:00, chrishtr wrote:
> lgtm

"box decorations" are the things that happen to decorate LayoutBox objects.
Outline, border, etc. m_decorationOutlineLayer is ok too, up to you.

[commit-bot: I haz the power, 2016-11-17 23:49:19]

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

[commit-bot: I haz the power, 2016-11-17 23:49:21]

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

[yigu, 2016-11-18 01:48:36]

On 2016/11/17 23:07:08, chrishtr wrote:
> On 2016/11/17 at 23:05:00, chrishtr wrote:
> > lgtm
> 
> "box decorations" are the things that happen to decorate LayoutBox objects.
> Outline, border, etc. m_decorationOutlineLayer is ok too, up to you.

Thanks for explaining!

[yigu, 2016-11-18 02:07:51]

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

[commit-bot: I haz the power, 2016-11-18 02:09:55]

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-11-18 04:12:38]

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

[commit-bot: I haz the power, 2016-11-18 04:12:39]

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

[yigu, 2016-11-18 04:14:07]

Hi Rob,
I renamed the decorationLayer to decorationOutlineLayer and all other related
variables and functions. PTAL.
Thanks!
Yi

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

https://codereview.chromium.org/2439113003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:843:
"will-change: transform;'></div>");
On 2016/11/17 16:10:45, flackr wrote:
> This element should have a negative offset outline such that if it were a
> composited scroller it would get a decoration layer. Perhaps the test could
> actually verify that it does by switching from overflow: visible to overflow:
> scroll.

Done.

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

https://codereview.chromium.org/2439113003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:283:
(isPaintingCompositedDecoration || !isPaintingScrollingContent) &&
On 2016/11/17 21:15:57, chrishtr wrote:
> What about cases when there is composited scrolling but no decoration layer?

In the cases we don't create decoration layer, the decoration phase is still
there regardless.

[yigu, 2016-11-18 15:33:48]

The CQ bit was checked by yigu@chromium.org

[yigu, 2016-11-18 15:33:49]

The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org,
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2439113003/#ps280001
(title: "Rename decorationLayer to decorationOutlineLayer")

[commit-bot: I haz the power, 2016-11-18 15:34:11]

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-11-18 15:39:47]

Description was changed from

==========
Fix the bug that negative outline-offset is covered up by composited
scrolling contents layer.

This CL adds a decoration layer, child of the graphics layer, painted
on top of other layers. Painting outline in this layer would guarantee
its appearance.

Currently the decoration layer is used only for this purpose. But
in the future more contents will be added into it.

BUG=642866
TEST=CompositedLayerMappingTest.DecorationLayerOnlyCreatedInCompositedScrolling;
CompositedLayerMappingTest.DecorationLayerCreatedAndDestroyedInCompositedScrolling;
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-negative-offset-translucent-outline.html;
third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-with-negative-offset-outline.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix the bug that negative outline-offset is covered up by composited
scrolling contents layer.

This CL adds a decoration layer, child of the graphics layer, painted
on top of other layers. Painting outline in this layer would guarantee
its appearance.

Currently the decoration layer is used only for this purpose. But
in the future more contents will be added into it.

BUG=642866
TEST=CompositedLayerMappingTest.DecorationLayerOnlyCreatedInCompositedScrolling;
CompositedLayerMappingTest.DecorationLayerCreatedAndDestroyedInCompositedScrolling;
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-negative-offset-translucent-outline.html;
third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-with-negative-offset-outline.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2016-11-18 15:39:48]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-11-18 15:41:32]

Description was changed from

==========
Fix the bug that negative outline-offset is covered up by composited
scrolling contents layer.

This CL adds a decoration layer, child of the graphics layer, painted
on top of other layers. Painting outline in this layer would guarantee
its appearance.

Currently the decoration layer is used only for this purpose. But
in the future more contents will be added into it.

BUG=642866
TEST=CompositedLayerMappingTest.DecorationLayerOnlyCreatedInCompositedScrolling;
CompositedLayerMappingTest.DecorationLayerCreatedAndDestroyedInCompositedScrolling;
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-negative-offset-translucent-outline.html;
third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-with-negative-offset-outline.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix the bug that negative outline-offset is covered up by composited
scrolling contents layer.

This CL adds a decoration layer, child of the graphics layer, painted
on top of other layers. Painting outline in this layer would guarantee
its appearance.

Currently the decoration layer is used only for this purpose. But
in the future more contents will be added into it.

BUG=642866
TEST=CompositedLayerMappingTest.DecorationLayerOnlyCreatedInCompositedScrolling;
CompositedLayerMappingTest.DecorationLayerCreatedAndDestroyedInCompositedScrolling;
third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-negative-offset-translucent-outline.html;
third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-with-negative-offset-outline.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/4199d1cead13a646b2cf14ad36ef00e61e9bab83
Cr-Commit-Position: refs/heads/master@{#433205}
==========

[commit-bot: I haz the power, 2016-11-18 15:41:34]

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