Make compositingState explicit

Make compositingState explicit (re-land #2 with bogus ASSERT removed)

A lot of code uses isComposited() and compositedLayerMapping()
as booleans to check the compositing state of a RenderLayer or
RenderObject. However, in the code there are actually 3 different
states:

(1) not composited (paints into CompositedLayerMapping of a
    composited ancestor)
(2) paints into own CompositedLayerMapping
(3) has its own CompositedLayerMapping but actually still paints
    into composited ancestor.

Furthermore, upcoming changes to compositing will add a
fourth state:

(4) paints into a GraphicsLayer that is shared by a group of
    layers.

This patch removes isComposited() boolean check, and
replaces it with compositingState() that returns an enum.

Most call sites outside of RenderLayer, CompositedLayerMapping,
and RenderLayerCompositor have been updated to reflect
what states they really intended to check for.  Remaining call
sites have been replaced with the compositedLayerMapping()
accessor for now, which is exactly equivalent to the old
isComposited() boolean, and those sites will be updated in
follow-up patches.

BUG=261605
R=jamesr@chromium.org, jchaffraix@chromium.org, vollick@chromium.org

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

[shawnsingh, 2013-09-27 09:09:30]

Dear reviewers - PTAL.  (requesting that both of you take a look)

boolean isComposited() is not expressive enough to characterize three different
states - and soon-to-be four states when I add the squashing feature.  This
patch is a prerequisite for squashing, otherwise we will never be able to feel
comfortable that the squashing feature is correctly integrated into the rest of
the code.

I apologize this patch will probably be a pain to review.  For every place where
isComposited() is replaced with CompositingState(), we have to ask ourselves if
the new condition will do the right thing in all possible scenarios, and each
call site has a slight variation of the new conditions that were not completely
expressed by the old isComposited() boolean.

Here are the guidelines I used to convert from isComposited() to
compositingState() usage:

- remove isComposited() entirely so that anyone else is strongly encouraged to
understand the various compositingStates when adding their conditional logic.

- Usage related to accelerated animations needs to check whether the layer
PaintsIntoOwnBacking. By definition composited layers with accelerated
animations will definitely paint into their own backing.

- When dealing with the renderView, I assumed that if it is composited at all,
then it definitely PaintsIntoOwnBacking.

- The expression (layer->isComposited() &&
!layer->backing()->paintsIntoCompositedAncestor()) always means
PaintsIntoOwnBacking.

- The expression (layer->isComposited() &&
layer->backing()->paintsIntoCompositedAncestor()) always means
HasOwnBackingButPaintsIntoAncestor.

- The expressions (layer->isComposited()) and (!layer->isComposited) are the
most tricky. They varied on a case-by-case basis:
    (a) In the Insepctor, I assumed they just want to know if a backing exists. 
So the new condition checks if it paints into own backing OR has own backing but
paints into ancestor, the two cases where a layer has its own backing.
    (b) In RenderBox::paintClippingMask, I already know that a composited layer
with a mask will never be HasOwnBackingButPaintsIntoAncestor. The proof can be
seen in RenderLayerBacking::requiresOwnBackingStore().
    (c) Usage in RenderObject::adjustStyleDifference needs to conservatively
cause repaints for layers that don't paint into their very own isolated backing.
    (d) In RenderTreeAsText.cpp it was clear that the usage was purely
administrative just to dump out a backing if it exists.

- Places where I was uncertain for now, I just used the backing() accessor,
which is equivalent to the prior isComposited() boolean.
    - computePositionConstraint static helper function in
ScrollingCoordinator.cpp
    - isCandidateForOpaquenessTest static helper in RenderBox.cpp
    - RenderBoxModelObject::fixedBackgroundPaintsInLocalCoordinates
    - paintsWithTransparency, paintsWithFilters, paintsWithTransform --> I think
I understand then, but want verification from reviewers.
    - SVGRenderingContext::calculateTransformationToOutermostCoordinateSystem


And finally, it's worth noting that I will need to comb through the usage of
backing() in one or two follow-up patches.  I also left several FIXME comments
that would require changes that probably should have their own tests so I didn't
want to do them in this patch.  This patch is already big enough.

[shawnsingh, 2013-10-01 19:42:17]

Ping to reviewers.   My layer squashing work is blocked on this, and it gets
increasingly hard to rebase this on other changes going on in the code.

[Ian Vollick, 2013-10-01 19:46:21]

Still going through this, but here are my comments so far.

https://codereview.chromium.org/24921002/diff/4001/Source/core/page/scrolling...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/24921002/diff/4001/Source/core/page/scrolling...
Source/core/page/scrolling/ScrollingCoordinator.cpp:173: //        squashed
layers? For now, keeping this code equivalent to what it has been already.
My interpretation of this function:

If any layer contributing to |layer|'s backing is fixed, return the appropriate
constraint, else no constraint.

If that's right, should the stopping condition be layer &&
layer->compositingState() != NotComposited ?

https://codereview.chromium.org/24921002/diff/4001/Source/core/rendering/Rend...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/24921002/diff/4001/Source/core/rendering/Rend...
Source/core/rendering/RenderBoxModelObject.cpp:986: if (!rootLayer ||
!rootLayer->backing())
I think that checking backing is fine here, but you could do
paints-into-own-backing since this is the renderView.

https://codereview.chromium.org/24921002/diff/4001/Source/core/rendering/Rend...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/24921002/diff/4001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayer.cpp:253: // FIXME: shouldn't we return true if
this layer has a backing but paints into its ancestor?
Whatever we decide for paintsWithTransparency, the same should go for the other
paintsWith* fn's.

https://codereview.chromium.org/24921002/diff/4001/Source/core/rendering/Rend...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/24921002/diff/4001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayer.h:504: // FIXME: shouldn't we return true if
this layer has a backing but paints into its ancestor?
This should return true everywhere opacity won't be handled by the render
surface. I think you're right; if we're painting into our ancestor's backing and
we're transparent, the painting code has to handle the blending.

[Ian Vollick, 2013-10-02 19:12:25]

The rest seems reasonable. Julien, wdyt?

https://codereview.chromium.org/24921002/diff/4001/Source/core/rendering/Rend...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/24921002/diff/4001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayerCompositor.cpp:816: void
RenderLayerCompositor::computeCompositingRequirements(RenderLayer*
ancestorLayer, RenderLayer* layer, OverlapMap* overlapMap,
CompositingRecursionData& currentState, bool& layersChanged, bool&
descendantHas3DTransform, Vector<RenderLayer*>& unclippedDescendants)
bikeshed: currentData?

https://codereview.chromium.org/24921002/diff/4001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayerCompositor.cpp:1835: if (!layer->backing() ||
!layer->parent())
I think this one is paints into own backing.

