Make OverlapMap a reference. We never actually pass null.

Make OverlapMap a reference. We never actually pass null.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169903

[esprehn, 2014-03-22 01:34:51]

lgtm

[esprehn, 2014-03-22 01:34:54]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-03-22 01:35:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ojan@chromium.org/208943005/20001

[commit-bot: I haz the power, 2014-03-22 01:37:23]

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

[commit-bot: I haz the power, 2014-03-22 01:37:24]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[Ian Vollick, 2014-03-22 05:54:01]

On 2014/03/22 01:37:24, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   tryserver.blink on win_blink_rel

In our last layerz meeting, I thought there was support for adding a flag for
turning off the overlap map (since we were undecided about killing it and
because it would make it easier to run experiments). Before this CL, that would
amount to passing a NULL overlap map, but this CL makes that trickier.

If the nullity checks aren't showing up in profiles, can we please leave them? I
would really like to add this flag and do some testing.

[ojan, 2014-03-22 21:19:10]

On 2014/03/22 05:54:01, Ian Vollick wrote:
> In our last layerz meeting, I thought there was support for adding a flag for
> turning off the overlap map (since we were undecided about killing it and
> because it would make it easier to run experiments). Before this CL, that
would
> amount to passing a NULL overlap map, but this CL makes that trickier.
> 
> If the nullity checks aren't showing up in profiles, can we please leave them?
I
> would really like to add this flag and do some testing.

I changed this because I was working through the code and started writing some
ASSERTs that needed to do something special if there was no overlap map only to
realize that that never happens. So, it took up some time.

I'm not opposed to a flag or the extra if-statements if we have a good reason
for having it, but I don't see why we need a flag just to do local testing. As
is, the no overlap map codepath is silently breaking anyways. Making the overlap
map null already won't work in a debug build because you'll hit the asserts I
added in my last patch.

[Ian Vollick, 2014-03-22 22:14:44]

On 2014/03/22 21:19:10, ojan wrote:
> On 2014/03/22 05:54:01, Ian Vollick wrote:
> > In our last layerz meeting, I thought there was support for adding a flag
for
> > turning off the overlap map (since we were undecided about killing it and
> > because it would make it easier to run experiments). Before this CL, that
> would
> > amount to passing a NULL overlap map, but this CL makes that trickier.
> > 
> > If the nullity checks aren't showing up in profiles, can we please leave
them?
> I
> > would really like to add this flag and do some testing.
> 
> I changed this because I was working through the code and started writing some
> ASSERTs that needed to do something special if there was no overlap map only
to
> realize that that never happens. So, it took up some time.
> 
> I'm not opposed to a flag or the extra if-statements if we have a good reason
> for having it, but I don't see why we need a flag just to do local testing.

Actually, I didn't mean for local testing. We could realize significant wins
from
removing the overlap map, even with the caching of absolute rects and geometry.
We had seen an increase in raster costs, but we hadn't attempted to measure the
improvements earlier in the pipeline, nor had we established that those loses
couldn't be mitigated (I'm going to be plumbing a content region anyhow to fix
the hit testing blocker that may be used to help here).

So I had argued that we not close the book on this yet; I would like to do an
analysis on this for the next iteration. It will be much easier to do this if
the no overlap mode can be supported via a flag and the codepath isn't busted.

It would also be nice if other folks could test with this configuration.

> is, the no overlap map codepath is silently breaking anyways.

I don't think this had been the case at the time that chrishtr last took his
measurements? If we've been unwittingly (or wittingly) been making code changes
that preclude the removal of the overlap map, I think that's an argument for
making the path more officially supported so that we don't accidentally break
it until we're certain we're done with it.

> Making the overlap map null already won't work in a debug build because you'll
> hit the asserts I added in my last patch.

If the asserts are due to bugs, I'd be happy to help with them. I can get the
patches for the flag ready on Monday.

[ojan, 2014-03-23 00:37:53]

I disagree in principle with keeping the code in trunk for doing analysis of
future decisions, but this bit of code is small enough that I don't feel
strongly enough about this to push this patch through, so I'll close it.

Whether this is showing up on profiles is not clear to me. We do spend 5% of our
time in computeCompositingRequirements inside computeCompositingRequirements
itself. I haven't dug into where that time is going though.

