[webkit-reviews] review granted: [Bug 118272] Take document height into account when determining when it is considered visually non-empy : [Attachment 205852] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 118272: Take document height into account when determining when it is
considered visually non-empy
https://bugs.webkit.org/show_bug.cgi?id=118272

Attachment 205852: patch
https://bugs.webkit.org/attachment.cgi?id=205852&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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(intrins
icSize()));
> +
>  }

Excess blank line being added here.