https://codereview.chromium.org/24921002/diff/4001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayerCompositor.cpp:2255: if (viewLayer->backing()
&& viewLayer->backing()->backgroundLayerPaintsFixedRootBackground())
this one's paints into own backing.

[shawnsingh, 2013-10-03 03:04:03]

Responses inline.

https://codereview.chromium.org/24921002/diff/4001/Source/core/page/scrolling...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/24921002/diff/4001/Source/core/page/scrolling...
Source/core/page/scrolling/ScrollingCoordinator.cpp:173: //        squashed
layers? For now, keeping this code equivalent to what it has been already.
On 2013/10/01 19:46:22, Ian Vollick wrote:
> My interpretation of this function:
> 
> If any layer contributing to |layer|'s backing is fixed, return the
appropriate
> constraint, else no constraint.
> 
> If that's right, should the stopping condition be layer &&
> layer->compositingState() != NotComposited ?

I'm not sure if your phrasing means the same thing - to me, this function says
"return the position constraint, if it exists, of any fixed position layer that
lies between |layer| and the nearest ancestor backing."  That way the composited
layer gets fixed correctly.

So after thinking over it, I think the condition you're proposing is almost
correct, except that we should also exclude squashing layers when walking up to
find a fixed-position layer.  If we don't do that, then we run into issues where
a composited layer is positioned with respect to a squashed layer that it is not
a descendant of in the layer hierarchy.

https://codereview.chromium.org/24921002/diff/4001/Source/core/rendering/Rend...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/24921002/diff/4001/Source/core/rendering/Rend...
Source/core/rendering/RenderBoxModelObject.cpp:986: if (!rootLayer ||
!rootLayer->backing())
On 2013/10/01 19:46:22, Ian Vollick wrote:
> I think that checking backing is fine here, but you could do
> paints-into-own-backing since this is the renderView.

