Add StyleChangeState to get rid of a bunch of static state in the render tree.

Add StyleChangeState to get rid of a bunch of static state in the render tree.

We can get rid of a bunch of static variables in the render tree by passing
a StyleChangeState object between RenderObject::styleWillChange and styleDidChange.

This makes it more clear where these values come from, and will be neccesary if we
ever want to do parallel style changes or layout.

R=ojan@chromium.org,eseidel@chromium.org

[esprehn, 2013-04-05 00:01:54]

Please review.

[ojan, 2013-04-05 01:38:27]

lgtm

[eseidel, 2013-04-05 01:50:36]

This change looks fantastic.  It also is the highest-risk change that I've seen
yet in blink.  I think we should hold off on this until we have test bots and
perf bots up and running stably.  I would aim for early next week.  Is that
cool?

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
File Source/WebCore/rendering/RenderBlock.cpp (left):

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderBlock.cpp:125: bool
RenderBlock::s_canPropagateFloatIntoSibling = false;
Makes me wonder where this was ever cleared....

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
File Source/WebCore/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderBox.cpp:376: if (!state.hadOverflowClip)
Hopefully the repaint tests will catch if we got this wrong.

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
File Source/WebCore/rendering/RenderBox.h (left):

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderBox.h:695: static bool s_hadOverflowClip;
This scares me! I can't believe we had this.

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
File Source/WebCore/rendering/RenderFlexibleBox.h (right):

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderFlexibleBox.h:64: virtual void
styleDidChange(StyleDifference, const RenderStyle* oldStyle, const
StyleChangeState&) OVERRIDE;
Makes me wonder a bit why StyleChangeState doesn't just include the other two
arguments.

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
File Source/WebCore/rendering/RenderFlowThread.h (right):

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderFlowThread.h:95: virtual void
styleDidChange(StyleDifference, const RenderStyle* oldStyle, const
StyleChangeState&) OVERRIDE;
Thank you for cleaning this up.  We really need an automated way to find these
and fix all these virtual/overrides.

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
File Source/WebCore/rendering/RenderLayerModelObject.cpp (right):

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderLayerModelObject.cpp:93:
state.layerWasSelfPainting = hasSelfPaintingLayer();
This no longer needs to be conditional?

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
File Source/WebCore/rendering/RenderLayerModelObject.h (left):

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderLayerModelObject.h:68: // Used to store state
between styleWillChange and styleDidChange
I'm gonna cry.  But I can't tell if they're tears of joy at what you've done, or
sadness that this ever existed.

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
File Source/WebCore/rendering/RenderObject.cpp (left):

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderObject.cpp:121: bool
RenderObject::s_affectsParentBlock = false;
Do we have any clue why these came to be this way?  Were the statics faster?  Do
we have perf concerns for this change?

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
File Source/WebCore/rendering/RenderObject.h (right):

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderObject.h:168: affectsParentBlock(0),
I take it the compiler doesn't like "false"?

[ojan, 2013-04-11 02:12:37]

Still working on this? Seems like the infrastructure is all ready.

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
File Source/WebCore/rendering/RenderFlexibleBox.h (right):

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderFlexibleBox.h:64: virtual void
styleDidChange(StyleDifference, const RenderStyle* oldStyle, const
StyleChangeState&) OVERRIDE;
On 2013/04/05 01:50:36, Eric Seidel (Google) wrote:
> Makes me wonder a bit why StyleChangeState doesn't just include the other two
> arguments.

I think including StyleDifference makes sense. Including oldStyle makes less
sense though since styleWillChange takes newStyle as it's RenderStyle argument.

[esprehn, 2013-04-12 03:07:05]

On 2013/04/11 02:12:37, ojan wrote:
> Still working on this? Seems like the infrastructure is all ready.

Yeah I am. I got side tracked with a million other things. I'll try to land this
tomorrow.

[esprehn, 2013-04-17 03:29:01]

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
File Source/WebCore/rendering/RenderBlock.cpp (left):

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderBlock.cpp:125: bool
RenderBlock::s_canPropagateFloatIntoSibling = false;
On 2013/04/05 01:50:36, Eric Seidel (Google) wrote:
> Makes me wonder where this was ever cleared....

It never was, we just always call styleWillChange before calling styleDidChange
which checks it so the value is valid. If a subclass forgot to call
styleWillChange you'd get stale state.

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
File Source/WebCore/rendering/RenderFlexibleBox.h (right):

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderFlexibleBox.h:64: virtual void
styleDidChange(StyleDifference, const RenderStyle* oldStyle, const
StyleChangeState&) OVERRIDE;
On 2013/04/11 02:12:37, ojan wrote:
> On 2013/04/05 01:50:36, Eric Seidel (Google) wrote:
> > Makes me wonder a bit why StyleChangeState doesn't just include the other
two
> > arguments.
> 
> I think including StyleDifference makes sense. Including oldStyle makes less
> sense though since styleWillChange takes newStyle as it's RenderStyle
argument.

I'll fold StyleDifference into the context object.

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
File Source/WebCore/rendering/RenderFlowThread.h (right):

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderFlowThread.h:95: virtual void
styleDidChange(StyleDifference, const RenderStyle* oldStyle, const
StyleChangeState&) OVERRIDE;
On 2013/04/05 01:50:36, Eric Seidel (Google) wrote:
> Thank you for cleaning this up.  We really need an automated way to find these
> and fix all these virtual/overrides.

Yup, we need some tools support to clean all this up.

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
File Source/WebCore/rendering/RenderLayerModelObject.cpp (right):

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderLayerModelObject.cpp:93:
state.layerWasSelfPainting = hasSelfPaintingLayer();
On 2013/04/05 01:50:36, Eric Seidel (Google) wrote:
> This no longer needs to be conditional?

The conditional is still there, it's just inside hasSelfPaintingLayer().

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
File Source/WebCore/rendering/RenderObject.cpp (left):

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderObject.cpp:121: bool
RenderObject::s_affectsParentBlock = false;
On 2013/04/05 01:50:36, Eric Seidel (Google) wrote:
> Do we have any clue why these came to be this way?  Were the statics faster? 
Do
> we have perf concerns for this change?

I had a discussion on IRC with apple folks way back about this and the reason
was apparently because it was easier at the time but nothing specific.

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
File Source/WebCore/rendering/RenderObject.h (right):

https://codereview.chromium.org/13679002/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderObject.h:168: affectsParentBlock(0),
On 2013/04/05 01:50:36, Eric Seidel (Google) wrote:
> I take it the compiler doesn't like "false"?

Nah, I was just moving fast. I'll change to false.