94982 2012-08-24 17:02:15 -0700 Make const versions of computeLogical{Height,Width} 2012-09-07 14:27:16 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 94984 95255 95595 95787 95907 96129 94855 1 tony webkit-unassigned hyatt ojan oldest_to_newest 704528 0 tony 2012-08-24 17:02:15 -0700 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! 704529 1 tony 2012-08-24 17:02:37 -0700 I'm going to split the work into multiple smaller patches, although I hope to get it all landed in about a weeks time. 714983 2 tony 2012-09-07 14:27:16 -0700 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