On 2014/03/22 22:14:44, Ian Vollick wrote:
> Actually, I didn't mean for local testing. We could realize significant wins
> from removing the overlap map, even with the caching of absolute rects and
geometry.
> We had seen an increase in raster costs, but we hadn't attempted to measure
the
> improvements earlier in the pipeline, nor had we established that those loses
> couldn't be mitigated (I'm going to be plumbing a content region anyhow to fix
> the hit testing blocker that may be used to help here).
> 
> So I had argued that we not close the book on this yet; I would like to do an
> analysis on this for the next iteration. It will be much easier to do this if
> the no overlap mode can be supported via a flag and the codepath isn't busted.

Adding that flag doesn't bother me. But I don't see how the codepath can
reasonably be maintained when it doesn't run by default anywhere.

Incidentally, I've recently become more convinced that we can't get rid of the
overlap map. I could imagine a future in which record/raster is super fast that
would allow getting rid of the overlap map, but that's not going to happen in
the short term.

> It would also be nice if other folks could test with this configuration.

How should people test this configuration? Even if we had the flag, you can't
just run the layout tests because many of the layer tree dump tests will
correctly have different output. People could test that it doesn't crash and
that no pixel tests fail, but I don't think it's really reasonable to expect
people to maintain codepaths without test coverage.

> > is, the no overlap map codepath is silently breaking anyways.
> 
> I don't think this had been the case at the time that chrishtr last took his
measurements? 

This just broke with https://codereview.chromium.org/208313004/. The issue is
that if there's no overlap map, then we don't set the absolute bounding box on
the RenderLayer. This will cause an assert to hit immediately after
computeCompositingRequirements (the assert is that the bounding box dirty bit
was cleared) and another one in
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
since the dirty bit will be dirty when reading the absoluteBoundingBox.

> If we've been unwittingly (or wittingly) been making code changes
> that preclude the removal of the overlap map, I think that's an argument for
> making the path more officially supported so that we don't accidentally break
> it until we're certain we're done with it.

I agree that the issue of whether to keep the overlap map is still an open
question, but I disagree that the trunk codebase should take the constraints
from that until such a time as we've decided to do it. It does make doing the
analysis of whether to keep it harder, but at the benefit of making the codebase
simpler for the rest of the team trying to work with the code and avoiding any
accidental performance hit.

> > Making the overlap map null already won't work in a debug build because
you'll
> > hit the asserts I added in my last patch.
> 
> If the asserts are due to bugs, I'd be happy to help with them. I can get the
> patches for the flag ready on Monday.

The only bug I know of from that patch is that the histogram in
GraphicsLayerUpdater.cpp will report the wrong values when there's no overlap
map, but I can imagine new code getting written that uses
RenderLayer::absoluteBoundingBox without being aware that there's a flag that
makes it invalid. And, since the flag isn't run by default anywhere, there's no
way for them to notice that they've broken anything.

[esprehn, 2014-03-23 01:26:09]

I would prefer we land this patch. Getting rid of the map is not a short term
goal, we have much bigger things to solve right now. If you want to explore that
we can come up with an API to disable the checks, but passing a null value into
here doesn't seem like a good long term thing to do.

[Ian Vollick, 2014-03-23 02:10:52]

On 2014/03/23 00:37:53, ojan wrote:
> I disagree in principle with keeping the code in trunk for doing analysis of
> future decisions, but this bit of code is small enough that I don't feel
> strongly enough about this to push this patch through, so I'll close it.

Thanks for your explanations, and apologies for being difficult here. I was
worried that we were heading in a direction that would make it difficult to
consider changing our mind on this point, but you've convinced me that I'm
being unreasonable about this patch.

As you said, the patch is tiny, and given your explanation for why it's not
correct at the moment to pass a NULL overlap map, this change lgtm and I think
we should land it.

I'd really like to chat with you about this next week, though, if you've got
some time.

> Whether this is showing up on profiles is not clear to me. We do spend 5% of
our
> time in computeCompositingRequirements inside computeCompositingRequirements
> itself. I haven't dug into where that time is going though.

I was thinking about how we might be able to get closer to solving the
chicken-egg
problems if we can get rid of more layout-dependent compositing changes. It
would
also allow for some optimizations that are currently illegal
(http://codereview.chromium.org/202683005/).

I do think that the geometry costs due to the overlap map can be made
negligible,
though, with changes like you've been making.

> On 2014/03/22 22:14:44, Ian Vollick wrote:
> > Actually, I didn't mean for local testing. We could realize significant wins
> > from removing the overlap map, even with the caching of absolute rects and
> geometry.
> > We had seen an increase in raster costs, but we hadn't attempted to measure
> the
> > improvements earlier in the pipeline, nor had we established that those
loses
> > couldn't be mitigated (I'm going to be plumbing a content region anyhow to
fix
> > the hit testing blocker that may be used to help here).
> > 
> > So I had argued that we not close the book on this yet; I would like to do
an
> > analysis on this for the next iteration. It will be much easier to do this
if
> > the no overlap mode can be supported via a flag and the codepath isn't
busted.
> 
> Adding that flag doesn't bother me. But I don't see how the codepath can
> reasonably be maintained when it doesn't run by default anywhere.