Done - except, it makes more sense to check if the root layer is NotComposited,
in my opinion, rather than !PaintsIntoOwnBacking. Such conditions used to be
equivalent when compositingState was binary, but now that's not true and
NotComposited is "more correct" to do here.

https://codereview.chromium.org/24921002/diff/4001/Source/core/rendering/Rend...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/24921002/diff/4001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayer.cpp:253: // FIXME: shouldn't we return true if
this layer has a backing but paints into its ancestor?
On 2013/10/01 19:46:22, Ian Vollick wrote:
> Whatever we decide for paintsWithTransparency, the same should go for the
other
> paintsWith* fn's.

done.

https://codereview.chromium.org/24921002/diff/4001/Source/core/rendering/Rend...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/24921002/diff/4001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayer.h:504: // FIXME: shouldn't we return true if
this layer has a backing but paints into its ancestor?
On 2013/10/01 19:46:22, Ian Vollick wrote:
> This should return true everywhere opacity won't be handled by the render
> surface. I think you're right; if we're painting into our ancestor's backing
and
> we're transparent, the painting code has to handle the blending.

done.

https://codereview.chromium.org/24921002/diff/4001/Source/core/rendering/Rend...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/24921002/diff/4001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayerCompositor.cpp:816: void
RenderLayerCompositor::computeCompositingRequirements(RenderLayer*
ancestorLayer, RenderLayer* layer, OverlapMap* overlapMap,
CompositingRecursionData& currentState, bool& layersChanged, bool&
descendantHas3DTransform, Vector<RenderLayer*>& unclippedDescendants)
On 2013/10/02 19:12:26, Ian Vollick wrote:
> bikeshed: currentData?

How about currentRecursionState?  "state" is actually not the wrong word.  It's
just not compositing state; it's the state of the recursive algorithm.   I used
that in the new patch, please feel free to bikeshed it more.

https://codereview.chromium.org/24921002/diff/4001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayerCompositor.cpp:1835: if (!layer->backing() ||
!layer->parent())
On 2013/10/02 19:12:26, Ian Vollick wrote:
> I think this one is paints into own backing.

Trying to do this makes it fail
compositing/overflow/scrollbars-with-clipped-owner.html.

https://codereview.chromium.org/24921002/diff/4001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayerCompositor.cpp:2255: if (viewLayer->backing()
&& viewLayer->backing()->backgroundLayerPaintsFixedRootBackground())
On 2013/10/02 19:12:26, Ian Vollick wrote:
> this one's paints into own backing.

Done.

[shawnsingh, 2013-10-03 03:05:46]

+jamesr for OWNERS review in Source/web/

[Julien - ping for review, 2013-10-03 20:08:35]

I think the direction is fine. The patch is gigantic and we are probably missing
some cases but I wouldn't be too concerned on that.

A high level comment is that we use interchangeably backing() and
compositingState() checks which makes the code more confusing unless you know
the implicit relationship between the 2. Ideally we should try to stick to one.

