Bug 206792 - [LFC][IFC] Layout logic should be driven by the type of the inline box
Summary: [LFC][IFC] Layout logic should be driven by the type of the inline box
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-01-24 18:58 PST by zalan
Modified: 2020-01-25 07:57 PST (History)
6 users (show)

See Also:


Attachments
Patch (7.77 KB, patch)
2020-01-24 19:02 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (8.30 KB, patch)
2020-01-25 06:36 PST, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2020-01-24 18:58:07 PST
ssia
Comment 1 Radar WebKit Bug Importer 2020-01-24 18:58:24 PST
<rdar://problem/58889080>
Comment 2 zalan 2020-01-24 19:02:25 PST
Created attachment 388753 [details]
Patch
Comment 3 Antti Koivisto 2020-01-24 23:59:20 PST
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?
Comment 4 zalan 2020-01-25 04:13:28 PST
(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.
Comment 5 zalan 2020-01-25 06:36:19 PST
Created attachment 388772 [details]
Patch
Comment 6 WebKit Commit Bot 2020-01-25 07:57:13 PST
Comment on attachment 388772 [details]
Patch

Clearing flags on attachment: 388772

Committed r255118: <https://trac.webkit.org/changeset/255118>
Comment 7 WebKit Commit Bot 2020-01-25 07:57:15 PST
All reviewed patches have been landed.  Closing bug.