[Webkit-unassigned] [Bug 94982] New: Make const versions of computeLogical{Height, Width}

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=94982

           Summary: Make const versions of computeLogical{Height,Width}
           Product: WebKit
           Version: 528+ (Nightly build)
          Platform: Unspecified
        OS/Version: Unspecified
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: Layout and Rendering
        AssignedTo: webkit-unassigned at lists.webkit.org
        ReportedBy: tony at chromium.org
                CC: hyatt at apple.com, ojan at chromium.org


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!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.