https://codereview.chromium.org/24921002/diff/16001/Source/core/page/scrollin...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/24921002/diff/16001/Source/core/page/scrollin...
Source/core/page/scrolling/ScrollingCoordinator.cpp:173: } while (layer &&
(layer->compositingState() == PaintsIntoOwnBacking || layer->compositingState()
== HasOwnBackingButPaintsIntoAncestor));
Couldn't we use backing() here instead to be consistent with the ASSERT above?

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.cpp:1351: if (!layer() ||
layer()->compositingState() != PaintsIntoOwnBacking)
How about we add an ASSERT that we never see HasOwnBackingButPaintsIntoAncestor
(per your original comment) as I was going to mention that it was fixing a bug
here?

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
Source/core/rendering/RenderBoxModelObject.cpp:986: if (!rootLayer ||
rootLayer->compositingState() == NotComposited)
Did you change your mind on this one as you said you would use backing()? Maybe
we can tighten the code (including ASSERTs & comments) based on Ian's review?

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:1281: //        the includeSelf option.
Nit: We usually don't align FIXMEs in Blink.

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:1447: if (backing())
This gets called from RenderLayer::beginTransparencyLayers, which filters
through paintsWithTransparency (ie !PaintsIntoOwnBacking) so wouldn't the only
condition where this triggers is HasOwnBackingButPaintsIntoAncestor? In this
case, it's probably better to state it explicitly (but I don't feel too strongly
about this).

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:4421: }
The PaintsIntoGroupBacking state is never reached so could we postpone adding
it?

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:183: struct
CompositingRecursionData {
This renamed feels like it's piggy backing on an already huge patch, it would be
nice to move it to its own (trivial) change.

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.cpp:1767: if (!isText() && (!hasLayer() ||
toRenderLayerModelObject(this)->layer()->compositingState() !=
PaintsIntoOwnBacking))
This looks like it is solving some bugs when we paint into our ancestor's
backing. Maybe worth adding some testing for it?

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.h:702: CompositingState compositingState()
const;
We discussed this over IM, I am not a huge fan of this change that exposes
internal details to non-rendering/ code but I don't think keeping isComposited()
is any better.

[shawnsingh, 2013-10-04 01:08:49]

Thanks for review.  I'll be back with a new patch here in a day or two.
 - with your comments addressed
 - rebaselined on top of a smaller separate CompositingState ->
CompositingRecursionData rename
 - rebaselined on top of the RenderLayerBacking -> CompositedLayerMapping rename
that should land tonight hopefully.

[shawnsingh, 2013-10-07 11:45:03]

Julien and Ian - PTAL - this patch is likely to meet both your approvals =)

The only last thing I need to do is try to create some layout tests that
reproduce subtle paintsIntoCompositedAncestor() bugs that are likely to be fixed
by this patch:
  - paintsWithTransform()
  - paintsWithFilters()
  - paintsWithTransparency()
  - adjustStyleDifference()

In these 4 places, the code seemed to be doing the wrong thing for the state
HasOwnBackingButPaintsIntoAncestor, and the patch probably fixes them.

I responded to Juliens comments below.  Thanks!

https://codereview.chromium.org/24921002/diff/16001/Source/core/page/scrollin...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/24921002/diff/16001/Source/core/page/scrollin...
Source/core/page/scrolling/ScrollingCoordinator.cpp:173: } while (layer &&
(layer->compositingState() == PaintsIntoOwnBacking || layer->compositingState()
== HasOwnBackingButPaintsIntoAncestor));
On 2013/10/03 20:08:36, Julien Chaffraix wrote:
> Couldn't we use backing() here instead to be consistent with the ASSERT above?

Actually yes, I am reverting this back to using backing()  (actually
compositedLayerMapping(), the new accessor).  The reason, however, is not just
to be consistent with the ASSERT.  I've actually convinced myself that, for now,
in cases where compositingState is queried for the sake of computing
positioning, usually they care whether the compositedLayerMapping() exists or
not.  They don't care whether the layer paints into ancestor or paints into own
backing.

I'm looking forward to fixing this over time.  As you've suggested, eventually
no code should use the compositedLayerBacking() accessor as a boolean condition
unless they really are dealing with allocating/deallocating it.  For now, a lot
of usage still exists for that.

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
Source/core/rendering/RenderBox.cpp:1351: if (!layer() ||
layer()->compositingState() != PaintsIntoOwnBacking)
On 2013/10/03 20:08:36, Julien Chaffraix wrote:
> How about we add an ASSERT that we never see
HasOwnBackingButPaintsIntoAncestor
> (per your original comment) as I was going to mention that it was fixing a bug
> here?

I'm not fixing any bug here, it's just a condition that I already knew would
never happen. The ASSERT is appropriate, so it's added on the latest patch now
=)

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
Source/core/rendering/RenderBoxModelObject.cpp:986: if (!rootLayer ||
rootLayer->compositingState() == NotComposited)
On 2013/10/03 20:08:36, Julien Chaffraix wrote:
> Did you change your mind on this one as you said you would use backing()?
Maybe
> we can tighten the code (including ASSERTs & comments) based on Ian's review?

I did address his comment, but the condition I chose is more appropriate.  You
can see my response to his comment... Please let me know if you think there is
still an issue here after seeing that.  =)

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:1281: //        the includeSelf option.
On 2013/10/03 20:08:36, Julien Chaffraix wrote:
> Nit: We usually don't align FIXMEs in Blink.

