Bug 20947 – Refactor RenderObject::setStyle code

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

RenderObject::setStyle should be refactored to add some styleWillChange/styleDidChange hooks.

[Simon Fraser (smfr)]

This turned out to be trickier than I thought.

[Simon Fraser (smfr)]

Created attachment 24061 [details]
Patch to make setStyle() take a const RenderStyle*

[Simon Fraser (smfr)]

Created attachment 24062 [details]
Same patch, fixed line endings.

[Dave Hyatt]

Comment on attachment 24062 [details]
Same patch, fixed line endings.

r=me

[Simon Fraser (smfr)]

	M	WebCore/rendering/RenderObject.cpp
	M	WebCore/rendering/RenderButton.cpp
	M	WebCore/rendering/RenderSVGBlock.h
	M	WebCore/rendering/RenderTableCell.cpp
	M	WebCore/rendering/RenderBox.h
	M	WebCore/rendering/RenderBR.h
	M	WebCore/rendering/RenderObject.h
	M	WebCore/rendering/RenderSlider.cpp
	M	WebCore/rendering/RenderTableRow.h
	M	WebCore/rendering/RenderSVGGradientStop.h
	M	WebCore/rendering/RenderListItem.cpp
	M	WebCore/rendering/RenderBR.cpp
	M	WebCore/rendering/RenderMenuList.h
	M	WebCore/rendering/RenderListMarker.cpp
	M	WebCore/rendering/RenderTableRow.cpp
	M	WebCore/rendering/RenderSVGGradientStop.cpp
	M	WebCore/rendering/RenderText.cpp
	M	WebCore/rendering/RenderSlider.h
	M	WebCore/rendering/RenderListBox.h
	M	WebCore/rendering/RenderListItem.h
	M	WebCore/rendering/RenderInline.cpp
	M	WebCore/rendering/RenderWidget.h
	M	WebCore/rendering/RenderBox.cpp
	M	WebCore/rendering/RenderBlock.cpp
	M	WebCore/rendering/RenderFieldset.h
	M	WebCore/rendering/RenderWidget.cpp
	M	WebCore/rendering/RenderInline.h
	M	WebCore/rendering/RenderTextControl.cpp
	M	WebCore/rendering/RenderFileUploadControl.cpp
	M	WebCore/rendering/RenderTable.h
	M	WebCore/rendering/RenderFieldset.cpp
	M	WebCore/rendering/RenderBlock.h
	M	WebCore/rendering/RenderMenuList.cpp
	M	WebCore/rendering/RenderListMarker.h
	M	WebCore/rendering/RenderListBox.cpp
	M	WebCore/rendering/RenderReplaced.h
	M	WebCore/rendering/RenderTable.cpp
	M	WebCore/rendering/RenderReplaced.cpp
	M	WebCore/rendering/RenderSVGBlock.cpp
	M	WebCore/rendering/style/RenderStyle.h
	M	WebCore/rendering/RenderButton.h
	M	WebCore/rendering/RenderText.h
	M	WebCore/rendering/RenderFileUploadControl.h
	M	WebCore/rendering/RenderTextControl.h
	M	WebCore/rendering/RenderTableCell.h
	M	WebCore/ChangeLog
r37252 = ed87cd2dca2c7c2ee58f3127988e98a29dc693a2 (trunk)

Keeping bug open for further work.

[Simon Fraser (smfr)]

Created attachment 24125 [details]
Patch to remove setStyle overrides, and replace with styleWill/DidChange methods.

[Simon Fraser (smfr)]

This last patch removes most of the setStyle overrides, and replaces with styleWillChange/styleDidChange. The only ugly thing that would be good to fix is the need to store some state in RenderBox between the willChange and the didChange (RenderObject has to do the same thing).

[Simon Fraser (smfr)]

I verified that the patch in attachment 24125 [details] does not have a measurable impact on PLT.

[Simon Fraser (smfr)]

Created attachment 24162 [details]
styleWillChange/styleDidChange patch that uses class statics in RenderBox

So rather than add member variables to RenderBox to store state between styleWillChange and styleDidChange, this version uses static variables, which I don't really like, but should be fine.

[Simon Fraser (smfr)]

Here's another option that would avoid statics:

class BitStack
{
public:
    BitStack()
        : m_level(-1)
        , m_bits(0)
    { }
    void push(bool b)
    {
        ASSERT(m_level < (int)(8 * sizeof(m_bits)));
        setBit(++m_level, b);
    }
    bool pop()
    {
        ASSERT(m_level >= 0);
        bool val = getBit(m_level);
        --m_level;
        return val;
    }
private:
    void setBit(int bit, bool val) { m_bits = (m_bits & ~(1 << bit)) | (val << bit); }
    bool getBit(int bit) { return (m_bits & (1 << bit)) != 0; }
    
    int m_level;
    unsigned int m_bits;
};


void RenderFoo::styleWillChange(Diff, const RenderStyle, BitSet& state)
{
   bool preState = ....
   state.push(preState);
}

void RenderFoo::styleDidChange(Diff, const RenderStyle, BitSet& state)
{
  bool preState = state.pop();
 ....
}

