Bug 94982 - Make const versions of computeLogical{Height,Width}
Summary: Make const versions of computeLogical{Height,Width}
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Depends on: 94984 95255 95595 95787 95907 96129
Blocks: 94855
  Show dependency treegraph
Reported: 2012-08-24 17:02 PDT by Tony Chang
Modified: 2012-09-07 14:27 PDT (History)
2 users (show)

See Also:


Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2012-08-24 17:02:15 PDT
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 
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 
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!
Comment 1 Tony Chang 2012-08-24 17:02:37 PDT
I'm going to split the work into multiple smaller patches, although I hope to get it all landed in about a weeks time.
Comment 2 Tony Chang 2012-09-07 14:27:16 PDT
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 
14:22 < dhyatt> tony^work: nah
14:22 < dhyatt> tony^work: i don't think you need to rename