Bug 154311

Summary: Every RenderLayer should not have to remove itself from the scrollableArea set
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: New BugsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, kondapallykalyan, simon.fraser
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch zalan: review+

Description Simon Fraser (smfr) 2016-02-16 15:05:33 PST
Every RenderLayer should not have to remove itself from the scrollableArea set
Comment 1 Simon Fraser (smfr) 2016-02-16 15:06:47 PST
Created attachment 271491 [details]
Patch
Comment 2 Simon Fraser (smfr) 2016-02-16 15:24:42 PST
https://trac.webkit.org/r196666
Comment 3 Said Abou-Hallawa 2016-02-16 15:48:04 PST
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
Comment 4 Simon Fraser (smfr) 2016-02-16 16:10:30 PST
(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.
Comment 5 Said Abou-Hallawa 2016-02-16 16:36:39 PST
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
Comment 6 Simon Fraser (smfr) 2016-02-16 16:42:16 PST
OK I'm sold. Please make a new bug and attach a patch.