Good point. I was thinking we could toggle it for layout tests like we're doing
for some other as-yet-unlaunched features (via internals.settings), but maybe
that's not enough.

> Incidentally, I've recently become more convinced that we can't get rid of the
> overlap map. I could imagine a future in which record/raster is super fast
that
> would allow getting rid of the overlap map, but that's not going to happen in
> the short term.

Yeah, I can believe it. I'm just pulling for the ability to test this out, but I
could imagine that the tests will show that it's impossible to remove in the
near
future.

> 
> > It would also be nice if other folks could test with this configuration.
> 
> How should people test this configuration? Even if we had the flag, you can't
> just run the layout tests because many of the layer tree dump tests will
> correctly have different output. People could test that it doesn't crash and
> that no pixel tests fail, but I don't think it's really reasonable to expect
> people to maintain codepaths without test coverage.

You're right, but would the layout tests I'd suggested give the needed coverage?

> 
> > > is, the no overlap map codepath is silently breaking anyways.
> > 
> > I don't think this had been the case at the time that chrishtr last took his
> measurements? 
> 
> This just broke with https://codereview.chromium.org/208313004/. The issue is
> that if there's no overlap map, then we don't set the absolute bounding box on
> the RenderLayer. This will cause an assert to hit immediately after
> computeCompositingRequirements (the assert is that the bounding box dirty bit
> was cleared) and another one in
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> since the dirty bit will be dirty when reading the absoluteBoundingBox.

That makes sense. Thanks for the explanation.

> 
> > If we've been unwittingly (or wittingly) been making code changes
> > that preclude the removal of the overlap map, I think that's an argument for
> > making the path more officially supported so that we don't accidentally
break
> > it until we're certain we're done with it.
> 
> I agree that the issue of whether to keep the overlap map is still an open
> question, but I disagree that the trunk codebase should take the constraints
> from that until such a time as we've decided to do it. It does make doing the
> analysis of whether to keep it harder, but at the benefit of making the
codebase
> simpler for the rest of the team trying to work with the code and avoiding any
> accidental performance hit.

Agreed.
 
> > > Making the overlap map null already won't work in a debug build because
> you'll
> > > hit the asserts I added in my last patch.
> > 
> > If the asserts are due to bugs, I'd be happy to help with them. I can get
the
> > patches for the flag ready on Monday.
> 
> The only bug I know of from that patch is that the histogram in
> GraphicsLayerUpdater.cpp will report the wrong values when there's no overlap
> map, but I can imagine new code getting written that uses
> RenderLayer::absoluteBoundingBox without being aware that there's a flag that
> makes it invalid. And, since the flag isn't run by default anywhere, there's
no
> way for them to notice that they've broken anything.

True, we'd certainly need tests.

[ojan, 2014-03-23 17:51:56]

The CQ bit was checked by ojan@chromium.org

