Mark foregroundLayer under scrollingContentsLayer GraphicsLayerPaintOverflowContents

third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp

 /*
  * Copyright (C) 2009, 2010, 2011 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 1119 matching lines @@
             // The ancestor clipping layer is already set up and has m_graphicsLayer under it.
             m_ancestorClippingLayer->addChild(m_squashingLayer.get());
         }
     }
 }
 
 void CompositedLayerMapping::updatePaintingPhases()
 {
     m_graphicsLayer->setPaintingPhase(paintingPhaseForPrimaryLayer());
     if (m_scrollingContentsLayer) {
-        GraphicsLayerPaintingPhase paintPhase = GraphicsLayerPaintOverflowContents | GraphicsLayerPaintCompositedScroll;
-        if (!m_foregroundLayer)
-            paintPhase |= GraphicsLayerPaintForeground;
-        m_scrollingContentsLayer->setPaintingPhase(paintPhase);
+        GraphicsLayerPaintingPhase scrollingContentsLayerPaintingPhase = GraphicsLayerPaintOverflowContents | GraphicsLayerPaintCompositedScroll;
+        if (m_foregroundLayer)
+            m_foregroundLayer->setPaintingPhase(m_foregroundLayer->paintingPhase() | GraphicsLayerPaintOverflowContents);
+        else
+            scrollingContentsLayerPaintingPhase |= GraphicsLayerPaintForeground;
+        m_scrollingContentsLayer->setPaintingPhase(scrollingContentsLayerPaintingPhase);
+    } else if (m_foregroundLayer) {
+        m_foregroundLayer->setPaintingPhase(m_foregroundLayer->paintingPhase() & ~GraphicsLayerPaintOverflowContents);

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

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]

Done with slight difference. Ptal.

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

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]

Done.

     }
 }
 
 void CompositedLayerMapping::updateContentsRect()
 {
     m_graphicsLayer->setContentsRect(pixelSnappedIntRect(contentsBox()));
 }
 
 void CompositedLayerMapping::updateContentsOffsetInCompositingLayer(const IntPoint& snappedOffsetFromCompositedAncestor, const IntPoint& graphicsLayerParentLocation)
 {
@@ skipping 1414 matching lines @@
     } else if (graphicsLayer == m_scrollingContentsLayer.get()) {
         name = "Scrolling Contents Layer";
     } else {
         ASSERT_NOT_REACHED();
     }
 
     return name;
 }
 
 } // namespace blink