Bug 46496

Created attachment 223081 [details] Patch

Dave, would you review+ or review- please?

Comment on attachment 223081 [details] Patch Clearing review? flag on old patch that no longer applies to trunk.

Created attachment 441486 [details] Patch

Created attachment 441503 [details] Patch

Created attachment 441735 [details] Patch

Comment on attachment 441735 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441735&action=review > Source/WebCore/rendering/RenderBox.cpp:3334 > + LayoutUnit containerWidth; > + if (isOutOfFlowPositioned()) > + containerWidth = containingBlockLogicalWidthForPositioned(downcast<RenderBoxModelObject>(*container())); > + else if (isHorizontalWritingMode() == containingBlock()->isHorizontalWritingMode()) > + containerWidth = containingBlockLogicalWidthForContent(); > + else > + containerWidth = perpendicularContainingBlockLogicalHeight(); I’ve been noticing something like this, perhaps if not exactly the same, as a repeated idiom in patches recently correcting orthogonal flow behavior. Is there a way to add another function to help us get the repeated pattern right, without the code having an if statement like this in the middle or hurting efficiency? Or maybe it’s not repeated often enough.

Comment on attachment 441735 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441735&action=review >> Source/WebCore/rendering/RenderBox.cpp:3334 >> + containerWidth = perpendicularContainingBlockLogicalHeight(); > > I’ve been noticing something like this, perhaps if not exactly the same, as a repeated idiom in patches recently correcting orthogonal flow behavior. Is there a way to add another function to help us get the repeated pattern right, without the code having an if statement like this in the middle or hurting efficiency? Or maybe it’s not repeated often enough. I checked but I do not see it repeated enough. I do think we can provide something for the frequent "isHorizontalWritingMode() == containingBlock()->isHorizontalWritingMode()" like checks, but that sounds like a different bug.

Comment on attachment 441735 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441735&action=review >>> Source/WebCore/rendering/RenderBox.cpp:3334 >>> + containerWidth = perpendicularContainingBlockLogicalHeight(); >> >> I’ve been noticing something like this, perhaps if not exactly the same, as a repeated idiom in patches recently correcting orthogonal flow behavior. Is there a way to add another function to help us get the repeated pattern right, without the code having an if statement like this in the middle or hurting efficiency? Or maybe it’s not repeated often enough. > > I checked but I do not see it repeated enough. I do think we can provide something for the frequent "isHorizontalWritingMode() == containingBlock()->isHorizontalWritingMode()" like checks, but that sounds like a different bug. I always get confused when I see code like this sine the writing-direction defines (at least the part that's relevant here) the block direction for the content. Now the block level child box's writing direction (replaced box in this case) should have nothing to do with how we get the available width constraint from the containing block. It's only the containing block's writing direction that rules the logical width/height for its content (again, replaced box in this case). When the containing block has horizontal writing mode, the available width for its block level boxes matches its "physical" width, while with vertical direction the available width becomes its "physical" height so that the box level child boxes grow vertically with their widths. i.e. when you call containingBlockLogicalWidthForContent() it shouldn't matter whether _this_ box is horizontal or vertical in the context of finding the available width for the child box. I know this code copies the logic from computeReplacedLogicalHeightUsing, but we either do yet another flip somewhere to have correct final values or simply the functions/variable names don't mean logical values.

or put it simply: when the child box ask “how much I can grow (logical)horizontally in this containing block”, the child box's writing direction is irrelevant.

Comment on attachment 441735 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441735&action=review >>>> Source/WebCore/rendering/RenderBox.cpp:3334 >>>> + containerWidth = perpendicularContainingBlockLogicalHeight(); >>> >>> I’ve been noticing something like this, perhaps if not exactly the same, as a repeated idiom in patches recently correcting orthogonal flow behavior. Is there a way to add another function to help us get the repeated pattern right, without the code having an if statement like this in the middle or hurting efficiency? Or maybe it’s not repeated often enough. >> >> I checked but I do not see it repeated enough. I do think we can provide something for the frequent "isHorizontalWritingMode() == containingBlock()->isHorizontalWritingMode()" like checks, but that sounds like a different bug. > > I always get confused when I see code like this sine the writing-direction defines (at least the part that's relevant here) the block direction for the content. > Now the block level child box's writing direction (replaced box in this case) should have nothing to do with how we get the available width constraint from the containing block. It's only the containing block's writing direction that rules the logical width/height for its content (again, replaced box in this case). > When the containing block has horizontal writing mode, the available width for its block level boxes matches its "physical" width, while with vertical direction the available width becomes its "physical" height so that the box level child boxes grow vertically with their widths. i.e. when you call containingBlockLogicalWidthForContent() it shouldn't matter whether _this_ box is horizontal or vertical in the context of finding the available width for the child box. I know this code copies the logic from computeReplacedLogicalHeightUsing, but we either do yet another flip somewhere to have correct final values or simply the functions/variable names don't mean logical values. I agree it's confusing but I don't think there is a problem with the code. In this case we're interested in the size of the container in the inline size of the child. If both are parallel then we need the inline size of the container, otherwise we are looking for the block size of the container. So we do need to check whether or not they're orthogonal. The question is I guess whether we do it in the caller does or the callee. BTW orthogonal to this bug, should we push for using Inline/Block instead of LogicalWidth/LogicalHeight ?

Committed r284548 (243290@main): <https://commits.webkit.org/243290@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 441735 [details].

<rdar://problem/84468824>