Bug 98463 - Post-r130226 Cleanup: Comment a complicate if statement and make it a helper
Summary: Post-r130226 Cleanup: Comment a complicate if statement and make it a helper
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-04 16:28 PDT by Nate Chapin
Modified: 2012-10-08 17:59 PDT (History)
4 users (show)

See Also:


Attachments
patch (2.57 KB, patch)
2012-10-04 16:38 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2012-10-04 16:28:39 PDT
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.
Comment 1 Nate Chapin 2012-10-04 16:38:12 PDT
Created attachment 167203 [details]
patch
Comment 2 Eric Seidel (no email) 2012-10-08 16:02:23 PDT
Comment on attachment 167203 [details]
patch

Amen, brother.  Amen.
Comment 3 WebKit Review Bot 2012-10-08 16:20:27 PDT
Comment on attachment 167203 [details]
patch

Clearing flags on attachment: 167203

Committed r130700: <http://trac.webkit.org/changeset/130700>
Comment 4 WebKit Review Bot 2012-10-08 16:20:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Darin Adler 2012-10-08 17:59:36 PDT
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.