[webkit-reviews] review granted: [Bug 64412] didFirstVisuallyNonEmptyLayout dispatched too early : [Attachment 100595] patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jul 12 17:35:17 PDT 2011
Darin Adler <darin at apple.com> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 64412: didFirstVisuallyNonEmptyLayout dispatched too early
https://bugs.webkit.org/show_bug.cgi?id=64412
Attachment 100595: patch
https://bugs.webkit.org/attachment.cgi?id=100595&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
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->image
Size(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?
More information about the webkit-reviews
mailing list