Take document height into account when determining when it is considered visually non-empy

[Antti Koivisto]

The current visually non-empy mechanism takes into account only the amount of contents in renderers. Add a simple layout dependency so that we don't consider page non-empty until the document height exceed a (low) height threshold (or the load completes).

[Antti Koivisto]

Created attachment 205852 [details]
patch

[Darin Adler]

Comment on attachment 205852 [details]
patch

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

> Source/WebCore/ChangeLog:8
> +        The current visually non-empy mechanism takes into account only the amount of contents in renderers. 

Typo: "empy".

> Source/WebCore/page/FrameView.cpp:3721
> +bool FrameView::determingIsVisuallyNonEmpty() const

Typo here: “determing”. Confusing function name, “determine is visually non empty”. Maybe “isNowVisuallyNonEmpty” or “isVisuallyNonEmptyUncached”. Neither of those is good, but I don’t like “determine is” better. Maybe “qualifiesAsVisuallyNonEmpty” is a good one.

> Source/WebCore/page/FrameView.cpp:3742
> +    // No content yet.
> +    Element* documentElement = m_frame->document()->documentElement();
> +    if (!documentElement || !documentElement->renderer())
> +        return false;
> +    // Ensure that we always get marked visually non-empty eventually.
> +    if (!m_frame->document()->parsing() && m_frame->loader()->stateMachine()->committedFirstRealDocumentLoad())
> +        return true;
> +    // Require the document to grow a bit.
> +    static const int documentHeightThreshold = 200;
> +    if (documentElement->renderBox()->layoutOverflowRect().pixelSnappedHeight() < documentHeightThreshold)
> +        return false;
> +    // The first few hundred characters rarely contain the interesting content of the page.
> +    static const unsigned visualCharacterThreshold = 200;
> +    if (m_visuallyNonEmptyCharacterCount > visualCharacterThreshold)
> +        return true;
> +    // Use a threshold value to prevent very small amounts of visible content from triggering didFirstVisuallyNonEmptyLayout
> +    static const unsigned visualPixelThreshold = 32 * 32;
> +    if (m_visuallyNonEmptyPixelCount > visualPixelThreshold)
> +        return true;
> +    return false;

I think the way this alternates false with true early returns makes it tricky to read the logic. Maybe an extra blank line to paragraph the sections could make it clearer?

> Source/WebCore/rendering/RenderHTMLCanvas.cpp:50
>  {
> -    view()->frameView()->setIsVisuallyNonEmpty();
> +    // Actual size is not known yet, report the default intrinsic size.
> +    view()->frameView()->incrementVisuallyNonEmptyPixelCount(roundedIntSize(intrinsicSize()));
> +
>  }

Excess blank line being added here.

[Antti Koivisto]

http://trac.webkit.org/changeset/152401