Bug 98463 – Post-r130226 Cleanup: Comment a complicate if statement and make it a helper

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

As requested in https://bugs.webkit.org/show_bug.cgi?id=96539#c6 :

>> Source/WebCore/rendering/RenderLayer.cpp:1793
>> +                if ((frameElement && frameElement->scrollingMode() != ScrollbarAlwaysOff) || (!frameView->frame()->eventHandler()->autoscrollInProgress() && !frameView->wasScrolledByUser())) {
> 
> it is turning to be a long line.

An inline helper function could spread this out over multiple lines and allow a comment on each line.

[Nate Chapin]

Created attachment 167203 [details]
patch

[Eric Seidel (no email)]

Comment on attachment 167203 [details]
patch

Amen, brother.  Amen.

[WebKit Review Bot]

Comment on attachment 167203 [details]
patch

Clearing flags on attachment: 167203

Committed r130700: <http://trac.webkit.org/changeset/130700>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 167203 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167203&action=review

> Source/WebCore/rendering/RenderLayer.cpp:1771
> +    // If scrollbars aren't explicitly forbidden, permit scrolling.
> +    if (frameElement && frameElement->scrollingMode() != ScrollbarAlwaysOff)
> +        return true;
> +
> +    // If scrollbars are forbidden, user initiated scrolls should obviously be ignored.
> +    if (frameView->wasScrolledByUser())
> +        return false;
> +
> +    // Forbid autoscrolls when scrollbars are off, but permits other programmatic scrolls,
> +    // like navigation to an anchor.
> +    return !frameView->frame()->eventHandler()->autoscrollInProgress();

Great idea to do this!

I’d prefer comments that talk more about “why” and do less restatement of what the code says. Kind of like this:

    // The goal here is to forbid user-initiated scrolling in frames that forbid scrollbars.

    // If scrollbars are not forbidden, we’d like to allow scrolling without further checks.

    // The most obvious kind of user-initiated scrolling is indicated by state in the FrameView.

    // Autoscrolling is an indirect form of user-initiated scrolling, also forbidden, and requires a separate check.

I also think it would be clearer if the last return statement was an if statement saying "if (autoscrollInProgress) return false;"

It’s also unclear why “was scrolled by user” is a way to tell that the current scrolling is a user scroll. The word “was” there seems to indicate we’re indicating something about the past, not about the current scroll event.

And given its true purpose, why doesn’t the “was scrolled by user” function return true when autoscroll is in progress? If it did, this code would be simpler.