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.
Created attachment 167203 [details] patch
Comment on attachment 167203 [details] patch Amen, brother. Amen.
Comment on attachment 167203 [details] patch Clearing flags on attachment: 167203 Committed r130700: <http://trac.webkit.org/changeset/130700>
All reviewed patches have been landed. Closing bug.
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.