Bug 154311 - Every RenderLayer should not have to remove itself from the scrollableArea set
Summary: Every RenderLayer should not have to remove itself from the scrollableArea set
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-16 15:05 PST by Simon Fraser (smfr)
Modified: 2016-02-16 16:42 PST (History)
5 users (show)

See Also:


Attachments
Patch (3.77 KB, patch)
2016-02-16 15:06 PST, Simon Fraser (smfr)
zalan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.