Bug 94984 – Make RenderBox::computePositionedLogicalHeight const

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Chang]

Make RenderBox::computePositionedLogicalHeight const

[Tony Chang]

Created attachment 160526 [details]
Patch

[Tony Chang]

A small patch just to get an idea.

[WebKit Review Bot]

Comment on attachment 160526 [details]
Patch

Attachment 160526 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13603299

New failing tests:
inspector/elements/highlight-node-scaled.html

[WebKit Review Bot]

Created attachment 160536 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Ojan Vafai]

Comment on attachment 160526 [details]
Patch

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?

[Tony Chang]

Created attachment 160739 [details]
Patch

[Tony Chang]

Comment on attachment 160526 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160526&action=review

>> 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.

I think we should still do it, but it should be in a separate patch.  It's a logically different refactor.

>> Source/WebCore/rendering/RenderBox.h:310
>> +        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. :)

I think for readability, it's less confusing to have m_logicalHeight and m_logicalTop.  My current thinking is that we'll end up with 3 structs (one for width, one for height, one for margins).  I'm not sure until I start looking into computeInlineDirectionMargins, which is why I dislike generalizing names before they need to be.

The margin struct may also gain methods like setMargin*ForChild.

I've renamed the struct to LogicalExtentComputedValues for now, but it'll probably end up changing.

>> 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?

If computeLogicalWidth and computeLogicalHeight both do the setting, it'll be duplicate code, right?  Also, it makes the code diffs smaller since I'll be moving the applyToRenderBox code around during this refactoring process.

[Ojan Vafai]

Comment on attachment 160739 [details]
Patch

You're right. We can easily change all these things once the refactor is done and we have a better picture of how things will look.

[Tony Chang]

Created attachment 160762 [details]
Patch

[Tony Chang]

inspector/elements/highlight-node-scaled.html failing was real.  We need to set the start/end margins rather than overwriting them with 0.

[Dave Hyatt]

Comment on attachment 160762 [details]
Patch

We talked about this in IRC. I think we want a separate margin struct, and then I'd like a single common struct we can use for either width or height that contains a position, an extent, and the margins.

[Ojan Vafai]

Comment on attachment 160762 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160762&action=review

We don't need to do this for marginBefore/marginAfter?

> Source/WebCore/rendering/RenderBox.cpp:1976
> +        computedValues.m_marginStart = marginStart();
> +        computedValues.m_marginEnd = marginEnd();

We don't need to do this for marginBefore/marginAfter?

[Tony Chang]

Created attachment 160800 [details]
Patch

[Ojan Vafai]

Comment on attachment 160800 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160800&action=review

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

So many plurals. How about MarginComputedValues?

[Tony Chang]

Created attachment 160807 [details]
Patch for landing

[Tony Chang]

(In reply to comment #14)
> (From update of attachment 160800 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160800&action=review
> 
> > Source/WebCore/rendering/RenderBox.h:310
> > +    struct MarginsComputedValues {
> > +        MarginsComputedValues()
> 
> So many plurals. How about MarginComputedValues?

Went with ComputedMarginValues.

[WebKit Review Bot]

Comment on attachment 160807 [details]
Patch for landing

Clearing flags on attachment: 160807

Committed r126802: <http://trac.webkit.org/changeset/126802>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.