154311 2016-02-16 15:05:33 -0800 Every RenderLayer should not have to remove itself from the scrollableArea set 2016-02-16 16:42:16 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 simon.fraser simon.fraser commit-queue esprehn+autocc glenn kondapallykalyan simon.fraser oldest_to_newest 1165340 0 simon.fraser 2016-02-16 15:05:33 -0800 Every RenderLayer should not have to remove itself from the scrollableArea set 1165341 1 271491 simon.fraser 2016-02-16 15:06:47 -0800 Created attachment 271491 Patch 1165356 2 simon.fraser 2016-02-16 15:24:42 -0800 https://trac.webkit.org/r196666 1165373 3 271491 sabouhallawa 2016-02-16 15:48:04 -0800 Comment on attachment 271491 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271491&action=review > Source/WebCore/rendering/RenderLayer.cpp:6743 > + m_registeredScrollableArea = true; I do not think this correct. You are changing m_registeredScrollableArea to true regardless addScrollableArea() returns true or not. This will lead to inconsistency if addedOrRemoved is set to false. > Source/WebCore/rendering/RenderLayer.cpp:6747 > + m_registeredScrollableArea = false; Ditto. > Source/WebCore/rendering/RenderLayer.cpp:6748 > + } I would write this block like this. I think it is clearer that the if-statement checks for consistency and the following code fixes it. if (isScrollable == m_registeredScrollableArea) return; if (!(isScrollable ? frameView.addScrollableArea(this) : frameView.removeScrollableArea(this))) return; updateNeedsCompositedScrolling(); m_registeredScrollableArea = !m_registeredScrollableArea; #if ENABLE(IOS_TOUCH_EVENTS) if (isScrollable && !hasAcceleratedTouchScrolling()) registerAsTouchEventListenerForScrolling(); else { // We only need the touch listener for unaccelerated overflow scrolling, so if we became // accelerated, remove ourselves as a touch event listener. unregisterAsTouchEventListenerForScrolling(); } #endif 1165382 4 simon.fraser 2016-02-16 16:10:30 -0800 (In reply to comment #3) > Comment on attachment 271491 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271491&action=review > > > Source/WebCore/rendering/RenderLayer.cpp:6743 > > + m_registeredScrollableArea = true; > > I do not think this correct. You are changing m_registeredScrollableArea to > true regardless addScrollableArea() returns true or not. This will lead to > inconsistency if addedOrRemoved is set to false. addScrollableArea() returns false only if the scrollableArea is in the set already, so it's safe to always set m_registeredScrollableArea to true. > > Source/WebCore/rendering/RenderLayer.cpp:6747 > > + m_registeredScrollableArea = false; > > Ditto. removeScrollableArea() will return false if the scrollableArea wasn't in the set, so it's safe to always set m_registeredScrollableArea to false here. > > > Source/WebCore/rendering/RenderLayer.cpp:6748 > > + } > > I would write this block like this. I think it is clearer that the > if-statement checks for consistency and the following code fixes it. > > if (isScrollable == m_registeredScrollableArea) > return; > > if (!(isScrollable ? frameView.addScrollableArea(this) : > frameView.removeScrollableArea(this))) > return; I don't really find that cleaner. 1165403 5 271491 sabouhallawa 2016-02-16 16:36:39 -0800 Comment on attachment 271491 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271491&action=review >>> Source/WebCore/rendering/RenderLayer.cpp:6743 >>> + m_registeredScrollableArea = true; >> >> I do not think this correct. You are changing m_registeredScrollableArea to true regardless addScrollableArea() returns true or not. This will lead to inconsistency if addedOrRemoved is set to false. > > addScrollableArea() returns false only if the scrollableArea is in the set already, so it's safe to always set m_registeredScrollableArea to true. Okay so why we store the return value in addedOrRemoved and then check its value two times after that? It looks like we can get rid of addedOrRemoved and check for m_registeredScrollableArea if we do the early return: if (isScrollable == m_registeredScrollableArea) return; >>> Source/WebCore/rendering/RenderLayer.cpp:6748 >>> + } >> >> I would write this block like this. I think it is clearer that the if-statement checks for consistency and the following code fixes it. >> >> if (isScrollable == m_registeredScrollableArea) >> return; >> >> if (!(isScrollable ? frameView.addScrollableArea(this) : frameView.removeScrollableArea(this))) >> return; >> >> updateNeedsCompositedScrolling(); >> m_registeredScrollableArea = !m_registeredScrollableArea; >> >> #if ENABLE(IOS_TOUCH_EVENTS) >> if (isScrollable && !hasAcceleratedTouchScrolling()) >> registerAsTouchEventListenerForScrolling(); >> else { >> // We only need the touch listener for unaccelerated overflow scrolling, so if we became >> // accelerated, remove ourselves as a touch event listener. >> unregisterAsTouchEventListenerForScrolling(); >> } >> #endif > > I don't really find that cleaner. Don't we prefer less indentation, early return and less verbose code? -- We define the variable addedOrRemoved and initialize it with false. -- We assign it to the return value of addScrollableArea() to removeScrollableArea() in two places. -- We check its value two times. -- We check if isScrollable is true and inside the then-block, we check if m_registeredScrollableArea is false -- Else we check if m_registeredScrollableAreais true. But with the following code, we can -- Reduce the six if-statments currently in this function to three if-statements. -- Delete the local variable addedOrRemoved. -- Reduce the assignment of m_registeredScrollableArea in two places to just one place. -- Eliminate the second level of indentation. if (isScrollable == m_registeredScrollableArea) return; if (isScrollable) frameView.addScrollableArea(this) else frameView.removeScrollableArea(this))) updateNeedsCompositedScrolling(); m_registeredScrollableArea = !m_registeredScrollableArea; #if ENABLE(IOS_TOUCH_EVENTS) if (isScrollable && !hasAcceleratedTouchScrolling()) registerAsTouchEventListenerForScrolling(); else { // We only need the touch listener for unaccelerated overflow scrolling, so if we became // accelerated, remove ourselves as a touch event listener. unregisterAsTouchEventListenerForScrolling(); } #endif 1165404 6 simon.fraser 2016-02-16 16:42:16 -0800 OK I'm sold. Please make a new bug and attach a patch. 271491 2016-02-16 15:06:47 -0800 2016-02-16 15:22:40 -0800 Patch bug-154311-20160216150627.patch text/plain 3859 simon.fraser U3VidmVyc2lvbiBSZXZpc2lvbjogMTk2NjQxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYmE1MTI3MDM3YTlkOTZi YWNkODRhODg1NWE4Mjg4ZThiMGUxYmU0NS4uYWViNzI0OWRkNDQ0ZmUwNzk4OTk0OTYzMzBkMTM2 NDQzNGI1NDgzNiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDIzIEBACiAyMDE2LTAyLTE2ICBTaW1v biBGcmFzZXIgIDxzaW1vbi5mcmFzZXJAYXBwbGUuY29tPgogCisgICAgICAgIEV2ZXJ5IFJlbmRl ckxheWVyIHNob3VsZCBub3QgaGF2ZSB0byByZW1vdmUgaXRzZWxmIGZyb20gdGhlIHNjcm9sbGFi bGVBcmVhIHNldAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9MTU0MzExCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgQSBzdWJzZXQgb2YgUmVuZGVyTGF5ZXJzIGFyZSBhcmUgc2Nyb2xsYWJsZSwgYW5kIGdldCBy ZWdpc3RlcmVkIG9uIHRoZSBGcmFtZVZpZXcsCisgICAgICAgIGJ1dCB3ZSBwYXkgdGhlIGNvc3Qg b2YgYSBoYXNoIGxvb2t1cCBmb3IgcmVtb3ZhbCBvbiBldmVyeSBSZW5kZXJMYXllciwgd2hpY2gg aXMgYSB3YXN0ZS4KKyAgICAgICAgCisgICAgICAgIFN0b3JlIGEgYml0IHRoYXQgdGVsbHMgUmVu ZGVyTGF5ZXIgdGhhdCBpdCdzIGluIHRoZSBzZXQgYW5kIG5lZWRzIHRvIGJlIHJlbW92ZWQuCisK KyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVyTGF5ZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6 UmVuZGVyTGF5ZXI6OlJlbmRlckxheWVyKToKKyAgICAgICAgKFdlYkNvcmU6OlJlbmRlckxheWVy Ojp+UmVuZGVyTGF5ZXIpOgorICAgICAgICAoV2ViQ29yZTo6UmVuZGVyTGF5ZXI6OmNhbGN1bGF0 ZUNsaXBSZWN0cyk6CisgICAgICAgICogcmVuZGVyaW5nL1JlbmRlckxheWVyLmg6CisKKzIwMTYt MDItMTYgIFNpbW9uIEZyYXNlciAgPHNpbW9uLmZyYXNlckBhcHBsZS5jb20+CisKICAgICAgICAg Um9sbG91dCByMTg4NjU5LiBUaGlzIGJyb2tlIHNjcm9sbGluZyBvZiBpZnJhbWVzIGFuZCBvdmVy ZmxvdyB3aGVuCiAgICAgICAgIG5hdmlnYXRpbmcgYmFjayB0byBhIHBhZ2UgaW4gdGhlIHBhZ2Ug Y2FjaGUuCiAgICAgICAgIApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1Jl bmRlckxheWVyLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJMYXllci5jcHAK aW5kZXggM2I5N2YwY2Y1NDIyYzlhNDRjMjMyYzI4MjRkNmI5ZjA4NDMxYzI2MS4uMDdlYTJkNTA5 NTFkZmQ1ZWNhZWIyYTk0ODA5NTk1NWYyYmNiMWIyZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNv cmUvcmVuZGVyaW5nL1JlbmRlckxheWVyLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJp bmcvUmVuZGVyTGF5ZXIuY3BwCkBAIC0yODAsNiArMjgwLDcgQEAgUmVuZGVyTGF5ZXI6OlJlbmRl ckxheWVyKFJlbmRlckxheWVyTW9kZWxPYmplY3QmIHJlbmRlcmVyTGF5ZXJNb2RlbE9iamVjdCkK ICAgICAsIG1faGFzVmlzaWJsZUNvbnRlbnQoZmFsc2UpCiAgICAgLCBtX3Zpc2libGVEZXNjZW5k YW50U3RhdHVzRGlydHkoZmFsc2UpCiAgICAgLCBtX2hhc1Zpc2libGVEZXNjZW5kYW50KGZhbHNl KQorICAgICwgbV9yZWdpc3RlcmVkU2Nyb2xsYWJsZUFyZWEoZmFsc2UpCiAgICAgLCBtXzNEVHJh bnNmb3JtZWREZXNjZW5kYW50U3RhdHVzRGlydHkodHJ1ZSkKICAgICAsIG1faGFzM0RUcmFuc2Zv cm1lZERlc2NlbmRhbnQoZmFsc2UpCiAgICAgLCBtX2hhc0NvbXBvc2l0aW5nRGVzY2VuZGFudChm YWxzZSkKQEAgLTM0NSw3ICszNDYsMTAgQEAgUmVuZGVyTGF5ZXI6On5SZW5kZXJMYXllcigpCiAg ICAgaWYgKGluUmVzaXplTW9kZSgpICYmICFyZW5kZXJlcigpLmRvY3VtZW50QmVpbmdEZXN0cm95 ZWQoKSkKICAgICAgICAgcmVuZGVyZXIoKS5mcmFtZSgpLmV2ZW50SGFuZGxlcigpLnJlc2l6ZUxh eWVyRGVzdHJveWVkKCk7CiAKLSAgICByZW5kZXJlcigpLnZpZXcoKS5mcmFtZVZpZXcoKS5yZW1v dmVTY3JvbGxhYmxlQXJlYSh0aGlzKTsKKyAgICBBU1NFUlQobV9yZWdpc3RlcmVkU2Nyb2xsYWJs ZUFyZWEgPT0gcmVuZGVyZXIoKS52aWV3KCkuZnJhbWVWaWV3KCkuY29udGFpbnNTY3JvbGxhYmxl QXJlYSh0aGlzKSk7CisKKyAgICBpZiAobV9yZWdpc3RlcmVkU2Nyb2xsYWJsZUFyZWEpCisgICAg ICAgIHJlbmRlcmVyKCkudmlldygpLmZyYW1lVmlldygpLnJlbW92ZVNjcm9sbGFibGVBcmVhKHRo aXMpOwogCiAgICAgaWYgKCFyZW5kZXJlcigpLmRvY3VtZW50QmVpbmdEZXN0cm95ZWQoKSkgewog I2lmIEVOQUJMRShJT1NfVE9VQ0hfRVZFTlRTKQpAQCAtNjczMCwxMCArNjczNCwxOCBAQCB2b2lk IFJlbmRlckxheWVyOjp1cGRhdGVTY3JvbGxhYmxlQXJlYVNldChib29sIGhhc092ZXJmbG93KQog CiAgICAgYm9vbCBpc1Njcm9sbGFibGUgPSBoYXNPdmVyZmxvdyAmJiBpc1Zpc2libGVUb0hpdFRl c3Q7CiAgICAgYm9vbCBhZGRlZE9yUmVtb3ZlZCA9IGZhbHNlOwotICAgIGlmIChpc1Njcm9sbGFi bGUpCi0gICAgICAgIGFkZGVkT3JSZW1vdmVkID0gZnJhbWVWaWV3LmFkZFNjcm9sbGFibGVBcmVh KHRoaXMpOwotICAgIGVsc2UKKyAgICAKKyAgICBBU1NFUlQobV9yZWdpc3RlcmVkU2Nyb2xsYWJs ZUFyZWEgPT0gZnJhbWVWaWV3LmNvbnRhaW5zU2Nyb2xsYWJsZUFyZWEodGhpcykpOworICAgIAor ICAgIGlmIChpc1Njcm9sbGFibGUpIHsKKyAgICAgICAgaWYgKCFtX3JlZ2lzdGVyZWRTY3JvbGxh YmxlQXJlYSkgeworICAgICAgICAgICAgYWRkZWRPclJlbW92ZWQgPSBmcmFtZVZpZXcuYWRkU2Ny b2xsYWJsZUFyZWEodGhpcyk7CisgICAgICAgICAgICBtX3JlZ2lzdGVyZWRTY3JvbGxhYmxlQXJl YSA9IHRydWU7CisgICAgICAgIH0KKyAgICB9IGVsc2UgaWYgKG1fcmVnaXN0ZXJlZFNjcm9sbGFi bGVBcmVhKSB7CiAgICAgICAgIGFkZGVkT3JSZW1vdmVkID0gZnJhbWVWaWV3LnJlbW92ZVNjcm9s bGFibGVBcmVhKHRoaXMpOworICAgICAgICBtX3JlZ2lzdGVyZWRTY3JvbGxhYmxlQXJlYSA9IGZh bHNlOworICAgIH0KICAgICAKICAgICBpZiAoYWRkZWRPclJlbW92ZWQpCiAgICAgICAgIHVwZGF0 ZU5lZWRzQ29tcG9zaXRlZFNjcm9sbGluZygpOwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUv cmVuZGVyaW5nL1JlbmRlckxheWVyLmggYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVy TGF5ZXIuaAppbmRleCBiMzc2ZmQ2N2E0MjRjZDk2Y2M5NTIxNDEzYzlmOWI2NTRiZmUzMzBlLi41 NWRiNTViOTdlYTUyYWE2MDg2ZDViMDViYTVkYTZiYzgzOTkyMzIyIDEwMDY0NAotLS0gYS9Tb3Vy Y2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyTGF5ZXIuaAorKysgYi9Tb3VyY2UvV2ViQ29yZS9y ZW5kZXJpbmcvUmVuZGVyTGF5ZXIuaApAQCAtMTAzOCw2ICsxMDM4LDcgQEAgcHJpdmF0ZToKICAg ICBib29sIG1faGFzVmlzaWJsZUNvbnRlbnQgOiAxOwogICAgIGJvb2wgbV92aXNpYmxlRGVzY2Vu ZGFudFN0YXR1c0RpcnR5IDogMTsKICAgICBib29sIG1faGFzVmlzaWJsZURlc2NlbmRhbnQgOiAx OworICAgIGJvb2wgbV9yZWdpc3RlcmVkU2Nyb2xsYWJsZUFyZWEgOiAxOwogCiAgICAgYm9vbCBt XzNEVHJhbnNmb3JtZWREZXNjZW5kYW50U3RhdHVzRGlydHkgOiAxOwogICAgIGJvb2wgbV9oYXMz RFRyYW5zZm9ybWVkRGVzY2VuZGFudCA6IDE7ICAvLyBTZXQgb24gYSBzdGFja2luZyBjb250ZXh0 IGxheWVyIHRoYXQgaGFzIDNEIGRlc2NlbmRhbnRzIGFueXdoZXJlCg==