98463 2012-10-04 16:28:39 -0700 Post-r130226 Cleanup: Comment a complicate if statement and make it a helper 2012-10-08 17:59:36 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 japhet japhet darin eric tonikitoo webkit.review.bot oldest_to_newest 735113 0 japhet 2012-10-04 16:28:39 -0700 As requested in https://bugs.webkit.org/show_bug.cgi?id=96539#c6 : >> Source/WebCore/rendering/RenderLayer.cpp:1793 >> + if ((frameElement && frameElement->scrollingMode() != ScrollbarAlwaysOff) || (!frameView->frame()->eventHandler()->autoscrollInProgress() && !frameView->wasScrolledByUser())) { > > it is turning to be a long line. An inline helper function could spread this out over multiple lines and allow a comment on each line. 735117 1 167203 japhet 2012-10-04 16:38:12 -0700 Created attachment 167203 patch 737155 2 167203 eric 2012-10-08 16:02:23 -0700 Comment on attachment 167203 patch Amen, brother. Amen. 737198 3 167203 webkit.review.bot 2012-10-08 16:20:27 -0700 Comment on attachment 167203 patch Clearing flags on attachment: 167203 Committed r130700: <http://trac.webkit.org/changeset/130700> 737199 4 webkit.review.bot 2012-10-08 16:20:30 -0700 All reviewed patches have been landed. Closing bug. 737283 5 167203 darin 2012-10-08 17:59:36 -0700 Comment on attachment 167203 patch View in context: https://bugs.webkit.org/attachment.cgi?id=167203&action=review > Source/WebCore/rendering/RenderLayer.cpp:1771 > + // If scrollbars aren't explicitly forbidden, permit scrolling. > + if (frameElement && frameElement->scrollingMode() != ScrollbarAlwaysOff) > + return true; > + > + // If scrollbars are forbidden, user initiated scrolls should obviously be ignored. > + if (frameView->wasScrolledByUser()) > + return false; > + > + // Forbid autoscrolls when scrollbars are off, but permits other programmatic scrolls, > + // like navigation to an anchor. > + return !frameView->frame()->eventHandler()->autoscrollInProgress(); Great idea to do this! I’d prefer comments that talk more about “why” and do less restatement of what the code says. Kind of like this: // The goal here is to forbid user-initiated scrolling in frames that forbid scrollbars. // If scrollbars are not forbidden, we’d like to allow scrolling without further checks. // The most obvious kind of user-initiated scrolling is indicated by state in the FrameView. // Autoscrolling is an indirect form of user-initiated scrolling, also forbidden, and requires a separate check. I also think it would be clearer if the last return statement was an if statement saying "if (autoscrollInProgress) return false;" It’s also unclear why “was scrolled by user” is a way to tell that the current scrolling is a user scroll. The word “was” there seems to indicate we’re indicating something about the past, not about the current scroll event. And given its true purpose, why doesn’t the “was scrolled by user” function return true when autoscroll is in progress? If it did, this code would be simpler. 167203 2012-10-04 16:38:12 -0700 2012-10-08 17:59:36 -0700 patch rl.txt text/plain 2628 japhet SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEzMDQzMikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDEyLTEwLTA0ICBOYXRlIENo YXBpbiAgPGphcGhldEBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUG9zdC1yMTMwMjI2IENsZWFu dXA6IENvbW1lbnQgYSBjb21wbGljYXRlZCBpZiBzdGF0ZW1lbnQgYW5kIG1ha2UgaXQgYSBoZWxw ZXIuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD05ODQ2 MworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE5vIG5l dyB0ZXN0cywgcmVmYWN0b3Igb25seS4KKworICAgICAgICAqIHJlbmRlcmluZy9SZW5kZXJMYXll ci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpmcmFtZUVsZW1lbnRBbmRWaWV3UGVybWl0U2Nyb2xs KToKKyAgICAgICAgKFdlYkNvcmU6OlJlbmRlckxheWVyOjpzY3JvbGxSZWN0VG9WaXNpYmxlKToK KwogMjAxMi0xMC0wNCAgUnlvc3VrZSBOaXdhICA8cm5pd2FAd2Via2l0Lm9yZz4KIAogICAgICAg ICBCdWlsZCBmaXggYWZ0ZXIgcjEzMDQxMS4gQWRkIHRoZSByaWdodCBvZmZzZXQuCkluZGV4OiBT b3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyTGF5ZXIuY3BwCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNv dXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJMYXllci5jcHAJKHJldmlzaW9uIDEzMDMzOCkK KysrIFNvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJMYXllci5jcHAJKHdvcmtpbmcgY29w eSkKQEAgLTE3NTYsNiArMTc1NiwyMSBAQAogICAgICAgICByZW5kZXJlcigpLT5ub2RlKCktPmRv Y3VtZW50KCktPmV2ZW50UXVldWUoKS0+ZW5xdWV1ZU9yRGlzcGF0Y2hTY3JvbGxFdmVudChyZW5k ZXJlcigpLT5ub2RlKCksIERvY3VtZW50RXZlbnRRdWV1ZTo6U2Nyb2xsRXZlbnRFbGVtZW50VGFy Z2V0KTsKIH0KIAorc3RhdGljIGlubGluZSBib29sIGZyYW1lRWxlbWVudEFuZFZpZXdQZXJtaXRT Y3JvbGwoSFRNTEZyYW1lRWxlbWVudCogZnJhbWVFbGVtZW50LCBGcmFtZVZpZXcqIGZyYW1lVmll dykgCit7CisgICAgLy8gSWYgc2Nyb2xsYmFycyBhcmVuJ3QgZXhwbGljaXRseSBmb3JiaWRkZW4s IHBlcm1pdCBzY3JvbGxpbmcuCisgICAgaWYgKGZyYW1lRWxlbWVudCAmJiBmcmFtZUVsZW1lbnQt PnNjcm9sbGluZ01vZGUoKSAhPSBTY3JvbGxiYXJBbHdheXNPZmYpCisgICAgICAgIHJldHVybiB0 cnVlOworCisgICAgLy8gSWYgc2Nyb2xsYmFycyBhcmUgZm9yYmlkZGVuLCB1c2VyIGluaXRpYXRl ZCBzY3JvbGxzIHNob3VsZCBvYnZpb3VzbHkgYmUgaWdub3JlZC4KKyAgICBpZiAoZnJhbWVWaWV3 LT53YXNTY3JvbGxlZEJ5VXNlcigpKQorICAgICAgICByZXR1cm4gZmFsc2U7CisKKyAgICAvLyBG b3JiaWQgYXV0b3Njcm9sbHMgd2hlbiBzY3JvbGxiYXJzIGFyZSBvZmYsIGJ1dCBwZXJtaXRzIG90 aGVyIHByb2dyYW1tYXRpYyBzY3JvbGxzLAorICAgIC8vIGxpa2UgbmF2aWdhdGlvbiB0byBhbiBh bmNob3IuCisgICAgcmV0dXJuICFmcmFtZVZpZXctPmZyYW1lKCktPmV2ZW50SGFuZGxlcigpLT5h dXRvc2Nyb2xsSW5Qcm9ncmVzcygpOworfQorCiB2b2lkIFJlbmRlckxheWVyOjpzY3JvbGxSZWN0 VG9WaXNpYmxlKGNvbnN0IExheW91dFJlY3QmIHJlY3QsIGNvbnN0IFNjcm9sbEFsaWdubWVudCYg YWxpZ25YLCBjb25zdCBTY3JvbGxBbGlnbm1lbnQmIGFsaWduWSkKIHsKICAgICBSZW5kZXJMYXll ciogcGFyZW50TGF5ZXIgPSAwOwpAQCAtMTgwNiw4ICsxODIxLDcgQEAKICAgICAgICAgICAgICAg ICBpZiAob3duZXJFbGVtZW50LT5oYXNUYWdOYW1lKGZyYW1lVGFnKSB8fCBvd25lckVsZW1lbnQt Pmhhc1RhZ05hbWUoaWZyYW1lVGFnKSkKICAgICAgICAgICAgICAgICAgICAgZnJhbWVFbGVtZW50 ID0gc3RhdGljX2Nhc3Q8SFRNTEZyYW1lRWxlbWVudCo+KG93bmVyRWxlbWVudCk7CiAKLSAgICAg ICAgICAgICAgICBpZiAoKGZyYW1lRWxlbWVudCAmJiBmcmFtZUVsZW1lbnQtPnNjcm9sbGluZ01v ZGUoKSAhPSBTY3JvbGxiYXJBbHdheXNPZmYpCi0gICAgICAgICAgICAgICAgICAgIHx8ICghZnJh bWVWaWV3LT5mcmFtZSgpLT5ldmVudEhhbmRsZXIoKS0+YXV0b3Njcm9sbEluUHJvZ3Jlc3MoKSAm JiAhZnJhbWVWaWV3LT53YXNTY3JvbGxlZEJ5VXNlcigpKSkgeworICAgICAgICAgICAgICAgIGlm IChmcmFtZUVsZW1lbnRBbmRWaWV3UGVybWl0U2Nyb2xsKGZyYW1lRWxlbWVudCwgZnJhbWVWaWV3 KSkgewogICAgICAgICAgICAgICAgICAgICBMYXlvdXRSZWN0IHZpZXdSZWN0ID0gZnJhbWVWaWV3 LT52aXNpYmxlQ29udGVudFJlY3QoKTsKICAgICAgICAgICAgICAgICAgICAgTGF5b3V0UmVjdCBl eHBvc2VSZWN0ID0gZ2V0UmVjdFRvRXhwb3NlKHZpZXdSZWN0LCByZWN0LCBhbGlnblgsIGFsaWdu WSk7CiAK