[webkit-reviews] review granted: [Bug 223569] [css-contain] Support contain: layout : [Attachment 425874] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Rob Buis <rbuis at igalia.com>'s request
for review:
Bug 223569: [css-contain] Support contain: layout
https://bugs.webkit.org/show_bug.cgi?id=223569

Attachment 425874: Patch

https://bugs.webkit.org/attachment.cgi?id=425874&action=review




--- Comment #20 from Darin Adler <darin at apple.com> ---
Comment on attachment 425874
  --> https://bugs.webkit.org/attachment.cgi?id=425874
Patch

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

No need for a feature flag?

> Source/WebCore/rendering/RenderBlock.cpp:2578
> +	   return Optional<LayoutUnit>();

WTF::nullopt is the way we normally write this; not sure why we’re using
something different here

> Source/WebCore/rendering/RenderBlock.cpp:2596
> +	   return Optional<LayoutUnit>(synthesizedBaselineFromBorderBox(*this,
lineDirection) + (lineDirection == HorizontalLine ? marginBottom():
marginLeft()));

We normally put a space before the ":".

Also, is the Optional<LayoutUnit> really needed? Can we just write LayoutUnit
instead? Or nothing at all?

> Source/WebCore/rendering/RenderObject.h:189
> +    bool isAtomicInlineLevelBox() const
> +    {
> +	   return style().isDisplayInlineType() && !(style().display() ==
DisplayType::Inline && !isReplaced());
> +    }

In a large class like this, it’s best to keep multi-line function bodies (even
if only one line has code) out of the class definition so we can see an
overview of what’s in the class rather than have it interspersed with code.
Instead we can just put an inline implementation after the class. See
RenderObject::isBeforeContent for an example.

> Source/WebCore/rendering/RenderObject.h:191
> +    inline bool shouldApplyLayoutContainment() const

The inline here is unneeded and redundant if we are putting the function body
inside the class.

Or we can move this out.

I’m a bit uncomfortable adding more operations to RenderObject itself. Seems
like the class is getting out of hand. Other options include putting this in
some derived class, or writing it as a free function that takes a const
RenderObject& argument, since nothing in these two functions requires access to
anything private.