Bug 187412 – Introduce a layout milestone to track when the document contains a large number of rendered characters

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Wenson Hsieh]

<rdar://problem/41744338>

[Wenson Hsieh]

Created attachment 344462 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 344462 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344462&action=review

> Source/WebCore/ChangeLog:9
> +        Implements a new layout milestone: `DidReachSignificantRenderedTextThreshold`. This is similar to the existing

Why not DidReachSignificantAmoutOfTextRenderedThreshold or DidRenderedSignificantAmoutOfText?
I don't think "Threshold" adds much value here since everything is heuristics based.

> Source/WebCore/page/FrameView.cpp:4364
> +    if (!m_numberOfTextRenderers || m_visuallyNonEmptyCharacterCount / (float)m_numberOfTextRenderers < significantRenderedTextMeanLength)

Use static_cast<float>

> Source/WebCore/page/FrameView.h:934
> +    ++m_numberOfTextRenderers;

I think more canonical way of naming this variable would be m_renderTextCount.
But this is the total number of render text ever created since FrameView::reset() is last called
so that's not quite right either.
How about m_renderTextCountForVisuallyNonEmptyCharacters?

[Wenson Hsieh]

Comment on attachment 344462 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344462&action=review

>> Source/WebCore/ChangeLog:9
>> +        Implements a new layout milestone: `DidReachSignificantRenderedTextThreshold`. This is similar to the existing
> 
> Why not DidReachSignificantAmoutOfTextRenderedThreshold or DidRenderedSignificantAmoutOfText?
> I don't think "Threshold" adds much value here since everything is heuristics based.

Sounds good — I'll go with DidRenderSignificantAmountOfText

>> Source/WebCore/page/FrameView.cpp:4364
>> +    if (!m_numberOfTextRenderers || m_visuallyNonEmptyCharacterCount / (float)m_numberOfTextRenderers < significantRenderedTextMeanLength)
> 
> Use static_cast<float>

👍

>> Source/WebCore/page/FrameView.h:934
>> +    ++m_numberOfTextRenderers;
> 
> I think more canonical way of naming this variable would be m_renderTextCount.
> But this is the total number of render text ever created since FrameView::reset() is last called
> so that's not quite right either.
> How about m_renderTextCountForVisuallyNonEmptyCharacters?

Sounds good — I'll go with m_renderTextCountForVisuallyNonEmptyCharacters.

[zalan]

Comment on attachment 344462 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344462&action=review

> Source/WebCore/ChangeLog:16
> +        being capable of Reader mode. In many article-like pages, the average length of an uninterrupted text node is

page is capable of being capable of

> Source/WebCore/ChangeLog:17
> +        significantly longer than other types of pages; thus, on pages where the average length of a text node is very

What's an uninterrupted text node?

> Source/WebCore/ChangeLog:19
> +        Safari reader mode on watchOS.

Is this a watchOS only feature?

> Source/WebCore/ChangeLog:38
> +        layout, this milestone is never guaranteed to fire.

What if an iframe fires this milestone and not the main frame? The page is still considered Reader like?

> Source/WebCore/page/FrameView.cpp:4315
> +    return snappedIntRect(element.renderBox()->layoutOverflowRect()).height() >= documentHeightThreshold;

on trunk it is guaranteed that element.renderBox != nullptr since qualifiesAsVisuallyNonEmpty early returns, but now I can just call this function when element->renderer() is nullptr.

[Wenson Hsieh]

Comment on attachment 344462 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344462&action=review

>> Source/WebCore/ChangeLog:16
>> +        being capable of Reader mode. In many article-like pages, the average length of an uninterrupted text node is
> 
> page is capable of being capable of

Oops! Fixed.

>> Source/WebCore/ChangeLog:17
>> +        significantly longer than other types of pages; thus, on pages where the average length of a text node is very
> 
> What's an uninterrupted text node?

I really just meant "text node" here — the "uninterrupted" part should've just been omitted.

>> Source/WebCore/ChangeLog:19
>> +        Safari reader mode on watchOS.
> 
> Is this a watchOS only feature?

It'll fire on iOS and macOS as well if the SPI client opts into the milestone (the API test included in this patch even exercises this using the iOS simulator).
That being said, Safari on watchOS is the only client that might make use of this layout milestone at the moment.

>> Source/WebCore/ChangeLog:38
>> +        layout, this milestone is never guaranteed to fire.
> 
> What if an iframe fires this milestone and not the main frame? The page is still considered Reader like?

That's...an interesting point. At the moment, an iframe isn't capable of firing this milestone (since FrameView::fireLayoutRelatedMilestonesIfNeeded limits firing layout milestones to the main frame). I'm not certain what our eventual policy should be regarding layout milestones for subframes. FWIW, we disable cross-origin subframes altogether on watchOS, so I don't think this will matter much for the time being.

>> Source/WebCore/page/FrameView.cpp:4315
>> +    return snappedIntRect(element.renderBox()->layoutOverflowRect()).height() >= documentHeightThreshold;
> 
> on trunk it is guaranteed that element.renderBox != nullptr since qualifiesAsVisuallyNonEmpty early returns, but now I can just call this function when element->renderer() is nullptr.

Fair point. I'll make this robust when `!element.renderBox()`.

[Wenson Hsieh]

Created attachment 344507 [details]
v2

[EWS Watchlist]

Comment on attachment 344462 [details]
Patch

Attachment 344462 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8463300

New failing tests:
http/tests/security/canvas-remote-read-remote-video-redirect.html

[EWS Watchlist]

Created attachment 344509 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 344507 [details]
v2

Attachment 344507 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8466035

New failing tests:
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 344521 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[WebKit Commit Bot]

Comment on attachment 344507 [details]
v2

Clearing flags on attachment: 344507

Committed r233623: <https://trac.webkit.org/changeset/233623>