Bug 151806 – Simple line layout: Print out simple line layout statistics for the current page from command line.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[alan]

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

[alan]

Created attachment 266535 [details]
Patch

[Simon Fraser (smfr)]

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.

[alan]

Created attachment 266546 [details]
Patch

[alan]

Created attachment 266548 [details]
Patch

[Antti Koivisto]

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 = { };

[WebKit Commit Bot]

Comment on attachment 266548 [details]
Patch

Clearing flags on attachment: 266548

Committed r193374: <http://trac.webkit.org/changeset/193374>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[alan]

Created attachment 266733 [details]
Addressing post-review comments on r193374