102970 2012-11-21 12:33:50 -0800 [WK1] REGRESSION (r129545): Main frame doesn't rubberband unless WebFrameLoadDelegate implements -webView:didFirstLayoutInFrame: 2012-11-29 11:36:54 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Mac (Intel) OS X 10.8 RESOLVED FIXED Regression P1 Normal --- 1 aroben bdakin bdakin sam oldest_to_newest 773183 0 aroben 2012-11-21 12:33:50 -0800 Starting in r129545, the main frame no longer rubberbands in WebKit1 apps. It's easy to see this with any long content. 773192 1 aroben 2012-11-21 12:45:47 -0800 I think I see the bug. In -[WebView _cacheFrameLoadDelegateImplementations] we have this code: http://trac.webkit.org/browser/trunk/Source/WebKit/mac/WebView/WebView.mm?rev=129545#L1653 // It would be nice to get rid of this code and transition all clients to using didLayout instead of // didFirstLayoutInFrame and didFirstVisuallyNonEmptyLayoutInFrame. In the meantime, this is required // for backwards compatibility. Page* page = core(self); if (page) { unsigned milestones = 0; if (cache->didFirstLayoutInFrameFunc) milestones |= DidFirstLayout; if (cache->didFirstVisuallyNonEmptyLayoutInFrameFunc) milestones |= DidFirstVisuallyNonEmptyLayout; page->addLayoutMilestones(static_cast<LayoutMilestones>(milestones)); } This means that we'll only listen for the DidFirstLayout milestone if the WebFrameLoadDelegate responds to -webView:didFirstLayoutInFrame:. In WebFrameLoaderClient::dispatchDidLayout, we have this code: http://trac.webkit.org/browser/trunk/Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm?rev=129545#L673 if (milestones & DidFirstLayout) { // FIXME: We should consider removing the old didFirstLayout API since this is doing double duty with the // new didLayout API. if (implementations->didFirstLayoutInFrameFunc) CallFrameLoadDelegate(implementations->didFirstLayoutInFrameFunc, webView, @selector(webView:didFirstLayoutInFrame:), m_webFrame.get()); // See WebFrameLoaderClient::provisionalLoadStarted. WebDynamicScrollBarsView *scrollView = [m_webFrame->_private->webFrameView _scrollView]; if ([getWebView(m_webFrame.get()) drawsBackground]) [scrollView setDrawsBackground:YES]; #if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1070 [scrollView setVerticalScrollElasticity:NSScrollElasticityAutomatic]; [scrollView setHorizontalScrollElasticity:NSScrollElasticityAutomatic]; #endif } So we'll only set the scroll view's elasticity if we get a DidFirstLayout milestone, but we'll only get that if the WebFrameLoadDelegate responds to -webView:didFirstLayoutInFrame:. (Note that not even implementing the new -webView:didLayout: method will get elasticity to work.) 773203 2 aroben 2012-11-21 13:08:21 -0800 A simple fix would be to always ask for DidFirstLayout in WK1. 776109 3 bdakin 2012-11-26 17:18:20 -0800 (In reply to comment #2) > A simple fix would be to always ask for DidFirstLayout in WK1. I think this fix makes sense. I did something similar for other ports that were doing necessary work when those events fire. I will post a patch in a minute! 776134 4 176127 bdakin 2012-11-26 17:45:07 -0800 Created attachment 176127 Patch 776203 5 176127 aroben 2012-11-26 18:52:40 -0800 Comment on attachment 176127 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176127&action=review > Source/WebKit/mac/ChangeLog:9 > + We should always register for DidFirstLayout in WK1 since we do stork stork! 777216 6 176337 bdakin 2012-11-27 14:15:21 -0800 Created attachment 176337 Stork-free patch 777218 7 176337 aroben 2012-11-27 14:16:58 -0800 Comment on attachment 176337 Stork-free patch This looks great to me. Probably worth having someone else take a look since I suggested the approach though. Thanks for jumping on it! 778042 8 176337 sam 2012-11-28 10:36:41 -0800 Comment on attachment 176337 Stork-free patch Looks good to me too. r=me. 778119 9 bdakin 2012-11-28 11:38:26 -0800 Excellent! Thanks, guys. http://trac.webkit.org/changeset/136033 778123 10 aroben 2012-11-28 11:39:32 -0800 Yay! 779247 11 aroben 2012-11-29 11:36:54 -0800 I can confirm this is fixed in the r136085 nightly build. 176127 2012-11-26 17:45:07 -0800 2012-11-27 14:15:21 -0800 Patch for-review.txt text/plain 1635 bdakin SW5kZXg6IFNvdXJjZS9XZWJLaXQvbWFjL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2Uv V2ViS2l0L21hYy9DaGFuZ2VMb2cJKHJldmlzaW9uIDEzNTc5MSkKKysrIFNvdXJjZS9XZWJLaXQv bWFjL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDEyLTExLTI2 ICBCZXRoIERha2luICA8YmRha2luQGFwcGxlLmNvbT4KKworICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAyOTcwCisgICAgICAgIFtXSzFdIFJFR1JFU1NJ T04gKHIxMjk1NDUpOiBNYWluIGZyYW1lIGRvZXNuJ3QgcnViYmVyYmFuZCB1bmxlc3MgCisgICAg ICAgIFdlYkZyYW1lTG9hZERlbGVnYXRlIGltcGxlbWVudHMgLXdlYlZpZXc6ZGlkRmlyc3RMYXlv dXRJbkZyYW1lOgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIFdlIHNob3VsZCBhbHdheXMgcmVnaXN0ZXIgZm9yIERpZEZpcnN0TGF5b3V0IGluIFdLMSBz aW5jZSB3ZSBkbyBzdG9yayAKKyAgICAgICAgYXQgdGhhdCB0aW1lIGJlc2lkZXMganVzdCBjYWxs aW5nIHRoZSBkZWxlZ2F0ZSBmdW5jdGlvbi4KKyAgICAgICAgKiBXZWJWaWV3L1dlYlZpZXcubW06 CisgICAgICAgICgtW1dlYlZpZXcgX2NhY2hlRnJhbWVMb2FkRGVsZWdhdGVJbXBsZW1lbnRhdGlv bnNdKToKKwogMjAxMi0xMS0yNiAgSmFtZXMgU2ltb25zZW4gIDxzaW1vbmphbUBjaHJvbWl1bS5v cmc+CiAKICAgICAgICAgQ29uc29saWRhdGUgRnJhbWVMb2FkZXI6OmxvYWQoKSBpbnRvIG9uZSBm dW5jdGlvbiB0YWtpbmcgYSBGcmFtZUxvYWRSZXF1ZXN0CkluZGV4OiBTb3VyY2UvV2ViS2l0L21h Yy9XZWJWaWV3L1dlYlZpZXcubW0KPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYktpdC9tYWMvV2Vi Vmlldy9XZWJWaWV3Lm1tCShyZXZpc2lvbiAxMzU3NzQpCisrKyBTb3VyY2UvV2ViS2l0L21hYy9X ZWJWaWV3L1dlYlZpZXcubW0JKHdvcmtpbmcgY29weSkKQEAgLTE2ODYsOSArMTY4Niw3IEBAIC0g KHZvaWQpX2NhY2hlRnJhbWVMb2FkRGVsZWdhdGVJbXBsZW1lbnQKICAgICAvLyBmb3IgYmFja3dh cmRzIGNvbXBhdGliaWxpdHkuCiAgICAgUGFnZSogcGFnZSA9IGNvcmUoc2VsZik7CiAgICAgaWYg KHBhZ2UpIHsKLSAgICAgICAgdW5zaWduZWQgbWlsZXN0b25lcyA9IDA7Ci0gICAgICAgIGlmIChj YWNoZS0+ZGlkRmlyc3RMYXlvdXRJbkZyYW1lRnVuYykKLSAgICAgICAgICAgIG1pbGVzdG9uZXMg fD0gRGlkRmlyc3RMYXlvdXQ7CisgICAgICAgIHVuc2lnbmVkIG1pbGVzdG9uZXMgPSBEaWRGaXJz dExheW91dDsKICAgICAgICAgaWYgKGNhY2hlLT5kaWRGaXJzdFZpc3VhbGx5Tm9uRW1wdHlMYXlv dXRJbkZyYW1lRnVuYykKICAgICAgICAgICAgIG1pbGVzdG9uZXMgfD0gRGlkRmlyc3RWaXN1YWxs eU5vbkVtcHR5TGF5b3V0OwogICAgICAgICBwYWdlLT5hZGRMYXlvdXRNaWxlc3RvbmVzKHN0YXRp Y19jYXN0PExheW91dE1pbGVzdG9uZXM+KG1pbGVzdG9uZXMpKTsK 176337 2012-11-27 14:15:21 -0800 2012-11-28 10:36:40 -0800 Stork-free patch for-review-2.txt text/plain 1611 bdakin SW5kZXg6IFNvdXJjZS9XZWJLaXQvbWFjL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2Uv V2ViS2l0L21hYy9DaGFuZ2VMb2cJKHJldmlzaW9uIDEzNTkxOSkKKysrIFNvdXJjZS9XZWJLaXQv bWFjL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDEyLTExLTI3 ICBCZXRoIERha2luICA8YmRha2luQGFwcGxlLmNvbT4KKworICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAyOTcwCisgICAgICAgIFtXSzFdIFJFR1JFU1NJ T04gKHIxMjk1NDUpOiBNYWluIGZyYW1lIGRvZXNuJ3QgcnViYmVyYmFuZCB1bmxlc3MgCisgICAg ICAgIFdlYkZyYW1lTG9hZERlbGVnYXRlIGltcGxlbWVudHMgLXdlYlZpZXc6ZGlkRmlyc3RMYXlv dXRJbkZyYW1lOgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIFdlIHNob3VsZCBhbHdheXMgcmVnaXN0ZXIgZm9yIERpZEZpcnN0TGF5b3V0IGluIFdLMSBz aW5jZSB3ZSBkbyB3b3JrIAorICAgICAgICBhdCB0aGF0IHRpbWUgYmVzaWRlcyBqdXN0IGNhbGxp bmcgdGhlIGRlbGVnYXRlIGZ1bmN0aW9uLgorICAgICAgICAqIFdlYlZpZXcvV2ViVmlldy5tbToK KyAgICAgICAgKC1bV2ViVmlldyBfY2FjaGVGcmFtZUxvYWREZWxlZ2F0ZUltcGxlbWVudGF0aW9u c10pOgorCiAyMDEyLTExLTI3ICBQcmF0aWsgU29sYW5raSAgPHBzb2xhbmtpQGFwcGxlLmNvbT4K IAogICAgICAgICBvYmpjL29iamMtcnVudGltZS5oIGRvZXMgbm90IGV4aXN0IG9uIGFsbCBQTEFU Rk9STShNQUMpCkluZGV4OiBTb3VyY2UvV2ViS2l0L21hYy9XZWJWaWV3L1dlYlZpZXcubW0KPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQotLS0gU291cmNlL1dlYktpdC9tYWMvV2ViVmlldy9XZWJWaWV3Lm1tCShyZXZpc2lv biAxMzU5MTgpCisrKyBTb3VyY2UvV2ViS2l0L21hYy9XZWJWaWV3L1dlYlZpZXcubW0JKHdvcmtp bmcgY29weSkKQEAgLTE2ODYsOSArMTY4Niw3IEBAIC0gKHZvaWQpX2NhY2hlRnJhbWVMb2FkRGVs ZWdhdGVJbXBsZW1lbnQKICAgICAvLyBmb3IgYmFja3dhcmRzIGNvbXBhdGliaWxpdHkuCiAgICAg UGFnZSogcGFnZSA9IGNvcmUoc2VsZik7CiAgICAgaWYgKHBhZ2UpIHsKLSAgICAgICAgdW5zaWdu ZWQgbWlsZXN0b25lcyA9IDA7Ci0gICAgICAgIGlmIChjYWNoZS0+ZGlkRmlyc3RMYXlvdXRJbkZy YW1lRnVuYykKLSAgICAgICAgICAgIG1pbGVzdG9uZXMgfD0gRGlkRmlyc3RMYXlvdXQ7CisgICAg ICAgIHVuc2lnbmVkIG1pbGVzdG9uZXMgPSBEaWRGaXJzdExheW91dDsKICAgICAgICAgaWYgKGNh Y2hlLT5kaWRGaXJzdFZpc3VhbGx5Tm9uRW1wdHlMYXlvdXRJbkZyYW1lRnVuYykKICAgICAgICAg ICAgIG1pbGVzdG9uZXMgfD0gRGlkRmlyc3RWaXN1YWxseU5vbkVtcHR5TGF5b3V0OwogICAgICAg ICBwYWdlLT5hZGRMYXlvdXRNaWxlc3RvbmVzKHN0YXRpY19jYXN0PExheW91dE1pbGVzdG9uZXM+ KG1pbGVzdG9uZXMpKTsK