Bug 184951 – [LFC] Implement LayoutContexet::layout() and its dependencies.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[zalan]

LFC -> Layout Formatting Context

[zalan]

Created attachment 338796 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 338796 [details]
Patch

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

> Source/WebCore/layout/FormattingContext.h:50
> +    const Box& root() const { return *m_root; }

You can't return a ref to a weak object. It might be null!

Can't we guarantee that the FormattingContext outlines the root and not use a WeakPtr?

> Source/WebCore/layout/LayoutContext.cpp:68
> +    if (formattingContextRoot.establishesBlockFormattingContext())
> +        return std::make_unique<BlockFormattingContext>(formattingContextRoot);
> +    if (formattingContextRoot.establishesInlineFormattingContext())
> +        return std::make_unique<InlineFormattingContext>(formattingContextRoot);
> +    ASSERT_NOT_REACHED();

I would prefer some blank lines after the returns.

> Source/WebCore/layout/LayoutContext.h:53
> +    LayoutContext(Box& root);

Can this be a const&?

[zalan]

(In reply to Simon Fraser (smfr) from comment #2)
> Comment on attachment 338796 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338796&action=review
> 
> > Source/WebCore/layout/FormattingContext.h:50
> > +    const Box& root() const { return *m_root; }
> 
> You can't return a ref to a weak object. It might be null!
In normal conditions, this can't be null (formatting context should never outlive the box). In unexpected conditions, it is just a null deref (as opposed to UAF).
I could return Box*, but that would indicate that it's okay to have a null root and callers would start checking against it.

> 
> Can't we guarantee that the FormattingContext outlines the root and not use
> a WeakPtr?
I don't think we can guarantee that (we could release assert on the m_root at best). 

> 
> > Source/WebCore/layout/LayoutContext.cpp:68
> > +    if (formattingContextRoot.establishesBlockFormattingContext())
> > +        return std::make_unique<BlockFormattingContext>(formattingContextRoot);
> > +    if (formattingContextRoot.establishesInlineFormattingContext())
> > +        return std::make_unique<InlineFormattingContext>(formattingContextRoot);
> > +    ASSERT_NOT_REACHED();
> 
> I would prefer some blank lines after the returns.
ok.

> 
> > Source/WebCore/layout/LayoutContext.h:53
> > +    LayoutContext(Box& root);
> 
> Can this be a const&?
Not yet. WeakPtr can't take const objects.

[zalan]

Created attachment 338808 [details]
Patch

[zalan]

Committed r231028: <https://trac.webkit.org/changeset/231028>

[Radar WebKit Bug Importer]

<rdar://problem/39738535>