| Summary: | ASSERTION FAILED: *trailingRunIndex >= overflowingRunIndex in WebCore::Layout::InlineContentBreaker::tryBreakingNextOverflowingRuns | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Fujii Hironori <Hironori.Fujii> | ||||||||||||
| Component: | Layout and Rendering | Assignee: | zalan <zalan> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | bfulgham, darin, koivisto, simon.fraser, webkit-bug-importer, zalan | ||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Bug Depends on: | 239879, 239886 | ||||||||||||||
| Bug Blocks: | |||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Fujii Hironori
2022-04-27 21:10:12 PDT
trailingRunIndex was 0, and overflowingRunIndex was 1. Created attachment 458491 [details]
a bit simplified content
I can repro the assertion with MiniBrowser loading the simplified content. Created attachment 458532 [details]
Test reduction
Created attachment 458586 [details]
Patch
Created attachment 458632 [details]
Patch
Created attachment 458633 [details]
[fast-cq]Patch
Committed r293646 (250150@main): <https://commits.webkit.org/250150@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 458633 [details]. Comment on attachment 458633 [details] [fast-cq]Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458633&action=review Some code style questions > Source/WebCore/layout/formattingContexts/inline/InlineLineBuilder.cpp:410 > + auto overflow = std::optional<PartialContent> { }; Another way to write this is: std::optional<PartialContent> overflow; Antti, do you really prefer the auto style for this specific case? > Source/WebCore/layout/formattingContexts/inline/InlineLineBuilder.h:63 > std::optional<PartialContent> partialOverflowingContent { }; Don’t think we need "{ }" here. > Source/WebCore/layout/formattingContexts/inline/InlineLineBuilder.h:65 > + std::optional<InlineLayoutUnit> trailingOverflowingContentWidth { }; Was this change really needed? I’m pretty sure that std::optional objects are initialized to std::nullopt without requiring any { }. > Source/WebCore/layout/formattingContexts/inline/InlineLineBuilder.h:91 > + std::optional<PartialContent> partialOverflowingContent { }; Don’t think we need "{ }" here. > > Source/WebCore/layout/formattingContexts/inline/InlineLineBuilder.h:63 > > std::optional<PartialContent> partialOverflowingContent { }; > > Don’t think we need "{ }" here. > > > Source/WebCore/layout/formattingContexts/inline/InlineLineBuilder.h:65 > > + std::optional<InlineLayoutUnit> trailingOverflowingContentWidth { }; > > Was this change really needed? I’m pretty sure that std::optional objects > are initialized to std::nullopt without requiring any { }. > > > Source/WebCore/layout/formattingContexts/inline/InlineLineBuilder.h:91 > > + std::optional<PartialContent> partialOverflowingContent { }; > > Don’t think we need "{ }" here. EWS complained about it (see the first version of the patch) ./layout/formattingContexts/inline/InlineFormattingContext.cpp:438:139: error: missing field 'trailingOverflowingContentWidth' initializer [-Werror,-Wmissing-field-initializers] > > Source/WebCore/layout/formattingContexts/inline/InlineLineBuilder.cpp:410 > > + auto overflow = std::optional<PartialContent> { }; > > Another way to write this is: > > std::optional<PartialContent> overflow; > > Antti, do you really prefer the auto style for this specific case? Not sure about Antti, but the reason why I write it this way is simply for esthetic reasons e.g. auto overflow = std::optional<PartialContent> { }; auto logicalRect = LayoutRect { x, y, width, height }; vs. std::optional<PartialContent> overflow; auto logicalRect = LayoutRect { x, y, width, height }; Don't know why my opinion is important here but I agree with Alan that consistency is often preferable. (In reply to zalan from comment #11) > EWS complained about it (see the first version of the patch) > ./layout/formattingContexts/inline/InlineFormattingContext.cpp:438:139: > error: missing field 'trailingOverflowingContentWidth' initializer > [-Werror,-Wmissing-field-initializers] Got it: if the compiler says it’s needed, I guess I am wrong. > Not sure about Antti, but the reason why I write it this way is simply for > esthetic reasons > > e.g. > auto overflow = std::optional<PartialContent> { }; > auto logicalRect = LayoutRect { x, y, width, height }; That sounds fine. |