DeprecatedPaintLayerStackingNode should walk DPL

DeprecatedPaintLayerStackingNode should walk DPL

https://codereview.chromium.org/1199413006 changed the way we
collect DeprecatedPaintLayers for the purpose of painting.
The change made the implicit assumptions that stacking contexts
would have DeprecatedPaintLayer, else there is no way we could
correctly paint. It turns out that this is not an assumption
that holds in our code. The reason is that some LayoutObject
just can't have DeprecatedPaintLayer (any objects that are not
LayoutBoxModelObject) or they don't want one (LayoutTableCol),
even if they should have one.

The old code worked because it didn't discriminate these cases
and was a lot more robust to the widespread abuse in the code.
Thus the logical solution is to revert the change.

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

[Julien - ping for review, 2015-09-19 01:02:43]

jchaffraix@chromium.org changed reviewers:
+ dsinclair@chromium.org, leviw@chromium.org

[Julien - ping for review, 2015-09-19 01:02:43]

+Levi: Real review this time. It's a rollout of a change Dan reviewed.
+Dan: FYI (though I wouldn't mind a review)

[chrishtr, 2015-09-21 17:18:18]

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

[chrishtr, 2015-09-21 17:18:18]

Why not just revert https://codereview.chromium.org/1199413006 ?

It would be sad not to merge this fix into M46 also.

[chrishtr, 2015-09-21 17:18:58]

On 2015/09/21 at 17:18:18, chrishtr wrote:
> Why not just revert https://codereview.chromium.org/1199413006 ?
> 
> It would be sad not to merge this fix into M46 also.

(crbug.com/527927 has 24 stars)

[Julien - ping for review, 2015-09-21 17:54:36]

On 2015/09/21 at 17:18:58, chrishtr wrote:
> On 2015/09/21 at 17:18:18, chrishtr wrote:
> > Why not just revert https://codereview.chromium.org/1199413006 ?
> > 
> > It would be sad not to merge this fix into M46 also.
> 
> (crbug.com/527927 has 24 stars)

It will be merged into M46. I don't think either a full revert or this change
will be a clean revert too. 

It's a partial revert as I think keeping track of LayoutObject inside DPLClipper
is a good change. What's reverted is the part that matters for the bug (the
algorithmic change in how we collect DPLStackingNodes that introduced the
regression).

[chrishtr, 2015-09-21 17:58:13]

On 2015/09/21 at 17:54:36, jchaffraix wrote:
> On 2015/09/21 at 17:18:58, chrishtr wrote:
> > On 2015/09/21 at 17:18:18, chrishtr wrote:
> > > Why not just revert https://codereview.chromium.org/1199413006 ?
> > > 
> > > It would be sad not to merge this fix into M46 also.
> > 
> > (crbug.com/527927 has 24 stars)
> 
> It will be merged into M46. I don't think either a full revert or this change
will be a clean revert too. 
> 
> It's a partial revert as I think keeping track of LayoutObject inside
DPLClipper is a good change. What's reverted is the part that matters for the
bug (the algorithmic change in how we collect DPLStackingNodes that introduced
the regression).

A full revert will be safer for production stability though right?

[Julien - ping for review, 2015-09-21 22:44:36]

On 2015/09/21 at 17:58:13, chrishtr wrote:
> On 2015/09/21 at 17:54:36, jchaffraix wrote:
> > On 2015/09/21 at 17:18:58, chrishtr wrote:
> > > On 2015/09/21 at 17:18:18, chrishtr wrote:
> > > > Why not just revert https://codereview.chromium.org/1199413006 ?
> > > > 
> > > > It would be sad not to merge this fix into M46 also.
> > > 
> > > (crbug.com/527927 has 24 stars)
> > 
> > It will be merged into M46. I don't think either a full revert or this
change will be a clean revert too. 
> > 
> > It's a partial revert as I think keeping track of LayoutObject inside
DPLClipper is a good change. What's reverted is the part that matters for the
bug (the algorithmic change in how we collect DPLStackingNodes that introduced
the regression).
> 
> A full revert will be safer for production stability though right?

Right, if by safer, you mean easier-to-get-right. In this case, the full revert
will be bigger, thus with more parts of having to merge and potentially more
manual merging and screw-ups possible. All in all, the 2 options are equivalent
IMHO but the one from this patch leaves us in a better world after the revert.

Let me know if what I wrote makes sense.

[chrishtr, 2015-09-21 23:07:29]

On 2015/09/21 at 22:44:36, jchaffraix wrote:
> On 2015/09/21 at 17:58:13, chrishtr wrote:
> > On 2015/09/21 at 17:54:36, jchaffraix wrote:
> > > On 2015/09/21 at 17:18:58, chrishtr wrote:
> > > > On 2015/09/21 at 17:18:18, chrishtr wrote:
> > > > > Why not just revert https://codereview.chromium.org/1199413006 ?
> > > > > 
> > > > > It would be sad not to merge this fix into M46 also.
> > > > 
> > > > (crbug.com/527927 has 24 stars)
> > > 
> > > It will be merged into M46. I don't think either a full revert or this
change will be a clean revert too. 
> > > 
> > > It's a partial revert as I think keeping track of LayoutObject inside
DPLClipper is a good change. What's reverted is the part that matters for the
bug (the algorithmic change in how we collect DPLStackingNodes that introduced
the regression).
> > 
> > A full revert will be safer for production stability though right?
> 
> Right, if by safer, you mean easier-to-get-right. In this case, the full
revert will be bigger, thus with more parts of having to merge and potentially
more manual merging and screw-ups possible. All in all, the 2 options are
equivalent IMHO but the one from this patch leaves us in a better world after
the revert.
> 
> Let me know if what I wrote makes sense.

Yes, easier to get right. I would prefer you try to merge a full revert.

[Julien - ping for review, 2015-09-22 00:36:15]

Now with the full revert.

[chrishtr, 2015-09-22 20:39:35]

lgtm

LGTM if it's just a clean revert.

[Julien - ping for review, 2015-09-22 23:02:50]

The CQ bit was checked by jchaffraix@chromium.org

[commit-bot: I haz the power, 2015-09-22 23:03:18]

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

[commit-bot: I haz the power, 2015-09-23 00:24:41]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202653