[Webkit-unassigned] [Bug 118272] Take document height into account when determining when it is considered visually non-empy

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jul 2 12:18:47 PDT 2013


https://bugs.webkit.org/show_bug.cgi?id=118272


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #205852|review?                     |review+
               Flag|                            |




--- Comment #2 from Darin Adler <darin at apple.com>  2013-07-02 12:20:46 PST ---
(From update of attachment 205852)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.


More information about the webkit-unassigned mailing list