Done

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:1447: if (backing())
On 2013/10/03 20:08:36, Julien Chaffraix wrote:
> This gets called from RenderLayer::beginTransparencyLayers, which filters
> through paintsWithTransparency (ie !PaintsIntoOwnBacking) so wouldn't the only
> condition where this triggers is HasOwnBackingButPaintsIntoAncestor? In this
> case, it's probably better to state it explicitly (but I don't feel too
strongly
> about this).

This is a confusing one, and for the sake of moving forward I would like to
defer it to a follow-up patch... is that OK with you?

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:4421: }
On 2013/10/03 20:08:36, Julien Chaffraix wrote:
> The PaintsIntoGroupBacking state is never reached so could we postpone adding
> it?

OK, removed for now.

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:183: struct
CompositingRecursionData {
On 2013/10/03 20:08:36, Julien Chaffraix wrote:
> This renamed feels like it's piggy backing on an already huge patch, it would
be
> nice to move it to its own (trivial) change.

Done, landed this separately already, since it was needed for this patch.

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.cpp:1767: if (!isText() && (!hasLayer() ||
toRenderLayerModelObject(this)->layer()->compositingState() !=
PaintsIntoOwnBacking))
On 2013/10/03 20:08:36, Julien Chaffraix wrote:
> This looks like it is solving some bugs when we paint into our ancestor's
> backing. Maybe worth adding some testing for it?

will look into it. =)

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/24921002/diff/16001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.h:702: CompositingState compositingState()
const;
On 2013/10/03 20:08:36, Julien Chaffraix wrote:
> We discussed this over IM, I am not a huge fan of this change that exposes
> internal details to non-rendering/ code but I don't think keeping
isComposited()
> is any better.

I think the new version is slightly better because it forces people to recognize
the various states correctly.  As long as other code relies on knowing
compositing state, they may as well know it correctly =)

It should be our mission to gradually refactor the code so that this function
can be removed.

[Julien - ping for review, 2013-10-07 18:26:23]

lgtm

If there are any inconsistencies, we will see them in the follow-up patches and
can sort them out in trunk.

https://codereview.chromium.org/24921002/diff/34001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorLayerTreeAgent.cpp (right):

https://codereview.chromium.org/24921002/diff/34001/Source/core/inspector/Ins...
Source/core/inspector/InspectorLayerTreeAgent.cpp:207: if
(root->compositingState() == PaintsIntoOwnBacking || root->compositingState() ==
HasOwnBackingButPaintsIntoAncestor) {
Nit: compositedLayerMapping()?

[shawnsingh, 2013-10-07 20:24:23]

Jamesr can you please give OWNER review to web/ ?  Thanks in advance.

The patch is ready for landing after I address Julien's nit.  The four cases I
mentioned that may be actual bugfixes -- it turns out they are not wrong as they
are.  The code in RLC::requiresOwnBackingStore() guarantees that we would never
be in the hasOwnBackingButPaintsIntoAncestor state if those functions return
true.

[jamesr, 2013-10-07 20:49:17]

../web/.. lgtm

[commit-bot: I haz the power, 2013-10-07 21:17:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shawnsingh@chromium.org/24921002/43001

[commit-bot: I haz the power, 2013-10-08 00:32:39]

Change committed as 159068

[shawnsingh, 2013-10-08 04:16:27]

Committed patchset #7 manually as r159084 (presubmit successful).

[shawnsingh, 2013-10-08 04:17:41]

On 2013/10/08 04:16:27, shawnsingh wrote:
> Committed patchset #7 manually as r159084 (presubmit successful).

159068 was reverted due to bogus ASSERT.

159084 re-landed with the assertion removed.  Let's hope it sticks!

[Ian Vollick, 2013-10-08 14:57:21]

On 2013/10/08 04:17:41, shawnsingh wrote:
> On 2013/10/08 04:16:27, shawnsingh wrote:
> > Committed patchset #7 manually as r159084 (presubmit successful).
> 
> 159068 was reverted due to bogus ASSERT.
> 
> 159084 re-landed with the assertion removed.  Let's hope it sticks!

slgtm

[shawnsingh, 2013-10-08 15:01:49]

Committed patchset #9 manually as r159118 (presubmit successful).