nytimes.com: com.apple.WebKit.showSimpleLineLayoutCoverage-> --------------------------------------------------- Number of text blocks: total(440) non-simple(328) Text length: total(19089) non-simple(13777) overflow: visible: 1.54 nested renderers: 68.99 missing glyph: 0.11 unsupported TextFragment: 0.01 missing primary font: 1.84 simple line layout coverage: 27.83 --------------------------------------------------- com.apple.WebKit.showSimpleLineLayoutReasons -> --------------------------------------------------- "A recall offoods containing ce"(177): nested renderers "NYT Now "(7): overflow: visible "SEARCH "(6): overflow: visible "Mobile Applications "(24): nested renderers "Times Insider "(27): nested renderers "Well: Ask Well: A Cure for Ros"(61): nested renderers "By IAN LOVETT, RICHARD PÉREZ-P"(68): nested renderers "MAGAZINE "(8): nested renderers "In the Air: The Enduring Appea"(86): nested renderers "Living In: Clinton Hill, Brook"(89): nested renderers "Search for Homes for Sale or R"(33): nested renderers "OPINION "(7): nested renderers "One day after firing his polic"(158): nested renderers "NJ.com "(6): nested renderers "With Coal Industry Under Press"(52): nested renderers " "(1): unsupported TextFragment "Mass Shootings Stoke Ideologic"(68): nested renderers "The Boston Globe "(16): nested renderers "Brazil’s lower house agreed to"(166): nested renderers "ABC News "(8): nested renderers " "(9): overflow: visible, nested renderers ... ... ... ---------------------------------------------------
Created attachment 266535 [details] Patch
Comment on attachment 266535 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266535&action=review > Source/WebCore/rendering/SimpleLineLayout.cpp:63 > +enum DismissingReason: uint64_t { I don't really like "DismissingReason". NonSimpleReasons? > Source/WebCore/rendering/SimpleLineLayout.cpp:64 > + NoReason = 1LLU << 1, Shouldn't this be 1 << 0? > Source/WebCore/rendering/SimpleLineLayout.cpp:780 > +static void printReason(DismissingReasonFlags reason, TextStream& stream) Should pass a DismissingReason here. > Source/WebCore/rendering/SimpleLineLayout.cpp:930 > + for (DismissingReasonFlags reasonItem = EndOfReasons >> 1; reasonItem != NoReason; reasonItem >>= 1) { DismissingReason or auto > Source/WebCore/rendering/SimpleLineLayout.cpp:1036 > + HashMap<DismissingReasonFlags, unsigned> flowStatistics; DismissingReason > Source/WebCore/rendering/SimpleLineLayout.cpp:1065 > + fprintf(stderr, "%s", stream.release().utf8().data()); Use WTFLogAlways so this works on iOS.
Created attachment 266546 [details] Patch
Created attachment 266548 [details] Patch
Comment on attachment 266535 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266535&action=review >> Source/WebCore/rendering/SimpleLineLayout.cpp:63 >> +enum DismissingReason: uint64_t { > > I don't really like "DismissingReason". NonSimpleReasons? It is really SimpleLineLayout::DismissingReason which sort of makes sense. SimpleLineLayout::NotUsedReason perhaps? >> Source/WebCore/rendering/SimpleLineLayout.cpp:64 >> + NoReason = 1LLU << 1, > > Shouldn't this be 1 << 0? It can be 0 const and doesn't need to be in this enum. Something like CanUse would read better: return canUseForWithReason(flow, FallThrough::No) == CanUse; > Source/WebCore/rendering/SimpleLineLayout.cpp:118 > +enum class FallThrough { Yes, No }; Maybe something like enum class IncludeReasons { First, All } > Source/WebCore/rendering/SimpleLineLayout.cpp:121 > +#ifndef NDEBUG > +#define SET_REASON_AND_RETURN_IF_NEEDED(reasons, reason, fallthrough) { \ Why can't we get all reasons in release build? > Source/WebCore/rendering/SimpleLineLayout.cpp:149 > + SET_REASON_AND_RETURN_IF_NEEDED(reasons, FlowTextHasNoBreakSpace, fallthrough); Marco could just hardcode reasons and fallthrough variable names. But maybe this is more understandable? It is bit strange that the meaningful parameter is the middle one. > Source/WebCore/rendering/SimpleLineLayout.cpp:207 > + DismissingReasonFlags reasons = NoReason; I think it is weird to initialize flags to non-zero value. If NoReason case is zero then this can be just DismissingReasonFlags reasons = { };
Comment on attachment 266548 [details] Patch Clearing flags on attachment: 266548 Committed r193374: <http://trac.webkit.org/changeset/193374>
All reviewed patches have been landed. Closing bug.
Created attachment 266733 [details] Addressing post-review comments on r193374