Bug 259018 – Potentially redundant condition in LocalFrameView style functions

• Status: NEW
• Component: WebCore Misc.

[Luke Warlow]

horizontalOverscrollBehavior(), verticalOverscrollBehavior() and scrollbarWidthStyle() inside of LocalFrameView have an if (scrollingObject && renderView()) condition before accessing the respective style values.

The renderView() check may be redundant.

scrollbarWidthStyle() has it as it's a copy of the overscrollBehavior functions.

They appear to have it as a left over from when they accessed a function inside of renderView() which was subsequently removed.

Check if this is needed for any of these functions and remove as appropriate.

[Ahmad Saleem]

Do you want to expand scope of this bug?

I think we can add more checks in 'addScrollableArea' and 'containsScrollableArea' to also assert to confirm we have 'ScrollableArea'.


bool LocalFrameView::addScrollableArea(ScrollableArea* scrollableArea)
{
   ASSERT(scrollableArea);


_________

bool LocalFrameView::containsScrollableArea(ScrollableArea* scrollableArea) const
{

    ASSERT(scrollableArea);
    if (!m_scrollableAreas || !scrollableArea)
        return false;

^ Kind of early return, if we don't have scrollableArea.

_____

I think it would make scroll code bit more robust as well.

________


If not, I am happy to separate PR, above compiles in my local build. :-)

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #1)
> Do you want to expand scope of this bug?
> 
> I think we can add more checks in 'addScrollableArea' and
> 'containsScrollableArea' to also assert to confirm we have 'ScrollableArea'.
> 
> 
> bool LocalFrameView::addScrollableArea(ScrollableArea* scrollableArea)
> {
>    ASSERT(scrollableArea);
> 
> 
> _________
> 
> bool LocalFrameView::containsScrollableArea(ScrollableArea* scrollableArea)
> const
> {
> 
>     ASSERT(scrollableArea);
>     if (!m_scrollableAreas || !scrollableArea)
>         return false;
> 
> ^ Kind of early return, if we don't have scrollableArea.
> 
> _____
> 
> I think it would make scroll code bit more robust as well.
> 
> ________
> 
> 
> If not, I am happy to separate PR, above compiles in my local build. :-)

Although 'containsScrollableArea' is not used much as well. (If we can also explore, if we can remove it).

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #1)
> Do you want to expand scope of this bug?
> 
> I think we can add more checks in 'addScrollableArea' and
> 'containsScrollableArea' to also assert to confirm we have 'ScrollableArea'.
> 
> 
> bool LocalFrameView::addScrollableArea(ScrollableArea* scrollableArea)
> {
>    ASSERT(scrollableArea);
> 
> 
> _________
> 
> bool LocalFrameView::containsScrollableArea(ScrollableArea* scrollableArea)
> const
> {
> 
>     ASSERT(scrollableArea);
>     if (!m_scrollableAreas || !scrollableArea)
>         return false;
> 
> ^ Kind of early return, if we don't have scrollableArea.
> 
> _____
> 
> I think it would make scroll code bit more robust as well.
> 
> ________
> 
> 
> If not, I am happy to separate PR, above compiles in my local build. :-)

Although 'containsScrollableArea' is not used much as well. (If we can also explore, if we can remove it).

[Luke Warlow]

Could you do this as a separate PR please. Not quite sure if this bug is actionable currently just something Darin spotted during a code review. Your changes look reasonable with my limited understanding.

[Radar WebKit Bug Importer]

<rdar://problem/112331229>