Bug 206792

<rdar://problem/58889080>

Created attachment 388753 [details]
Patch

Comment on attachment 388753 [details]
Patch

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

> Source/WebCore/layout/inlineformatting/InlineFormattingContext.cpp:68
>  static inline const Box* nextInPreOrder(const Box& layoutBox, const Container& stayWithin)
>  {
> -    const Box* nextInPreOrder = nullptr;
> -    if (!layoutBox.establishesFormattingContext() && is<Container>(layoutBox) && downcast<Container>(layoutBox).hasInFlowOrFloatingChild())
> -        return downcast<Container>(layoutBox).firstInFlowOrFloatingChild();
> -
> -    for (nextInPreOrder = &layoutBox; nextInPreOrder && nextInPreOrder != &stayWithin; nextInPreOrder = nextInPreOrder->parent()) {
> -        if (auto* nextSibling = nextInPreOrder->nextInFlowOrFloatingSibling())
> -            return nextSibling;
> +    auto opaqueBox = layoutBox.isFloatingPositioned() || layoutBox.isAtomicInlineLevelBox();
> +    if (opaqueBox || layoutBox.isAnonymous() || layoutBox.isLineBreakBox()) {
> +        for (auto* nextInPreOrder = &layoutBox; nextInPreOrder && nextInPreOrder != &stayWithin; nextInPreOrder = nextInPreOrder->parent()) {
> +            if (auto* nextSibling = nextInPreOrder->nextInFlowOrFloatingSibling())
> +                return nextSibling;
> +        }
> +        return nullptr;
>      }
> -    return nullptr;
> +    ASSERT(layoutBox.isInlineBox());
> +    return downcast<Container>(layoutBox).firstInFlowOrFloatingChild();
>  }

I feel this is making previously robust and understandable pre-order traversal code much less so. How de we know at the end this is a Container that is guaranteed to have at least one child?

(In reply to Antti Koivisto from comment #3)
> Comment on attachment 388753 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388753&action=review
> 
> > Source/WebCore/layout/inlineformatting/InlineFormattingContext.cpp:68
> >  static inline const Box* nextInPreOrder(const Box& layoutBox, const Container& stayWithin)
> >  {
> > -    const Box* nextInPreOrder = nullptr;
> > -    if (!layoutBox.establishesFormattingContext() && is<Container>(layoutBox) && downcast<Container>(layoutBox).hasInFlowOrFloatingChild())
> > -        return downcast<Container>(layoutBox).firstInFlowOrFloatingChild();
> > -
> > -    for (nextInPreOrder = &layoutBox; nextInPreOrder && nextInPreOrder != &stayWithin; nextInPreOrder = nextInPreOrder->parent()) {
> > -        if (auto* nextSibling = nextInPreOrder->nextInFlowOrFloatingSibling())
> > -            return nextSibling;
> > +    auto opaqueBox = layoutBox.isFloatingPositioned() || layoutBox.isAtomicInlineLevelBox();
> > +    if (opaqueBox || layoutBox.isAnonymous() || layoutBox.isLineBreakBox()) {
> > +        for (auto* nextInPreOrder = &layoutBox; nextInPreOrder && nextInPreOrder != &stayWithin; nextInPreOrder = nextInPreOrder->parent()) {
> > +            if (auto* nextSibling = nextInPreOrder->nextInFlowOrFloatingSibling())
> > +                return nextSibling;
> > +        }
> > +        return nullptr;
> >      }
> > -    return nullptr;
> > +    ASSERT(layoutBox.isInlineBox());
> > +    return downcast<Container>(layoutBox).firstInFlowOrFloatingChild();
> >  }
> 
> I feel this is making previously robust and understandable pre-order
> traversal code much less so. How de we know at the end this is a Container
> that is guaranteed to have at least one child?
I totally agree. I probably should be doing it the other way around and remove the Container class first since at this point that class is more of a nuisance than an actual help.

Created attachment 388772 [details]
Patch

Comment on attachment 388772 [details]
Patch

Clearing flags on attachment: 388772

Committed r255118: <https://trac.webkit.org/changeset/255118>

All reviewed patches have been landed.  Closing bug.