[commit-bot: I haz the power, 2014-03-23 17:52:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ojan@chromium.org/208943005/20001

[commit-bot: I haz the power, 2014-03-23 17:52:11]

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

[commit-bot: I haz the power, 2014-03-23 17:52:11]

Failed to apply patch for
Source/core/rendering/compositing/RenderLayerCompositor.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/compositing/RenderLayerCompositor.cpp
  Hunk #1 FAILED at 487.
  Hunk #2 succeeded at 934 (offset 22 lines).
  Hunk #3 FAILED at 969.
  Hunk #4 succeeded at 1009 with fuzz 1 (offset 30 lines).
  Hunk #5 succeeded at 1030 (offset 30 lines).
  Hunk #6 FAILED at 1028.
  Hunk #7 succeeded at 1118 (offset 31 lines).
  Hunk #8 succeeded at 1133 (offset 31 lines).
  Hunk #9 succeeded at 1167 (offset 33 lines).
  Hunk #10 succeeded at 1193 (offset 33 lines).
  3 out of 10 hunks FAILED -- saving rejects to file
Source/core/rendering/compositing/RenderLayerCompositor.cpp.rej

Patch:       Source/core/rendering/compositing/RenderLayerCompositor.cpp
Index: Source/core/rendering/compositing/RenderLayerCompositor.cpp
diff --git a/Source/core/rendering/compositing/RenderLayerCompositor.cpp
b/Source/core/rendering/compositing/RenderLayerCompositor.cpp
index
66bd5b488179b5cf34a04121d8662ab5b137a1ba..219ca8f259edad85a753856d20faceac6d8b15d8
100644
--- a/Source/core/rendering/compositing/RenderLayerCompositor.cpp
+++ b/Source/core/rendering/compositing/RenderLayerCompositor.cpp
@@ -487,7 +487,7 @@ void
RenderLayerCompositor::updateCompositingLayersInternal()
             // scrolling and animation bounds is implemented
(crbug.com/252472).
             Vector<RenderLayer*> unclippedDescendants;
             IntRect absoluteDecendantBoundingBox;
-            computeCompositingRequirements(0, updateRoot,
&overlapTestRequestMap, recursionData, saw3DTransform, unclippedDescendants,
absoluteDecendantBoundingBox);
+            computeCompositingRequirements(0, updateRoot,
overlapTestRequestMap, recursionData, saw3DTransform, unclippedDescendants,
absoluteDecendantBoundingBox);
         }
 
         {
@@ -912,12 +912,11 @@ void RenderLayerCompositor::addToOverlapMap(OverlapMap&
overlapMap, RenderLayer*
 //      must be compositing so that its contents render over that child.
 //      This implies that its positive z-index children must also be
compositing.
 //
-void RenderLayerCompositor::computeCompositingRequirements(RenderLayer*
ancestorLayer, RenderLayer* layer, OverlapMap* overlapMap,
CompositingRecursionData& currentRecursionData, bool& descendantHas3DTransform,
Vector<RenderLayer*>& unclippedDescendants, IntRect&
absoluteDecendantBoundingBox)
+void RenderLayerCompositor::computeCompositingRequirements(RenderLayer*
ancestorLayer, RenderLayer* layer, OverlapMap& overlapMap,
CompositingRecursionData& currentRecursionData, bool& descendantHas3DTransform,
Vector<RenderLayer*>& unclippedDescendants, IntRect&
absoluteDecendantBoundingBox)
 {
     layer->stackingNode()->updateLayerListsIfNeeded();
 
-    if (overlapMap)
-        overlapMap->geometryMap().pushMappingsToAncestor(layer, ancestorLayer);
+    overlapMap.geometryMap().pushMappingsToAncestor(layer, ancestorLayer);
 
     // Clear the flag
     layer->setHasCompositingDescendant(false);
@@ -970,8 +969,8 @@ void
RenderLayerCompositor::computeCompositingRequirements(RenderLayer* ancestor
     }
 
     IntRect absBounds;
-    if (overlapMap && !layer->isRootLayer()) {
-        absBounds =
enclosingIntRect(overlapMap->geometryMap().absoluteRect(layer->overlapBounds()));
+    if (!layer->isRootLayer()) {
+        absBounds =
enclosingIntRect(overlapMap.geometryMap().absoluteRect(layer->overlapBounds()));
         // Setting the absBounds to 1x1 instead of 0x0 makes very little sense,
         // but removing this code will make JSGameBench sad.
         // See https://codereview.chromium.org/13912020/
@@ -980,8 +979,8 @@ void
RenderLayerCompositor::computeCompositingRequirements(RenderLayer* ancestor
     }
     absoluteDecendantBoundingBox = absBounds;
 
-    if (overlapMap && currentRecursionData.m_testingOverlap &&
!requiresCompositingOrSquashing(directReasons))
-        overlapCompositingReason = overlapMap->overlapsLayers(absBounds) ?
CompositingReasonOverlap : CompositingReasonNone;
+    if (currentRecursionData.m_testingOverlap &&
!requiresCompositingOrSquashing(directReasons))
+        overlapCompositingReason = overlapMap.overlapsLayers(absBounds) ?
CompositingReasonOverlap : CompositingReasonNone;
 
     reasonsToComposite |= overlapCompositingReason;
 
@@ -1001,8 +1000,7 @@ void
RenderLayerCompositor::computeCompositingRequirements(RenderLayer* ancestor
         // Here we know that all children and the layer's own contents can
blindly paint into
         // this layer's backing, until a descendant is composited. So, we don't
need to check
         // for overlap with anything behind this layer.
-        if (overlapMap)
-            overlapMap->beginNewOverlapTestingContext();
+        overlapMap.beginNewOverlapTestingContext();
         // This layer is going to be composited, so children can safely ignore
the fact that there's an
         // animation running behind this layer, meaning they can rely on the
overlap map testing again.
         childRecursionData.m_testingOverlap = true;
@@ -1030,35 +1028,32 @@ void
RenderLayerCompositor::computeCompositingRequirements(RenderLayer* ancestor
                 if (!willBeCompositedOrSquashed) {
                     // make layer compositing
                     childRecursionData.m_compositingAncestor = layer;
-                    if (overlapMap)
-                        overlapMap->beginNewOverlapTestingContext();
+                    overlapMap.beginNewOverlapTestingContext();
                     willBeCompositedOrSquashed = true;
                     willHaveForegroundLayer = true;
 
                     // FIXME: temporary solution for the first negative z-index
composited child:
                     //        re-compute the absBounds for the child so that we
can add the
                     //        negative z-index child's bounds to the new
overlap context.
-                    if (overlapMap) {
-                       
overlapMap->geometryMap().pushMappingsToAncestor(curNode->layer(), layer);
-                        IntRect childAbsBounds =
enclosingIntRect(overlapMap->geometryMap().absoluteRect(curNode->layer()->overlapBounds()));
-                        overlapMap->beginNewOverlapTestingContext();
-                        addToOverlapMap(*overlapMap, curNode->layer(),
childAbsBounds);
-                        overlapMap->finishCurrentOverlapTestingContext();
-                        overlapMap->geometryMap().popMappingsToAncestor(layer);
-                    }
+                   
overlapMap.geometryMap().pushMappingsToAncestor(curNode->layer(), layer);
+                    IntRect childAbsBounds =
enclosingIntRect(overlapMap.geometryMap().absoluteRect(curNode->layer()->overlapBounds()));
+                    overlapMap.beginNewOverlapTestingContext();
+                    addToOverlapMap(overlapMap, curNode->layer(),
childAbsBounds);
+                    overlapMap.finishCurrentOverlapTestingContext();
+                    overlapMap.geometryMap().popMappingsToAncestor(layer);
                 }
             }
         }
     }
 
-    if (overlapMap && willHaveForegroundLayer) {
+    if (willHaveForegroundLayer) {
         ASSERT(willBeCompositedOrSquashed);
         // A foreground layer effectively is a new backing for all subsequent
children, so
         // we don't need to test for overlap with anything behind this. So, we
can finish
         // the previous context that was accumulating rects for the negative
z-index
         // children, and start with a fresh new empty context.
-        overlapMap->finishCurrentOverlapTestingContext();
-        overlapMap->beginNewOverlapTestingContext();
+        overlapMap.finishCurrentOverlapTestingContext();
+        overlapMap.beginNewOverlapTestingContext();
         // This layer is going to be composited, so children can safely ignore
the fact that there's an
         // animation running behind this layer, meaning they can rely on the
overlap map testing again
         childRecursionData.m_testingOverlap = true;
@@ -1089,8 +1084,8 @@ void
RenderLayerCompositor::computeCompositingRequirements(RenderLayer* ancestor
     // All layers (even ones that aren't being composited) need to get added to
     // the overlap map. Layers that are not separately composited will paint
into their
     // compositing ancestor's backing, and so are still considered for overlap.
-    if (overlapMap && childRecursionData.m_compositingAncestor &&
!childRecursionData.m_compositingAncestor->isRootLayer())
-        addToOverlapMap(*overlapMap, layer, absBounds);
+    if (childRecursionData.m_compositingAncestor &&
!childRecursionData.m_compositingAncestor->isRootLayer())
+        addToOverlapMap(overlapMap, layer, absBounds);
 
     if (layer->stackingNode()->isStackingContext()) {
        
layer->setShouldIsolateCompositedDescendants(childRecursionData.m_hasUnisolatedCompositedBlendingDescendant);
@@ -1104,13 +1099,11 @@ void
RenderLayerCompositor::computeCompositingRequirements(RenderLayer* ancestor
     reasonsToComposite |= subtreeCompositingReasons;
     if (!willBeCompositedOrSquashed && canBeComposited(layer) &&
requiresCompositingOrSquashing(subtreeCompositingReasons)) {
         childRecursionData.m_compositingAncestor = layer;
-        if (overlapMap) {
-            // FIXME: this context push is effectively a no-op but needs to
exist for
-            // now, because the code is designed to push overlap information to
the
-            // second-from-top context of the stack.
-            overlapMap->beginNewOverlapTestingContext();
-            addToOverlapMap(*overlapMap, layer, absoluteDecendantBoundingBox);
-        }
+        // FIXME: this context push is effectively a no-op but needs to exist
for
+        // now, because the code is designed to push overlap information to the
+        // second-from-top context of the stack.
+        overlapMap.beginNewOverlapT…
(message too large)

[ojan, 2014-03-23 17:56:43]

On 2014/03/23 02:10:52, Ian Vollick wrote:
> I'd really like to chat with you about this next week, though, if you've got
> some time.

Yeah, lets do it. Maybe before/after/during the compositing sync-up?

[ojan, 2014-03-23 17:56:51]

The CQ bit was checked by ojan@chromium.org

[commit-bot: I haz the power, 2014-03-23 17:56:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ojan@chromium.org/208943005/20001

[commit-bot: I haz the power, 2014-03-23 17:57:07]

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

[commit-bot: I haz the power, 2014-03-23 17:57:08]

Failed to apply patch for
Source/core/rendering/compositing/RenderLayerCompositor.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/compositing/RenderLayerCompositor.cpp
  Hunk #1 FAILED at 487.
  Hunk #2 succeeded at 934 (offset 22 lines).
  Hunk #3 FAILED at 969.
  Hunk #4 succeeded at 1009 with fuzz 1 (offset 30 lines).
  Hunk #5 succeeded at 1030 (offset 30 lines).
  Hunk #6 FAILED at 1028.
  Hunk #7 succeeded at 1118 (offset 31 lines).
  Hunk #8 succeeded at 1133 (offset 31 lines).
  Hunk #9 succeeded at 1167 (offset 33 lines).
  Hunk #10 succeeded at 1193 (offset 33 lines).
  3 out of 10 hunks FAILED -- saving rejects to file
Source/core/rendering/compositing/RenderLayerCompositor.cpp.rej

Patch:       Source/core/rendering/compositing/RenderLayerCompositor.cpp
Index: Source/core/rendering/compositing/RenderLayerCompositor.cpp
diff --git a/Source/core/rendering/compositing/RenderLayerCompositor.cpp
b/Source/core/rendering/compositing/RenderLayerCompositor.cpp
index
66bd5b488179b5cf34a04121d8662ab5b137a1ba..219ca8f259edad85a753856d20faceac6d8b15d8
100644
--- a/Source/core/rendering/compositing/RenderLayerCompositor.cpp
+++ b/Source/core/rendering/compositing/RenderLayerCompositor.cpp
@@ -487,7 +487,7 @@ void
RenderLayerCompositor::updateCompositingLayersInternal()
             // scrolling and animation bounds is implemented
(crbug.com/252472).
             Vector<RenderLayer*> unclippedDescendants;
             IntRect absoluteDecendantBoundingBox;
-            computeCompositingRequirements(0, updateRoot,
&overlapTestRequestMap, recursionData, saw3DTransform, unclippedDescendants,
absoluteDecendantBoundingBox);
+            computeCompositingRequirements(0, updateRoot,
overlapTestRequestMap, recursionData, saw3DTransform, unclippedDescendants,
absoluteDecendantBoundingBox);
         }
 
         {
@@ -912,12 +912,11 @@ void RenderLayerCompositor::addToOverlapMap(OverlapMap&
overlapMap, RenderLayer*
 //      must be compositing so that its contents render over that child.
 //      This implies that its positive z-index children must also be
compositing.
 //
-void RenderLayerCompositor::computeCompositingRequirements(RenderLayer*
ancestorLayer, RenderLayer* layer, OverlapMap* overlapMap,
CompositingRecursionData& currentRecursionData, bool& descendantHas3DTransform,
Vector<RenderLayer*>& unclippedDescendants, IntRect&
absoluteDecendantBoundingBox)
+void RenderLayerCompositor::computeCompositingRequirements(RenderLayer*
ancestorLayer, RenderLayer* layer, OverlapMap& overlapMap,
CompositingRecursionData& currentRecursionData, bool& descendantHas3DTransform,
Vector<RenderLayer*>& unclippedDescendants, IntRect&
absoluteDecendantBoundingBox)
 {
     layer->stackingNode()->updateLayerListsIfNeeded();
 
-    if (overlapMap)
-        overlapMap->geometryMap().pushMappingsToAncestor(layer, ancestorLayer);
+    overlapMap.geometryMap().pushMappingsToAncestor(layer, ancestorLayer);
 
     // Clear the flag
     layer->setHasCompositingDescendant(false);
@@ -970,8 +969,8 @@ void
RenderLayerCompositor::computeCompositingRequirements(RenderLayer* ancestor
     }
 
     IntRect absBounds;
-    if (overlapMap && !layer->isRootLayer()) {
-        absBounds =
enclosingIntRect(overlapMap->geometryMap().absoluteRect(layer->overlapBounds()));
+    if (!layer->isRootLayer()) {
+        absBounds =
enclosingIntRect(overlapMap.geometryMap().absoluteRect(layer->overlapBounds()));
         // Setting the absBounds to 1x1 instead of 0x0 makes very little sense,
         // but removing this code will make JSGameBench sad.
         // See https://codereview.chromium.org/13912020/
@@ -980,8 +979,8 @@ void
RenderLayerCompositor::computeCompositingRequirements(RenderLayer* ancestor
     }
     absoluteDecendantBoundingBox = absBounds;
 
-    if (overlapMap && currentRecursionData.m_testingOverlap &&
!requiresCompositingOrSquashing(directReasons))
-        overlapCompositingReason = overlapMap->overlapsLayers(absBounds) ?
CompositingReasonOverlap : CompositingReasonNone;
+    if (currentRecursionData.m_testingOverlap &&
!requiresCompositingOrSquashing(directReasons))
+        overlapCompositingReason = overlapMap.overlapsLayers(absBounds) ?
CompositingReasonOverlap : CompositingReasonNone;
 
     reasonsToComposite |= overlapCompositingReason;
 
@@ -1001,8 +1000,7 @@ void
RenderLayerCompositor::computeCompositingRequirements(RenderLayer* ancestor
         // Here we know that all children and the layer's own contents can
blindly paint into
         // this layer's backing, until a descendant is composited. So, we don't
need to check
         // for overlap with anything behind this layer.
-        if (overlapMap)
-            overlapMap->beginNewOverlapTestingContext();
+        overlapMap.beginNewOverlapTestingContext();
         // This layer is going to be composited, so children can safely ignore
the fact that there's an
         // animation running behind this layer, meaning they can rely on the
overlap map testing again.
         childRecursionData.m_testingOverlap = true;
@@ -1030,35 +1028,32 @@ void
RenderLayerCompositor::computeCompositingRequirements(RenderLayer* ancestor
                 if (!willBeCompositedOrSquashed) {
                     // make layer compositing
                     childRecursionData.m_compositingAncestor = layer;
-                    if (overlapMap)
-                        overlapMap->beginNewOverlapTestingContext();
+                    overlapMap.beginNewOverlapTestingContext();
                     willBeCompositedOrSquashed = true;
                     willHaveForegroundLayer = true;
 
                     // FIXME: temporary solution for the first negative z-index
composited child:
                     //        re-compute the absBounds for the child so that we
can add the
                     //        negative z-index child's bounds to the new
overlap context.
-                    if (overlapMap) {
-                       
overlapMap->geometryMap().pushMappingsToAncestor(curNode->layer(), layer);
-                        IntRect childAbsBounds =
enclosingIntRect(overlapMap->geometryMap().absoluteRect(curNode->layer()->overlapBounds()));
-                        overlapMap->beginNewOverlapTestingContext();
-                        addToOverlapMap(*overlapMap, curNode->layer(),
childAbsBounds);
-                        overlapMap->finishCurrentOverlapTestingContext();
-                        overlapMap->geometryMap().popMappingsToAncestor(layer);
-                    }
+                   
overlapMap.geometryMap().pushMappingsToAncestor(curNode->layer(), layer);
+                    IntRect childAbsBounds =
enclosingIntRect(overlapMap.geometryMap().absoluteRect(curNode->layer()->overlapBounds()));
+                    overlapMap.beginNewOverlapTestingContext();
+                    addToOverlapMap(overlapMap, curNode->layer(),
childAbsBounds);
+                    overlapMap.finishCurrentOverlapTestingContext();
+                    overlapMap.geometryMap().popMappingsToAncestor(layer);
                 }
             }
         }
     }
 
-    if (overlapMap && willHaveForegroundLayer) {
+    if (willHaveForegroundLayer) {
         ASSERT(willBeCompositedOrSquashed);
         // A foreground layer effectively is a new backing for all subsequent
children, so
         // we don't need to test for overlap with anything behind this. So, we
can finish
         // the previous context that was accumulating rects for the negative
z-index
         // children, and start with a fresh new empty context.
-        overlapMap->finishCurrentOverlapTestingContext();
-        overlapMap->beginNewOverlapTestingContext();
+        overlapMap.finishCurrentOverlapTestingContext();
+        overlapMap.beginNewOverlapTestingContext();
         // This layer is going to be composited, so children can safely ignore
the fact that there's an
         // animation running behind this layer, meaning they can rely on the
overlap map testing again
         childRecursionData.m_testingOverlap = true;
@@ -1089,8 +1084,8 @@ void
RenderLayerCompositor::computeCompositingRequirements(RenderLayer* ancestor
     // All layers (even ones that aren't being composited) need to get added to
     // the overlap map. Layers that are not separately composited will paint
into their
     // compositing ancestor's backing, and so are still considered for overlap.
-    if (overlapMap && childRecursionData.m_compositingAncestor &&
!childRecursionData.m_compositingAncestor->isRootLayer())
-        addToOverlapMap(*overlapMap, layer, absBounds);
+    if (childRecursionData.m_compositingAncestor &&
!childRecursionData.m_compositingAncestor->isRootLayer())
+        addToOverlapMap(overlapMap, layer, absBounds);
 
     if (layer->stackingNode()->isStackingContext()) {
        
layer->setShouldIsolateCompositedDescendants(childRecursionData.m_hasUnisolatedCompositedBlendingDescendant);
@@ -1104,13 +1099,11 @@ void
RenderLayerCompositor::computeCompositingRequirements(RenderLayer* ancestor
     reasonsToComposite |= subtreeCompositingReasons;
     if (!willBeCompositedOrSquashed && canBeComposited(layer) &&
requiresCompositingOrSquashing(subtreeCompositingReasons)) {
         childRecursionData.m_compositingAncestor = layer;
-        if (overlapMap) {
-            // FIXME: this context push is effectively a no-op but needs to
exist for
-            // now, because the code is designed to push overlap information to
the
-            // second-from-top context of the stack.
-            overlapMap->beginNewOverlapTestingContext();
-            addToOverlapMap(*overlapMap, layer, absoluteDecendantBoundingBox);
-        }
+        // FIXME: this context push is effectively a no-op but needs to exist
for
+        // now, because the code is designed to push overlap information to the
+        // second-from-top context of the stack.
+        overlapMap.beginNewOverlapT…
(message too large)

[ojan, 2014-03-24 17:22:19]

The CQ bit was checked by ojan@chromium.org

[commit-bot: I haz the power, 2014-03-24 17:22:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ojan@chromium.org/208943005/60001

[commit-bot: I haz the power, 2014-03-24 17:25:40]

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

[commit-bot: I haz the power, 2014-03-24 17:25:41]

Try jobs failed on following builders:
  tryserver.blink on win_blink_compile_dbg

[esprehn, 2014-03-24 17:33:03]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-03-24 17:33:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ojan@chromium.org/208943005/60001

[commit-bot: I haz the power, 2014-03-24 17:34:32]

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

[commit-bot: I haz the power, 2014-03-24 17:34:33]

Try jobs failed on following builders:
  tryserver.blink on win_blink_compile_dbg

[ojan, 2014-03-24 22:57:11]

The CQ bit was checked by ojan@chromium.org

[commit-bot: I haz the power, 2014-03-24 22:57:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ojan@chromium.org/208943005/60001

[commit-bot: I haz the power, 2014-03-24 22:58:40]

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

[commit-bot: I haz the power, 2014-03-24 22:58:41]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[ojan, 2014-03-24 23:04:05]

The CQ bit was checked by ojan@chromium.org

[commit-bot: I haz the power, 2014-03-24 23:04:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ojan@chromium.org/208943005/60001

[commit-bot: I haz the power, 2014-03-24 23:05:26]

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

[commit-bot: I haz the power, 2014-03-24 23:05:27]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[ojan, 2014-03-24 23:38:49]

The CQ bit was checked by ojan@chromium.org

[commit-bot: I haz the power, 2014-03-24 23:38:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ojan@chromium.org/208943005/60001

[commit-bot: I haz the power, 2014-03-24 23:49:43]

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

[commit-bot: I haz the power, 2014-03-24 23:49:43]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[ojan, 2014-03-24 23:59:23]

The CQ bit was checked by ojan@chromium.org

[commit-bot: I haz the power, 2014-03-24 23:59:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ojan@chromium.org/208943005/60001

[commit-bot: I haz the power, 2014-03-25 00:01:40]

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

[commit-bot: I haz the power, 2014-03-25 00:01:41]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[ojan, 2014-03-25 01:36:23]

The CQ bit was checked by ojan@chromium.org

[commit-bot: I haz the power, 2014-03-25 01:36:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ojan@chromium.org/208943005/60001

[commit-bot: I haz the power, 2014-03-25 02:38:42]

Change committed as 169903