This is nice in that it would avoid the proliferation of statics. It doesn't help with storage of types other than bools though.

[Simon Fraser (smfr)]

Created attachment 24194 [details]
Remove all but one setStyle override with styleWillChange/styleDidChange.

This patch is similar to the previous patch, but adds one more static, s_affectsParentBlock, in RenderObject to allow me to clean up its setStyle() a little more.

[Darin Adler]

Comment on attachment 24194 [details]
Remove all but one setStyle override with styleWillChange/styleDidChange.

This is better than making render objects bigger, but it would be nice to avoid the global variables.

I have two ideas:

1) We probably don't need a dynamic open ended system for this, since we don't host arbitrary renderer classes from add-ons or anything like that.

I propse a structure with some preallocated slots for the data needed by various RenderObject derived classes. We can comment it to make it clear which slots are for which derived classes. Something like this:

    struct CachedStateFromBeforeStyleChange {
        // RenderObject
        bool affectsParentBlock;

        // RenderBox
        bool boxWasFloating;
        bool boxHadOverflowClip;
    }

We'd pass a CachedStateFromBeforeStyleChange& into styleWillChange and a const CachedStateFromBeforeStyleChange& into styleDidChange.

2) A more-complex but possibly more elegant option would be to make versions of the functions that answer these key questions such as affectsParentBlock, isFloating, and hasOverflowClip take a RenderStyle* argument so that we could use them in the "did" functions to answer the questions about the old state. That might be tricky to get right and to maintain, but I think it's a cleaner solution so worth considering.

Lets try one of the non-global-variable solutions, so review- on this one.

If after thinking it over you want to stick with the global variable approach, feel free to set the review flag to review? again; I'm not strongly opposed.

[Simon Fraser (smfr)]

(In reply to comment #12)

> I propse a structure with some preallocated slots for the data needed by
> various RenderObject derived classes. We can comment it to make it clear which
> slots are for which derived classes. Something like this:
> 
>     struct CachedStateFromBeforeStyleChange {
>         // RenderObject
>         bool affectsParentBlock;
> 
>         // RenderBox
>         bool boxWasFloating;
>         bool boxHadOverflowClip;
>     }
> 
> We'd pass a CachedStateFromBeforeStyleChange& into styleWillChange and a const
> CachedStateFromBeforeStyleChange& into styleDidChange.

I pondered this, so it's nice to get agreement. I think this is a good solution.

> 2) A more-complex but possibly more elegant option would be to make versions of
> the functions that answer these key questions such as affectsParentBlock,
> isFloating, and hasOverflowClip take a RenderStyle* argument so that we could
> use them in the "did" functions to answer the questions about the old state.

The oldStyle is passed to styleDidChange, so I could have done this already. The problem is with state that gets set on the RenderObject and overrides what the style specifies (e.g. isPositioned in some cases). So we really do need to compute the old state in 'will', and consult it in 'did'.

[Darin Adler]

(In reply to comment #13)
> > 2) A more-complex but possibly more elegant option would be to make versions of
> > the functions that answer these key questions such as affectsParentBlock,
> > isFloating, and hasOverflowClip take a RenderStyle* argument so that we could
> > use them in the "did" functions to answer the questions about the old state.
> 
> The oldStyle is passed to styleDidChange, so I could have done this already.
> The problem is with state that gets set on the RenderObject and overrides what
> the style specifies (e.g. isPositioned in some cases). So we really do need to
> compute the old state in 'will', and consult it in 'did'.

I think you misunderstood my suggestion.

My suggestion is that functions like isPositioned would take a RenderStyle* argument and give the answer based on another style, not the current style of the object.

[Simon Fraser (smfr)]

