Summary: | didFirstVisuallyNonEmptyLayout dispatched too early | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||
Component: | Page Loading | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | sam, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Antti Koivisto
2011-07-12 17:14:27 PDT
Created attachment 100595 [details]
patch
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 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? > 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. |