Bug 259018

Summary: Potentially redundant condition in LocalFrameView style functions
Product: WebKit Reporter: Luke Warlow <lwarlow>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: ahmad.saleem792, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   

Description Luke Warlow 2023-07-08 14:18:05 PDT 
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.
Comment 1 Ahmad Saleem 2023-07-08 16:13:08 PDT 
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. :-)
Comment 2 Ahmad Saleem 2023-07-08 16:15:58 PDT 
(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).
Comment 3 Ahmad Saleem 2023-07-08 16:16:04 PDT 
(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).
Comment 4 Luke Warlow 2023-07-08 16:23:51 PDT 
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.
Comment 5 Radar WebKit Bug Importer 2023-07-15 14:19:16 PDT 
<rdar://problem/112331229>