Bug 154311 – Every RenderLayer should not have to remove itself from the scrollableArea set

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Fraser (smfr)]

Every RenderLayer should not have to remove itself from the scrollableArea set

[Simon Fraser (smfr)]

Created attachment 271491 [details]
Patch

[Simon Fraser (smfr)]

https://trac.webkit.org/r196666

[Said Abou-Hallawa]

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.

> 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

[Simon Fraser (smfr)]

(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.

[Said Abou-Hallawa]

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.

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

[Simon Fraser (smfr)]

OK I'm sold. Please make a new bug and attach a patch.