Mark foregroundLayer under scrollingContentsLayer GraphicsLayerPaintOverflowContents

Mark foregroundLayer under scrollingContentsLayer GraphicsLayerPaintOverflowContents

Missing the flag made contents of foregroundLayer under
scrollingContentsLayer incorrectly applied overflow clip, causing
checkboarding during composited scrolling.

BUG=599682
Committed: https://crrev.com/9b8e5d585a1a1f61b7afee8353e346f7764cecd2
Cr-Commit-Position: refs/heads/master@{#388000}

[Xianzhu, 2016-04-15 19:24:14]

wangxianzhu@chromium.org changed reviewers:
+ aelias@chromium.org, vollick@chromium.org

[Ian Vollick, 2016-04-16 00:29:03]

On 2016/04/15 19:24:15, Xianzhu wrote:

lgtm!

[aelias_OOO_until_Jul13, 2016-04-16 00:33:19]

https://codereview.chromium.org/1895443003/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/1895443003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1147:
m_foregroundLayer->setPaintingPhase(m_foregroundLayer->paintingPhase() &
~GraphicsLayerPaintOverflowContents);
The bit-setting/clearing logic is a bit hard to read, and mutating the existing
phase state isn't a pattern that's followed elsewhere in this file, and it
doesn't look needed for this either.  I'd rather we only set the state from
scratch.

How about removing the
m_foregroundLayer->setPaintingPhase(GraphicsLayerPaintForeground) call in
updateForegroundLayer() and in this method, write:

GraphicsLayerPaintingPhase foregroundLayerPaintingPhase =
GraphicsLayerPaintForeground;
if (m_scrollingContentsLayer) {
  foregroundLayerPaintingPhase |= GraphicsLayerPaintOverflowContents;
  ...
}
if (m_foregroundLayer)
  m_foregroundLayer->setPaintingPhase(foregroundLayerPaintingPhase);

[Xianzhu, 2016-04-16 22:07:36]

https://codereview.chromium.org/1895443003/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/1895443003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1147:
m_foregroundLayer->setPaintingPhase(m_foregroundLayer->paintingPhase() &
~GraphicsLayerPaintOverflowContents);
On 2016/04/16 00:33:19, aelias wrote:
> The bit-setting/clearing logic is a bit hard to read, and mutating the
existing
> phase state isn't a pattern that's followed elsewhere in this file, and it
> doesn't look needed for this either.  I'd rather we only set the state from
> scratch.
> 
> How about removing the
> m_foregroundLayer->setPaintingPhase(GraphicsLayerPaintForeground) call in
> updateForegroundLayer() and in this method, write:
> 
> GraphicsLayerPaintingPhase foregroundLayerPaintingPhase =
> GraphicsLayerPaintForeground;
> if (m_scrollingContentsLayer) {
>   foregroundLayerPaintingPhase |= GraphicsLayerPaintOverflowContents;
>   ...
> }
> if (m_foregroundLayer)
>   m_foregroundLayer->setPaintingPhase(foregroundLayerPaintingPhase);

Done with slight difference. Ptal.

[aelias_OOO_until_Jul13, 2016-04-18 17:09:41]

https://codereview.chromium.org/1895443003/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/1895443003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1147:
m_foregroundLayer->setPaintingPhase(m_foregroundLayer->paintingPhase() &
~GraphicsLayerPaintOverflowContents);
On 2016/04/16 at 22:07:36, Xianzhu wrote:
> Done with slight difference. Ptal.

Slight difference looks fine, but can you also delete the the line
m_foregroundLayer>setPaintingPhase(GraphicsLayerPaintForeground) in
updateForegroundLayer() too?  I believe that line is now no-op because
updatePaintingPhases() will always be called when updateForegroundLayer() adds a
new foreground layer.

[Xianzhu, 2016-04-18 18:06:47]

https://codereview.chromium.org/1895443003/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/1895443003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1147:
m_foregroundLayer->setPaintingPhase(m_foregroundLayer->paintingPhase() &
~GraphicsLayerPaintOverflowContents);
On 2016/04/18 17:09:41, aelias wrote:
> On 2016/04/16 at 22:07:36, Xianzhu wrote:
> > Done with slight difference. Ptal.
> 
> Slight difference looks fine, but can you also delete the the line
> m_foregroundLayer>setPaintingPhase(GraphicsLayerPaintForeground) in
> updateForegroundLayer() too?  I believe that line is now no-op because
> updatePaintingPhases() will always be called when updateForegroundLayer() adds
a
> new foreground layer.

Done.

[aelias_OOO_until_Jul13, 2016-04-18 18:11:24]

lgtm, thanks!

[Xianzhu, 2016-04-18 18:27:41]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-04-18 18:27:41]

The patchset sent to the CQ was uploaded after l-g-t-m from vollick@chromium.org
Link to the patchset: https://codereview.chromium.org/1895443003/#ps50001
(title: " ")

[commit-bot: I haz the power, 2016-04-18 18:28:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1895443003/50001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1895443003/50001

[commit-bot: I haz the power, 2016-04-18 20:27:02]

Committed patchset #4 (id:50001)

[commit-bot: I haz the power, 2016-04-18 20:28:30]

Description was changed from

==========
Mark foregroundLayer under scrollingContentsLayer
GraphicsLayerPaintOverflowContents

Missing the flag made contents of foregroundLayer under
scrollingContentsLayer incorrectly applied overflow clip, causing
checkboarding during composited scrolling.

BUG=599682
==========

to

==========
Mark foregroundLayer under scrollingContentsLayer
GraphicsLayerPaintOverflowContents

Missing the flag made contents of foregroundLayer under
scrollingContentsLayer incorrectly applied overflow clip, causing
checkboarding during composited scrolling.

BUG=599682

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

[commit-bot: I haz the power, 2016-04-18 20:28:31]

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