Bug 64412 - didFirstVisuallyNonEmptyLayout dispatched too early
Summary: didFirstVisuallyNonEmptyLayout dispatched too early
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-12 17:14 PDT by Antti Koivisto
Modified: 2011-07-13 03:58 PDT (History)
2 users (show)

See Also:


Attachments
patch (6.25 KB, patch)
2011-07-12 17:27 PDT, Antti Koivisto
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2011-07-12 17:14:27 PDT
didFirstVisuallyNonEmptyLayout is often dispatched before the page has any real content.
Comment 1 Antti Koivisto 2011-07-12 17:27:34 PDT
Created attachment 100595 [details]
patch
Comment 2 WebKit Review Bot 2011-07-12 17:31:08 PDT
Attachment 100595 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/page/FrameView.h:231:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Antti Koivisto 2011-07-12 17:31:40 PDT
<rdar://problem/9407710>
Comment 4 Darin Adler 2011-07-12 17:35:18 PDT
Comment on attachment 100595 [details]
patch

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

> Source/WebCore/page/FrameView.cpp:2059
> -        if (m_isVisuallyNonEmpty && m_firstVisuallyNonEmptyLayoutCallbackPending) {
> +
> +        // Ensure that we always send this eventually.
> +        if (!m_frame->document()->parsing())
> +            m_isVisuallyNonEmpty = true;
> +
> +        if (m_isVisuallyNonEmpty && !m_frame->document()->didLayoutWithPendingStylesheets() && m_firstVisuallyNonEmptyLayoutCallbackPending) {

There is nothing in the change log or code that explains why you added a new check of didLayoutWithPendingStylesheets here.

> Source/WebCore/page/FrameView.h:231
> +    void incrementVisuallyNonEmptyPixelCount(const IntSize& size);

The name “size” here doesn’t add anything, so you should omit it.

> Source/WebCore/page/FrameView.h:461
> +    static const unsigned visualCharacterThreshold = 200;

Would like a why comment explaining where this threshold came from. There isn’t even a comment explaining why we made visually non-empty a heuristic rather than a more brittle but well defined mechanism. There should be.

> Source/WebCore/page/FrameView.h:470
> +    m_visuallyNonEmptyPixelCount += size.width() * size.height();

Any risk of overflow here?

> Source/WebCore/page/FrameView.h:471
> +    static const unsigned visualPixelThreshold = 256 * 256;

Same kind of comment about threshold needed as above.

> Source/WebCore/rendering/RenderImage.cpp:146
> +    if (!m_didIncrementVisuallyNonEmptyPixelCount) {
> +        view()->frameView()->incrementVisuallyNonEmptyPixelCount(m_imageResource->imageSize(1.0f));
> +        m_didIncrementVisuallyNonEmptyPixelCount = true;
> +    }

Does this do the right thing for incrementally loading images? Seems like it will only do the pixel count thing once, perhaps with a smaller size than the actual final size?

> Source/WebCore/rendering/RenderText.cpp:114
> -    // FIXME: It would be better to call this only if !m_text->containsOnlyWhitespace().
> -    // But that might slow things down, and maybe should only be done if visuallyNonEmpty
> -    // is still false. Not making any change for now, but should consider in the future.
> -    view()->frameView()->setIsVisuallyNonEmpty();
> +    view()->frameView()->incrementVisuallyNonEmptyCharacterCount(m_text.length());

Not worried about cases where there is nothing but a lot of whitespace?
Comment 5 Antti Koivisto 2011-07-13 02:57:08 PDT
> There is nothing in the change log or code that explains why you added a new check of didLayoutWithPendingStylesheets here.

- Wait until stylesheets are loaded.  <--- this one, but i'll expand a bit

> Would like a why comment explaining where this threshold came from. There isn’t even a comment explaining why we made visually non-empty a heuristic rather than a more brittle but well defined mechanism. There should be.

Ok.

> > Source/WebCore/page/FrameView.h:470
> > +    m_visuallyNonEmptyPixelCount += size.width() * size.height();
> 
> Any risk of overflow here?

Yes, but it is unlikely to happen and even more unlikely to cause problems.

> Does this do the right thing for incrementally loading images? Seems like it will only do the pixel count thing once, perhaps with a smaller size than the actual final size?

It uses the flnal size of the image even when loading incrementally. I don't think anything fancier is worth the effort.

> > +    view()->frameView()->incrementVisuallyNonEmptyCharacterCount(m_text.length());
> 
> Not worried about cases where there is nothing but a lot of whitespace?

I don't want to take the (small) performance hit unless there is a proof that this is a real world issue.
Comment 6 Antti Koivisto 2011-07-13 03:58:55 PDT
http://trac.webkit.org/changeset/90900