[webkit-reviews] review granted: [Bug 42949] REGRESSION: Incorrect repaint on scrolling with position:fixed element on Windows : [Attachment 64606] v1 patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Darin Fisher (:fishd, Google)
<fishd at chromium.org>'s request for review:
Bug 42949: REGRESSION: Incorrect repaint on scrolling with position:fixed
element on Windows
https://bugs.webkit.org/show_bug.cgi?id=42949

Attachment 64606: v1 patch
https://bugs.webkit.org/attachment.cgi?id=64606&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Thanks for tackling this.

Normally we use prefixes like [Chromium] on patches that only touch
platform-specific code. This patch is entirely in platform-independent code, so
we should not use the [Chromium] prefix on the bug's title. I think that's
fixed in the bug, but it's still wrong in the patch.

Is there any way to make a regression test for this? Until we have at least a
bit of automated testing I think we'll keep breaking this.

The division of responsibilities between ScrollView and FrameView here is not
good. It seems bizarre that repaintFixedElementsAfterScrolling is a virtual
function, yet scrollPositionChanged is non-virtual. I think we'd be better off
if we made scrollPositionChanged a virtual function called by ScrollView. If we
did that, then I think we might be able to leave the call to
repaintFixedElementsAfterScrolling inside scrollPositionChanged.

It seems clear to me that the calls to repaintFixedElementsAfterScrolling
should all be inside ScrollView, but the name of that function blurs the lines
of responsibilities. After all, a scroll view doesn't know about HTML and the
term "fixed elements" seems a little HTML-specific.

It would be worth regularizing and fixing this, although not required in this
bug. At this point, it seems likely that any class other than FrameView derived
from ScrollView will not work correctly because of the way we've set the
classes up.

r=me on the change, but please consider spending a bit more time on this -- if
we improve the division of responsibilities between ScrollView and FrameView we
could save ourselves future headaches.