Bug 20947 - Refactor RenderObject::setStyle code
Summary: Refactor RenderObject::setStyle code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-09-19 12:40 PDT by Simon Fraser (smfr)
Modified: 2008-10-09 20:16 PDT (History)
4 users (show)

See Also:


Attachments
Patch to make setStyle() take a const RenderStyle* (31.34 KB, patch)
2008-10-03 12:46 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Same patch, fixed line endings. (31.34 KB, patch)
2008-10-03 12:51 PDT, Simon Fraser (smfr)
hyatt: review+
Details | Formatted Diff | Diff
Patch to remove setStyle overrides, and replace with styleWill/DidChange methods. (51.77 KB, patch)
2008-10-06 14:49 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
styleWillChange/styleDidChange patch that uses class statics in RenderBox (52.96 KB, patch)
2008-10-07 16:52 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Remove all but one setStyle override with styleWillChange/styleDidChange. (53.40 KB, patch)
2008-10-08 11:39 PDT, Simon Fraser (smfr)
darin: review-
Details | Formatted Diff | Diff
Final patch using CachedStyleChangeState struct (55.74 KB, patch)
2008-10-09 12:48 PDT, Simon Fraser (smfr)
hyatt: review-
Details | Formatted Diff | Diff
My final answer (57.85 KB, patch)
2008-10-09 15:52 PDT, Simon Fraser (smfr)
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2008-09-19 12:40:44 PDT
RenderObject::setStyle should be refactored to add some styleWillChange/styleDidChange hooks.
Comment 1 Simon Fraser (smfr) 2008-09-19 17:08:43 PDT
This turned out to be trickier than I thought.
Comment 2 Simon Fraser (smfr) 2008-10-03 12:46:10 PDT
Created attachment 24061 [details]
Patch to make setStyle() take a const RenderStyle*
Comment 3 Simon Fraser (smfr) 2008-10-03 12:51:02 PDT
Created attachment 24062 [details]
Same patch, fixed line endings.
Comment 4 Dave Hyatt 2008-10-03 13:06:11 PDT
Comment on attachment 24062 [details]
Same patch, fixed line endings.

r=me
Comment 5 Simon Fraser (smfr) 2008-10-03 13:20:46 PDT
	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.
Comment 6 Simon Fraser (smfr) 2008-10-06 14:49:07 PDT
Created attachment 24125 [details]
Patch to remove setStyle overrides, and replace with styleWill/DidChange methods.
Comment 7 Simon Fraser (smfr) 2008-10-06 14:50:21 PDT
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).
Comment 8 Simon Fraser (smfr) 2008-10-06 18:01:34 PDT
I verified that the patch in attachment 24125 [details] does not have a measurable impact on PLT.
Comment 9 Simon Fraser (smfr) 2008-10-07 16:52:56 PDT
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.
Comment 10 Simon Fraser (smfr) 2008-10-07 16:55:52 PDT
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.
Comment 11 Simon Fraser (smfr) 2008-10-08 11:39:54 PDT
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.
Comment 12 Darin Adler 2008-10-08 13:03:11 PDT
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.
Comment 13 Simon Fraser (smfr) 2008-10-08 13:15:09 PDT
(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'.
Comment 14 Darin Adler 2008-10-08 14:03:43 PDT
(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.
Comment 15 Simon Fraser (smfr) 2008-10-08 14:11:03 PDT
(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.
Comment 16 Dave Hyatt 2008-10-08 15:25:39 PDT
I'm not really convinced this patch is making the code significantly cleaner.

Comment 17 Dave Hyatt 2008-10-08 15:28:00 PDT
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.

Comment 18 Dave Hyatt 2008-10-08 15:33:12 PDT
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.

Comment 19 Simon Fraser (smfr) 2008-10-08 15:48:46 PDT
(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.
Comment 20 Simon Fraser (smfr) 2008-10-09 12:48:36 PDT
Created attachment 24233 [details]
Final patch using CachedStyleChangeState struct
Comment 21 Dave Hyatt 2008-10-09 12:53:49 PDT
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.
Comment 22 Dave Hyatt 2008-10-09 12:59:45 PDT
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.
Comment 23 Simon Fraser (smfr) 2008-10-09 13:07:15 PDT
> 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.
Comment 24 Dave Hyatt 2008-10-09 13:07:20 PDT
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.
Comment 25 Simon Fraser (smfr) 2008-10-09 15:52:03 PDT
Created attachment 24243 [details]
My final answer
Comment 26 Dave Hyatt 2008-10-09 19:56:31 PDT
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.
Comment 27 Simon Fraser (smfr) 2008-10-09 20:16:13 PDT
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.