[webkit-reviews] review denied: [Bug 94984] Make RenderBox::computePositionedLogicalHeight const : [Attachment 160526] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Tony Chang <tony at chromium.org>'s
request for review:
Bug 94984: Make RenderBox::computePositionedLogicalHeight const
https://bugs.webkit.org/show_bug.cgi?id=94984

Attachment 160526: Patch
https://bugs.webkit.org/attachment.cgi?id=160526&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=160526&action=review


R- until we agree about naming and general design of the struct.

> Source/WebCore/rendering/RenderBox.cpp:1973
> +	   LogicalHeightComputedValues computedValues;

You had mentioned maybe setting computedValues.m_logicalHeight =
logicalHeight() and then only calling logicalHeight() in the top-level
computeLogicalHeight function. WDYT of that now? I'm on the fence about it.

Of course we can always do that later if we find we need it.

> Source/WebCore/rendering/RenderBox.h:310
> +    struct LogicalHeightComputedValues {
> +	   LogicalHeightComputedValues()

I imagine we'd want the same exact struct for computing width except
s/Height/Width and s/Top/Left. Would be nice if we could just make these names
a bit more generic. m_extent and m_positionedLocation? m_outOfFlowPosition?
m_outOfFlowStart? m_outOfFlowLocation?

One of those has to be good enough. :)

> Source/WebCore/rendering/RenderBox.h:320
> +	   void applyToRenderBox(RenderBox*);

I don't think we really gain anything by putting this method on the struct
itself. RenderBox::computeLogicalHeight can do the setting directly. In the
end, it will just be computeLogicalWidth and computeLogicalHeight that do the
setting anyways, right?