(In reply to comment #14)
> My suggestion is that functions like isPositioned would take a RenderStyle*
> argument and give the answer based on another style, not the current style of
> the object.

What I was trying to say is that the correct state cannot be determined simply by looking at a RenderStyle (old or new). The state has been set on the object via some other logic, so we have to retrieve it directly from that RenderObject.

[Dave Hyatt]

I'm not really convinced this patch is making the code significantly cleaner.

[Dave Hyatt]

I really don't like the cached struct solution.  Now we're making new classes/structs just to refactor this code?  How is that simpler than what is already there now?

I'm not convinced styleWillChange and styleDidChange are even necessary, since I suspect sometimes whether code was placed before or after the base class setStyle was somewhat arbitrary.  I'm just not really convinced that this change needs to be made to the code.

[Dave Hyatt]

In other words, the fact that there even is state you need to somehow pass between the pre-change and post-change implies to me that this code is more connected than you think.

[Simon Fraser (smfr)]

(In reply to comment #18)
> In other words, the fact that there even is state you need to somehow pass
> between the pre-change and post-change implies to me that this code is more
> connected than you think.

Only 2 of the 20 overrides of setStyle() need to do the state caching thing, and they are "special" (RenderObject, RenderBox). Almost all the overrides are just there to do some work after the style has changed (or sometimes, before). So I think it makes sense to provide hooks they can override, rather than them overriding setStyle itself. Ideally, setStyle would be non-virtual.

My ultimate goal is to also factor out some of the repaint logic, and to do that I need to have RenderObject run some code after all the derived classes have done their post style-change work.

[Simon Fraser (smfr)]

Created attachment 24233 [details]
Final patch using CachedStyleChangeState struct

[Dave Hyatt]

Comment on attachment 24233 [details]
Final patch using CachedStyleChangeState struct

I hate the struct.  You've gone and taken something that had nothing to do with any of the derived classes (and only had to do with RenderBox) and made it part of the virtual function's argument list.  That makes no sense to me.  I think statics in the RenderBox file are a much better way to handle this.

[Dave Hyatt]

Maybe we should talk about this in a meeting or something.  The problem with this refactoring is it assume that the derived classes really meant "did" vs. "will."  In fact most of the time the ordering didn't matter at all.

Take, for example, RenderBlock.  setReplaced is in styleWillChange?  Did that matter?  No.  It  could just as easily have gone into styleDidChange.  Given that the setStyle method has been changed to take a const RenderStyle, there's basically no difference between did vs. will.

This refactoring, which in theory should be all about the derived classes, doesn't really matter to any of them.

The only time "did" vs. "will" is really meaning something is in the two complicated "core" classes, RenderBox and RenderObject.  I think it's worth backing up for a second and figuring out what problem you're trying to solve that is motivating this refactoring.  It may be that we could change only the two core classes (or shuffle code around in them) to get you what you need.

But right now I think adding these two new virtual methods and arbitrarily splitting the derived classes based off where the author of the derived class happened to place some line of code (before or after setStyle) is the wrong thing to do.

[Simon Fraser (smfr)]

> really meant "did" vs. "will."  In fact most of the time the ordering didn't matter at all

I was simply preserving existing ordering. I'll try to get a better feel for what has to happen when, and whether there's another approach that solves my problem.

[Dave Hyatt]

Actually I guess I do see some cases where the ordering did in fact matter (button and list markers), so I am wrong about did vs. will not being useful in the derived classes.

Go back to statics and I can review.  I can't stomach the struct (that has nothing to do with the derived classes) being passed around as an argument.

[Simon Fraser (smfr)]

Created attachment 24243 [details]
My final answer

[Dave Hyatt]

Comment on attachment 24243 [details]
My final answer

r=me, but you'll need to fix RenderScrollbarPart (which just landed).  It will need a styleDidChange method instead of setStyle.

[Simon Fraser (smfr)]

Committed r37464
	M	WebCore/rendering/RenderObject.cpp
	M	WebCore/rendering/RenderButton.cpp
	M	WebCore/rendering/RenderTableCell.cpp
	M	WebCore/rendering/RenderBox.h
	M	WebCore/rendering/RenderBR.h
	M	WebCore/rendering/RenderObject.h
	M	WebCore/rendering/RenderLayer.cpp
	M	WebCore/rendering/RenderSlider.cpp
	M	WebCore/rendering/RenderTableRow.h
	M	WebCore/rendering/RenderSVGGradientStop.h
	M	WebCore/rendering/RenderListItem.cpp
	M	WebCore/rendering/RenderBR.cpp
	M	WebCore/rendering/RenderMenuList.h
	M	WebCore/rendering/RenderScrollbarPart.cpp
	M	WebCore/rendering/RenderListMarker.cpp
	M	WebCore/rendering/RenderTableRow.cpp
	M	WebCore/rendering/RenderSVGGradientStop.cpp
	M	WebCore/rendering/RenderText.cpp
	M	WebCore/rendering/RenderSlider.h
	M	WebCore/rendering/RenderListBox.h
	M	WebCore/rendering/RenderListItem.h
	M	WebCore/rendering/RenderInline.cpp
	M	WebCore/rendering/RenderScrollbarPart.h
	M	WebCore/rendering/RenderWidget.h
	M	WebCore/rendering/RenderBox.cpp
	M	WebCore/rendering/RenderBlock.cpp
	M	WebCore/rendering/RenderFieldset.h
	M	WebCore/rendering/RenderWidget.cpp
	M	WebCore/rendering/RenderInline.h
	M	WebCore/rendering/RenderTextControl.cpp
	M	WebCore/rendering/RenderFileUploadControl.cpp
	M	WebCore/rendering/RenderTable.h
	M	WebCore/rendering/RenderFieldset.cpp
	M	WebCore/rendering/RenderBlock.h
	M	WebCore/rendering/RenderMenuList.cpp
	M	WebCore/rendering/RenderListMarker.h
	M	WebCore/rendering/RenderListBox.cpp
	M	WebCore/rendering/RenderReplaced.h
	M	WebCore/rendering/RenderTable.cpp
	M	WebCore/rendering/RenderReplaced.cpp
	M	WebCore/rendering/RenderLayer.h
	M	WebCore/rendering/RenderButton.h
	M	WebCore/rendering/RenderText.h
	M	WebCore/rendering/RenderFileUploadControl.h
	M	WebCore/rendering/RenderTextControl.h
	M	WebCore/rendering/RenderTableCell.h
	M	WebCore/ChangeLog
r37464 = a89b8cd8bcde931f8c857e1649fcdcd04735190e (trunk)

That'll do, pig.