Bug 94984

Created attachment 160526 [details] Patch

A small patch just to get an idea.

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

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

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?

Created attachment 160739 [details] Patch

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.

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.

Created attachment 160762 [details] Patch

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

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.

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?

Created attachment 160800 [details] Patch

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?

Created attachment 160807 [details] Patch for landing

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

Comment on attachment 160807 [details] Patch for landing Clearing flags on attachment: 160807 Committed r126802: <http://trac.webkit.org/changeset/126802>

All reviewed patches have been landed. Closing bug.