64412 2011-07-12 17:14:27 -0700 didFirstVisuallyNonEmptyLayout dispatched too early 2011-07-13 03:58:55 -0700 1 1 1 Unclassified WebKit Page Loading 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 koivisto webkit-unassigned sam webkit.review.bot oldest_to_newest 436094 0 koivisto 2011-07-12 17:14:27 -0700 didFirstVisuallyNonEmptyLayout is often dispatched before the page has any real content. 436105 1 100595 koivisto 2011-07-12 17:27:34 -0700 Created attachment 100595 patch 436106 2 webkit.review.bot 2011-07-12 17:31:08 -0700 Attachment 100595 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. 436108 3 koivisto 2011-07-12 17:31:40 -0700 <rdar://problem/9407710> 436112 4 100595 darin 2011-07-12 17:35:18 -0700 Comment on attachment 100595 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? 436325 5 koivisto 2011-07-13 02:57:08 -0700 > 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. 436347 6 koivisto 2011-07-13 03:58:55 -0700 http://trac.webkit.org/changeset/90900 100595 2011-07-12 17:27:34 -0700 2011-07-12 17:35:18 -0700 patch first-visual-layout-2.patch text/plain 6397 koivisto SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDkwODcwKQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMjYgQEAKKzIwMTEtMDctMTIgIEFudHRpIEtv aXZpc3RvICA8YW50dGlAYXBwbGUuY29tPgorCisgICAgICAgIGRpZEZpcnN0VmlzdWFsbHlOb25F bXB0eUxheW91dCBkaXNwYXRjaGVkIHRvbyBlYXJseQorICAgICAgICBodHRwczovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NjQ0MTIKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICAtIFdhaXQgdW50aWwgYSB0aHJlc2hvbGQgb2YgY2hhcmFj dGVycyBhbmQgcGl4ZWxzIGhhcyBiZWVuIGV4Y2VlZGVkIGJlZm9yZSBkaXNwYXRjaGluZy4KKyAg ICAgICAgLSBXYWl0IHVudGlsIHN0eWxlc2hlZXRzIGFyZSBsb2FkZWQuCisKKyAgICAgICAgKiBw YWdlL0ZyYW1lVmlldy5jcHA6CisgICAgICAgIChXZWJDb3JlOjpGcmFtZVZpZXc6OnJlc2V0KToK KyAgICAgICAgKFdlYkNvcmU6OkZyYW1lVmlldzo6cGVyZm9ybVBvc3RMYXlvdXRUYXNrcyk6Cisg ICAgICAgICogcGFnZS9GcmFtZVZpZXcuaDoKKyAgICAgICAgKFdlYkNvcmU6OkZyYW1lVmlldzo6 aW5jcmVtZW50VmlzdWFsbHlOb25FbXB0eUNoYXJhY3RlckNvdW50KToKKyAgICAgICAgKFdlYkNv cmU6OkZyYW1lVmlldzo6aW5jcmVtZW50VmlzdWFsbHlOb25FbXB0eVBpeGVsQ291bnQpOgorICAg ICAgICAqIHJlbmRlcmluZy9SZW5kZXJJbWFnZS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpSZW5k ZXJJbWFnZTo6UmVuZGVySW1hZ2UpOgorICAgICAgICAoV2ViQ29yZTo6UmVuZGVySW1hZ2U6Omlt YWdlQ2hhbmdlZCk6CisgICAgICAgICogcmVuZGVyaW5nL1JlbmRlckltYWdlLmg6CisgICAgICAg ICogcmVuZGVyaW5nL1JlbmRlclRleHQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6UmVuZGVyVGV4 dDo6UmVuZGVyVGV4dCk6CisKIDIwMTEtMDctMTIgIEVtaWwgQSBFa2x1bmQgIDxlYWVAY2hyb21p dW0ub3JnPgogCiAgICAgICAgIFN3aXRjaCBJbmxpbmVGbG93Qm94IHRvIHRvIG5ldyBsYXlvdXQg dHlwZXMKSW5kZXg6IFNvdXJjZS9XZWJDb3JlL3BhZ2UvRnJhbWVWaWV3LmNwcAo9PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 Ci0tLSBTb3VyY2UvV2ViQ29yZS9wYWdlL0ZyYW1lVmlldy5jcHAJKHJldmlzaW9uIDkwODU1KQor KysgU291cmNlL1dlYkNvcmUvcGFnZS9GcmFtZVZpZXcuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0y MjcsNiArMjI3LDggQEAgdm9pZCBGcmFtZVZpZXc6OnJlc2V0KCkKICAgICBtX2xhc3RQYWludFRp bWUgPSAwOwogICAgIG1fcGFpbnRCZWhhdmlvciA9IFBhaW50QmVoYXZpb3JOb3JtYWw7CiAgICAg bV9pc1BhaW50aW5nID0gZmFsc2U7CisgICAgbV92aXN1YWxseU5vbkVtcHR5Q2hhcmFjdGVyQ291 bnQgPSAwOworICAgIG1fdmlzdWFsbHlOb25FbXB0eVBpeGVsQ291bnQgPSAwOwogICAgIG1faXNW aXN1YWxseU5vbkVtcHR5ID0gZmFsc2U7CiAgICAgbV9maXJzdFZpc3VhbGx5Tm9uRW1wdHlMYXlv dXRDYWxsYmFja1BlbmRpbmcgPSB0cnVlOwogICAgIG1fbWFpbnRhaW5TY3JvbGxQb3NpdGlvbkFu Y2hvciA9IDA7CkBAIC0yMDQ5LDggKzIwNTEsMTIgQEAgdm9pZCBGcmFtZVZpZXc6OnBlcmZvcm1Q b3N0TGF5b3V0VGFza3MoKQogICAgICAgICAgICAgbV9maXJzdExheW91dENhbGxiYWNrUGVuZGlu ZyA9IGZhbHNlOwogICAgICAgICAgICAgbV9mcmFtZS0+bG9hZGVyKCktPmRpZEZpcnN0TGF5b3V0 KCk7CiAgICAgICAgIH0KLSAgICAgICAgCi0gICAgICAgIGlmIChtX2lzVmlzdWFsbHlOb25FbXB0 eSAmJiBtX2ZpcnN0VmlzdWFsbHlOb25FbXB0eUxheW91dENhbGxiYWNrUGVuZGluZykgeworCisg ICAgICAgIC8vIEVuc3VyZSB0aGF0IHdlIGFsd2F5cyBzZW5kIHRoaXMgZXZlbnR1YWxseS4KKyAg ICAgICAgaWYgKCFtX2ZyYW1lLT5kb2N1bWVudCgpLT5wYXJzaW5nKCkpCisgICAgICAgICAgICBt X2lzVmlzdWFsbHlOb25FbXB0eSA9IHRydWU7CisKKyAgICAgICAgaWYgKG1faXNWaXN1YWxseU5v bkVtcHR5ICYmICFtX2ZyYW1lLT5kb2N1bWVudCgpLT5kaWRMYXlvdXRXaXRoUGVuZGluZ1N0eWxl c2hlZXRzKCkgJiYgbV9maXJzdFZpc3VhbGx5Tm9uRW1wdHlMYXlvdXRDYWxsYmFja1BlbmRpbmcp IHsKICAgICAgICAgICAgIG1fZmlyc3RWaXN1YWxseU5vbkVtcHR5TGF5b3V0Q2FsbGJhY2tQZW5k aW5nID0gZmFsc2U7CiAgICAgICAgICAgICBtX2ZyYW1lLT5sb2FkZXIoKS0+ZGlkRmlyc3RWaXN1 YWxseU5vbkVtcHR5TGF5b3V0KCk7CiAgICAgICAgIH0KSW5kZXg6IFNvdXJjZS9XZWJDb3JlL3Bh Z2UvRnJhbWVWaWV3LmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvcGFnZS9GcmFtZVZp ZXcuaAkocmV2aXNpb24gOTA4NTUpCisrKyBTb3VyY2UvV2ViQ29yZS9wYWdlL0ZyYW1lVmlldy5o CSh3b3JraW5nIGNvcHkpCkBAIC0yMjcsNiArMjI3LDggQEAgcHVibGljOgogICAgIHZvaWQgdXBk YXRlTGF5b3V0QW5kU3R5bGVJZk5lZWRlZFJlY3Vyc2l2ZSgpOwogICAgIHZvaWQgZmx1c2hEZWZl cnJlZFJlcGFpbnRzKCk7CiAKKyAgICB2b2lkIGluY3JlbWVudFZpc3VhbGx5Tm9uRW1wdHlDaGFy YWN0ZXJDb3VudCh1bnNpZ25lZCBjb3VudCk7CisgICAgdm9pZCBpbmNyZW1lbnRWaXN1YWxseU5v bkVtcHR5UGl4ZWxDb3VudChjb25zdCBJbnRTaXplJiBzaXplKTsKICAgICB2b2lkIHNldElzVmlz dWFsbHlOb25FbXB0eSgpIHsgbV9pc1Zpc3VhbGx5Tm9uRW1wdHkgPSB0cnVlOyB9CiAKICAgICB2 b2lkIGZvcmNlTGF5b3V0KGJvb2wgYWxsb3dTdWJ0cmVlID0gZmFsc2UpOwpAQCAtNDMzLDYgKzQz NSw4IEBAIHByaXZhdGU6CiAgICAgUGFpbnRCZWhhdmlvciBtX3BhaW50QmVoYXZpb3I7CiAgICAg Ym9vbCBtX2lzUGFpbnRpbmc7CiAKKyAgICB1bnNpZ25lZCBtX3Zpc3VhbGx5Tm9uRW1wdHlDaGFy YWN0ZXJDb3VudDsKKyAgICB1bnNpZ25lZCBtX3Zpc3VhbGx5Tm9uRW1wdHlQaXhlbENvdW50Owog ICAgIGJvb2wgbV9pc1Zpc3VhbGx5Tm9uRW1wdHk7CiAgICAgYm9vbCBtX2ZpcnN0VmlzdWFsbHlO b25FbXB0eUxheW91dENhbGxiYWNrUGVuZGluZzsKIApAQCAtNDQ5LDYgKzQ1MywyNiBAQCBwcml2 YXRlOgogICAgIHN0YXRpYyBkb3VibGUgc19kZWZlcnJlZFJlcGFpbnREZWxheUluY3JlbWVudER1 cmluZ0xvYWRpbmc7CiB9OwogCitpbmxpbmUgdm9pZCBGcmFtZVZpZXc6OmluY3JlbWVudFZpc3Vh bGx5Tm9uRW1wdHlDaGFyYWN0ZXJDb3VudCh1bnNpZ25lZCBjb3VudCkKK3sKKyAgICBpZiAobV9p c1Zpc3VhbGx5Tm9uRW1wdHkpCisgICAgICAgIHJldHVybjsKKyAgICBtX3Zpc3VhbGx5Tm9uRW1w dHlDaGFyYWN0ZXJDb3VudCArPSBjb3VudDsKKyAgICBzdGF0aWMgY29uc3QgdW5zaWduZWQgdmlz dWFsQ2hhcmFjdGVyVGhyZXNob2xkID0gMjAwOworICAgIGlmIChtX3Zpc3VhbGx5Tm9uRW1wdHlD aGFyYWN0ZXJDb3VudCA+IHZpc3VhbENoYXJhY3RlclRocmVzaG9sZCkKKyAgICAgICAgc2V0SXNW aXN1YWxseU5vbkVtcHR5KCk7Cit9CisKK2lubGluZSB2b2lkIEZyYW1lVmlldzo6aW5jcmVtZW50 VmlzdWFsbHlOb25FbXB0eVBpeGVsQ291bnQoY29uc3QgSW50U2l6ZSYgc2l6ZSkKK3sKKyAgICBp ZiAobV9pc1Zpc3VhbGx5Tm9uRW1wdHkpCisgICAgICAgIHJldHVybjsKKyAgICBtX3Zpc3VhbGx5 Tm9uRW1wdHlQaXhlbENvdW50ICs9IHNpemUud2lkdGgoKSAqIHNpemUuaGVpZ2h0KCk7CisgICAg c3RhdGljIGNvbnN0IHVuc2lnbmVkIHZpc3VhbFBpeGVsVGhyZXNob2xkID0gMjU2ICogMjU2Owor ICAgIGlmIChtX3Zpc3VhbGx5Tm9uRW1wdHlQaXhlbENvdW50ID4gdmlzdWFsUGl4ZWxUaHJlc2hv bGQpCisgICAgICAgIHNldElzVmlzdWFsbHlOb25FbXB0eSgpOworfQorCiB9IC8vIG5hbWVzcGFj ZSBXZWJDb3JlCiAKICNlbmRpZiAvLyBGcmFtZVZpZXdfaApJbmRleDogU291cmNlL1dlYkNvcmUv cmVuZGVyaW5nL1JlbmRlckltYWdlLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9y ZW5kZXJpbmcvUmVuZGVySW1hZ2UuY3BwCShyZXZpc2lvbiA5MDg1NSkKKysrIFNvdXJjZS9XZWJD b3JlL3JlbmRlcmluZy9SZW5kZXJJbWFnZS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTUxLDEwICs1 MSw5IEBAIHVzaW5nIG5hbWVzcGFjZSBIVE1MTmFtZXM7CiBSZW5kZXJJbWFnZTo6UmVuZGVySW1h Z2UoTm9kZSogbm9kZSkKICAgICA6IFJlbmRlclJlcGxhY2VkKG5vZGUsIEludFNpemUoMCwgMCkp CiAgICAgLCBtX25lZWRzVG9TZXRTaXplRm9yQWx0VGV4dChmYWxzZSkKKyAgICAsIG1fZGlkSW5j cmVtZW50VmlzdWFsbHlOb25FbXB0eVBpeGVsQ291bnQoZmFsc2UpCiB7CiAgICAgdXBkYXRlQWx0 VGV4dCgpOwotCi0gICAgdmlldygpLT5mcmFtZVZpZXcoKS0+c2V0SXNWaXN1YWxseU5vbkVtcHR5 KCk7CiB9CiAKIFJlbmRlckltYWdlOjp+UmVuZGVySW1hZ2UoKQpAQCAtMTQwLDYgKzEzOSwxMSBA QCB2b2lkIFJlbmRlckltYWdlOjppbWFnZUNoYW5nZWQoV3JhcHBlZEltCiAKICAgICBpZiAobmV3 SW1hZ2UgIT0gbV9pbWFnZVJlc291cmNlLT5pbWFnZVB0cigpIHx8ICFuZXdJbWFnZSkKICAgICAg ICAgcmV0dXJuOworICAgIAorICAgIGlmICghbV9kaWRJbmNyZW1lbnRWaXN1YWxseU5vbkVtcHR5 UGl4ZWxDb3VudCkgeworICAgICAgICB2aWV3KCktPmZyYW1lVmlldygpLT5pbmNyZW1lbnRWaXN1 YWxseU5vbkVtcHR5UGl4ZWxDb3VudChtX2ltYWdlUmVzb3VyY2UtPmltYWdlU2l6ZSgxLjBmKSk7 CisgICAgICAgIG1fZGlkSW5jcmVtZW50VmlzdWFsbHlOb25FbXB0eVBpeGVsQ291bnQgPSB0cnVl OworICAgIH0KIAogICAgIGJvb2wgaW1hZ2VTaXplQ2hhbmdlZCA9IGZhbHNlOwogCkluZGV4OiBT b3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVySW1hZ2UuaAo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3Vy Y2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVySW1hZ2UuaAkocmV2aXNpb24gOTA4NTUpCisrKyBT b3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVySW1hZ2UuaAkod29ya2luZyBjb3B5KQpAQCAt OTgsNiArOTgsNyBAQCBwcml2YXRlOgogICAgIFN0cmluZyBtX2FsdFRleHQ7CiAgICAgT3duUHRy PFJlbmRlckltYWdlUmVzb3VyY2U+IG1faW1hZ2VSZXNvdXJjZTsKICAgICBib29sIG1fbmVlZHNU b1NldFNpemVGb3JBbHRUZXh0OworICAgIGJvb2wgbV9kaWRJbmNyZW1lbnRWaXN1YWxseU5vbkVt cHR5UGl4ZWxDb3VudDsKIAogICAgIGZyaWVuZCBjbGFzcyBSZW5kZXJJbWFnZVNjYWxlT2JzZXJ2 ZXI7CiB9OwpJbmRleDogU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlclRleHQuY3BwCj09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJUZXh0LmNwcAkocmV2 aXNpb24gOTA4NTUpCisrKyBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyVGV4dC5jcHAJ KHdvcmtpbmcgY29weSkKQEAgLTExMSwxMCArMTExLDcgQEAgUmVuZGVyVGV4dDo6UmVuZGVyVGV4 dChOb2RlKiBub2RlLCBQYXNzUgogCiAgICAgc2V0SXNUZXh0KCk7CiAKLSAgICAvLyBGSVhNRTog SXQgd291bGQgYmUgYmV0dGVyIHRvIGNhbGwgdGhpcyBvbmx5IGlmICFtX3RleHQtPmNvbnRhaW5z T25seVdoaXRlc3BhY2UoKS4KLSAgICAvLyBCdXQgdGhhdCBtaWdodCBzbG93IHRoaW5ncyBkb3du LCBhbmQgbWF5YmUgc2hvdWxkIG9ubHkgYmUgZG9uZSBpZiB2aXN1YWxseU5vbkVtcHR5Ci0gICAg Ly8gaXMgc3RpbGwgZmFsc2UuIE5vdCBtYWtpbmcgYW55IGNoYW5nZSBmb3Igbm93LCBidXQgc2hv dWxkIGNvbnNpZGVyIGluIHRoZSBmdXR1cmUuCi0gICAgdmlldygpLT5mcmFtZVZpZXcoKS0+c2V0 SXNWaXN1YWxseU5vbkVtcHR5KCk7CisgICAgdmlldygpLT5mcmFtZVZpZXcoKS0+aW5jcmVtZW50 VmlzdWFsbHlOb25FbXB0eUNoYXJhY3RlckNvdW50KG1fdGV4dC5sZW5ndGgoKSk7CiB9CiAKICNp Zm5kZWYgTkRFQlVHCg==