[webkit-reviews] review granted: [Bug 187412] Introduce a layout milestone to track when the document contains a large number of rendered characters : [Attachment 344462] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jul 6 19:21:05 PDT 2018


Ryosuke Niwa <rniwa at webkit.org> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 187412: Introduce a layout milestone to track when the document contains a
large number of rendered characters
https://bugs.webkit.org/show_bug.cgi?id=187412

Attachment 344462: Patch

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




--- Comment #2 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 344462
  --> https://bugs.webkit.org/attachment.cgi?id=344462
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?


More information about the webkit-reviews mailing list