ssia
<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.