Bug 94982 – Make const versions of computeLogical{Height,Width}

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

[Tony Chang]

tl;dr version: computeLogical{Height,Width} actually compute values then set the height/width and margin values for a box.  This leads to awkwardness if you need the value, but don't want to set it.  Instead, we want const versions of these functions that return the height/width and margin values.

IRC discussion with Hyatt below:

12:18 < tony^work> dhyatt: while you're here, what do you think about making a 
                   helper function like computeLogicalHeight that returns the 
                   height and top rather than setting the values.
12:18 < tony^work> computeLogicalHeight would just call this function
12:20 < dhyatt> tony^work: oh i absolutely want to do that
12:20 < dhyatt> tony^work: for width and height
12:20 < dhyatt> tony^work: you can see FIXMES in the code where i say just that
12:20 < tony^work> dhyatt: ok, I can make the changes for height
12:20 < tony^work> we need it for column flexboxes
12:20 < dhyatt> tony^work: regions have this hack where they pull out all old 
                values
12:20 < tony^work> probably for grid too
12:20 < dhyatt> tony^work: and cache them
12:20 < dhyatt> and then call the function
12:20 < dhyatt> and restore the values
12:21 < dhyatt> tony^work: i would do it for both width and height. seems silly 
                to have one do it and the other not.
12:21 < tony^work> dhyatt: ok, I'll look into doing it for both
12:21 < dhyatt> tony^work: you would be doing me a great service if you did it 
                for both :)
12:21 < ojan> surely it can be done in two separate patches though
12:21 < dhyatt> tony^work: but yeah absolutely want things changed to work that 
                way
12:22 < dhyatt> ojan: oh of course
12:22 < dhyatt> ojan: just saying i don't want to see height changed and then 
                width not changed for months
12:22 < ojan> yeah, makes sense
12:22 < dhyatt> ideally someone does both as two tasks that happen close 
                together :)
12:22 < tony^work> ok, I'll do both
12:23 < dhyatt> tony^work: in particular admire the disgusting casting away of 
                the const in renderBoxRegionInfo
(snip example in RenderBox.cpp)
12:24 < dhyatt> i would suggest making little structs to hold the margins and 
                the width/height
12:25 < dhyatt> can still keep the version of the function that fills in the 
                members for you of course, but it would become nonvirtual
12:25 < dhyatt> and just call the virtual one to get the struct
12:25 < dhyatt> and fill the members in from that
12:25 < dhyatt> and then patch a bunch of call sites to use the struct-based 
                one when you don't want to mutate your renderobject
12:25 < dhyatt> and make the method const so we catch the baddies who try!

[Tony Chang]

I'm going to split the work into multiple smaller patches, although I hope to get it all landed in about a weeks time.

[Tony Chang]

I'm going to declare this done. I have some related cleanup (cleaning up some hacks that save the old value, call computeLogical*, then restore the value), but we don't need this meta bug anymore.


14:13 < tony^work> dhyatt: I'm mostly done with adding const versions of 
                   computeLogical{Width,Height} (see 
                   http://trac.webkit.org/changeset/127914).  Do you think it's 
                   worthwhile to rename those functions to 
                   computeAndSetLogical{Width,Height}?
14:22 < dhyatt> tony^work: nah
14:22 < dhyatt> tony^work: i don't think you need to rename