ssia
Created attachment 429468 [details] Patch
Comment on attachment 429468 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429468&action=review > Source/WebCore/layout/formattingContexts/table/TableFormattingQuirks.cpp:65 > + return height.isFixed() ? makeOptional(LayoutUnit { height.value() }) : WTF::nullopt; Instead of this: makeOptional(LayoutUnit { height.value() }) Does this compile? Optional<LayoutUnit> { height.value() } I think I would prefer that. > Source/WebCore/layout/formattingContexts/table/TableFormattingQuirks.cpp:70 > + for (auto* ancestor = &layoutBox.containingBlock(); ancestor && ancestor != &tableBox; ancestor = &ancestor->containingBlock()) { seems like we write this idiomatic loop so often; wish there was a cleaner idiom for it, like we have for DOM tree ancestry:
(In reply to Darin Adler from comment #2) > Comment on attachment 429468 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=429468&action=review > > > Source/WebCore/layout/formattingContexts/table/TableFormattingQuirks.cpp:65 > > + return height.isFixed() ? makeOptional(LayoutUnit { height.value() }) : WTF::nullopt; > > Instead of this: > > makeOptional(LayoutUnit { height.value() }) > > Does this compile? > > Optional<LayoutUnit> { height.value() } > > I think I would prefer that. This works: Optional<LayoutUnit> { LayoutUnit { height.value() } } > > > Source/WebCore/layout/formattingContexts/table/TableFormattingQuirks.cpp:70 > > + for (auto* ancestor = &layoutBox.containingBlock(); ancestor && ancestor != &tableBox; ancestor = &ancestor->containingBlock()) { > > seems like we write this idiomatic loop so often; wish there was a cleaner > idiom for it, like we have for DOM tree ancestry: We've got similar ancestor, descendant iterators in both rendering and layout codebase but you are right, we are missing the containing block iterator (which is a specialized ancestor iterator where we may skip ancestors as climbing up on the tree). I'll add on.
and thanks for the review.
Comment on attachment 429468 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429468&action=review >>> Source/WebCore/layout/formattingContexts/table/TableFormattingQuirks.cpp:65 >>> + return height.isFixed() ? makeOptional(LayoutUnit { height.value() }) : WTF::nullopt; >> >> Instead of this: >> >> makeOptional(LayoutUnit { height.value() }) >> >> Does this compile? >> >> Optional<LayoutUnit> { height.value() } >> >> I think I would prefer that. > > This works: Optional<LayoutUnit> { LayoutUnit { height.value() } } Well, that one is too wordy. I think what we need is to make a deduction guide so you can write: Optional { LayoutUnit { height.value() } } For now, makeOptional seems best. >>> Source/WebCore/layout/formattingContexts/table/TableFormattingQuirks.cpp:70 >>> + for (auto* ancestor = &layoutBox.containingBlock(); ancestor && ancestor != &tableBox; ancestor = &ancestor->containingBlock()) { >> >> seems like we write this idiomatic loop so often; wish there was a cleaner idiom for it, like we have for DOM tree ancestry: > > We've got similar ancestor, descendant iterators in both rendering and layout codebase but you are right, we are missing the containing block iterator (which is a specialized ancestor iterator where we may skip ancestors as climbing up on the tree). I'll add on. Sounds like it will work. There’s also the other part here: "stop when I hit what I think is my ancestor (tableBox here), but seriously if you hit null then just stop silently even though that should not happen".
Created attachment 429496 [details] Patch
Created attachment 429609 [details] Patch
Committed r278010 (238123@main): <https://commits.webkit.org/238123@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 429609 [details].
<rdar://problem/78457938>