Bug 159670

Created attachment 283492 [details] Patch

Comment on attachment 283492 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283492&action=review > Source/WebCore/rendering/RenderBox.cpp:3138 > + LayoutUnit stretchedHeight = -1; Should we start using Optional values for the overrideLogicalContentHeight() output ? That way we might avoid this harcoded values. > Source/WebCore/rendering/RenderBox.cpp:3167 > + else if (stretchedHeight != -1) Can´t we check here (block.isGridItem() && block.hasOverrideLogicalContentHeight())) and assign the block.overrideLogicalContentHeight() directly ? > Source/WebCore/rendering/RenderBox.h:-676 > - Is this change really needed ? > Source/WebCore/rendering/RenderBoxModelObject.cpp:234 > +bool RenderBoxModelObject::hasAutoHeightOrContainingBlockWithAutoHeight(bool checkingContainingBlock) const We could add the argument to the already defined method, but setting false as default value. > Source/WebCore/rendering/RenderBoxModelObject.h:327 > + bool hasAutoHeightOrContainingBlockWithAutoHeight(bool checkingContainingBlock) const; ditto.

Comment on attachment 283492 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283492&action=review > Source/WebCore/rendering/RenderBox.h:635 > +#if ENABLE(CSS_GRID_LAYOUT) Wouldn't be protected enough visibility ?

Comment on attachment 283492 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283492&action=review Thanks for the review, I'm uploading a new version addressing your comments. >> Source/WebCore/rendering/RenderBox.cpp:3138 >> + LayoutUnit stretchedHeight = -1; > > Should we start using Optional values for the overrideLogicalContentHeight() output ? That way we might avoid this harcoded values. Maybe we should do it at some point. >> Source/WebCore/rendering/RenderBox.cpp:3167 >> + else if (stretchedHeight != -1) > > Can´t we check here (block.isGridItem() && block.hasOverrideLogicalContentHeight())) and assign the block.overrideLogicalContentHeight() directly ? Yes I can do it, I'm changing this part so the thing about stretchedHeight and using Optional is not relevant for this patch. >> Source/WebCore/rendering/RenderBox.h:635 >> +#if ENABLE(CSS_GRID_LAYOUT) > > Wouldn't be protected enough visibility ? No, because I want to call it from RenderBoxModelObject, which is the parent of RenderBox. >> Source/WebCore/rendering/RenderBox.h:-676 >> - > > Is this change really needed ? No a mistake in the patch, I'm removing this change in the new version. >> Source/WebCore/rendering/RenderBoxModelObject.cpp:234 >> +bool RenderBoxModelObject::hasAutoHeightOrContainingBlockWithAutoHeight(bool checkingContainingBlock) const > > We could add the argument to the already defined method, but setting false as default value. The thing is that from the outside we should always call it with hasAutoHeightOrContainingBlockWithAutoHeight(false). The "true" value is just an implementation detail to know if we're checking the element or the containing block. So, I prefer this approach of keeping the API simple (so nobody makes the mistake to call it with "true"). And we've the implementation detail hidden internally on the class. If we make the change in the public method, probably I'd use an enum instead of a bool.

Created attachment 283738 [details] Patch

Comment on attachment 283738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283738&action=review OK as is, but could probably be written in a clearer way. > Source/WebCore/rendering/RenderBoxModelObject.cpp:231 > + return hasAutoHeightOrContainingBlockWithAutoHeight(false); I think you defended the use of the boolean here by saying "if this was a public method I would use an enum", but I think this is still quite mysterious here at this call site even though it’s not a public member function. Instead these could be two separate functions. Combining them with the boolean doesn’t seem to add much value or avoid much code. I think it would be better to reorganize hasAutoHeightOrContainingBlockWithAutoHeight into multiple functions and change it so it becomes a loop instead of a recursive tail call. One function (A) would do the early exit for hasOverrideContainingBlockLogicalHeight (that would probably return a bool, or perhaps an Optional<bool>). Another function (B) would get the logical height and do those checks (would return an Optional<bool>). A third function (C) would advance from the object to the containing block (would return a RenderBlock&, except I’m not sure what should happen when we encounter a null containing block; the existing code seems to crash if that happens, because the loop checks for null but then the next line of code dereferences the pointer). A fourth function (D) would do the checks on the containing block (probably return Optional<bool>). Then the main function would contain the loop structure and call the others. Trying to write it as a single function, the code ends up being a bit peculiar, for example because containing blocks are RenderBlock, but then we dumb the pointer back down to a RenderBoxModelObject and then call is<RenderBox> on it which is guaranteed to be true if it’s the containing block. I think rearranging the logic could make it cleaner. It’s also wasteful to keep checking inQuirksMode during the containing block loop. If that’s true we will never enter the containing block loop. So maybe I would leave the quirks mode rule out of (B) and do it in the main function instead so it would only be done once before the loop. > Source/WebCore/rendering/RenderBoxModelObject.cpp:238 > + const auto* thisBox = is<RenderBox>(this) ? downcast<RenderBox>(this) : nullptr; > + if (thisBox && thisBox->isGridItem()) { auto* would do the same thing as const auto*, so I suggest not explicitly stating const I’m not a huge fan of using a null check instead of checking is<RenderBox> directly. I suggest writing: if (is<RenderBox>(*this)) { auto& box = downcast<RenderBox>(*this); if (box.isGridItem()) { Even though this idiom happens all over our code, it’s pretty ugly to have code specific to RenderBox and use is<RenderBox> instead of virtual functions. > Source/WebCore/rendering/RenderBoxModelObject.cpp:278 > + return cb->hasAutoHeightOrContainingBlockWithAutoHeight(true); Mysterious here too.

Ok, so this patch gets rotten (due to my fault as I was not working on the last stuff suggested by darin). The thing is that the bug has been fixed in the big code import from Blink in bug #168657. So I'll just upload the patch with the test, as it was not imported.

Created attachment 306895 [details] Patch for landing

Comment on attachment 306895 [details] Patch for landing Clearing flags on attachment: 306895 Committed r215264: <http://trac.webkit.org/changeset/215264>

All reviewed patches have been landed. Closing bug.