Bug 152589 – Remove ScrollView::scrollOffset() in preparation for scrollOffset vs. scrollPosition clarification

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Fraser (smfr)]

Remove ScrollView::scrollOffset() in preparation for scrollOffset vs. scrollPosition clarification

[Simon Fraser (smfr)]

Created attachment 267989 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 267992 [details]
Patch

[Sam Weinig]

Comment on attachment 267992 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:2170
> +    IntPoint newPosition = scrollPosition();
> +    if (oldPosition != newPosition) {
>          updateLayerPositionsAfterScrolling();
>          if (frame().settings().acceleratedCompositingForFixedPositionEnabled())
>              updateCompositingLayersAfterScrolling();
> -        IntPoint newPosition = scrollPosition();
> -        scrollAnimator().setCurrentPosition(scrollPosition());
> +        scrollAnimator().setCurrentPosition(newPosition);

This subtly changes the logic of this function such that if updateLayerPositionsAfterScrolling() or updateCompositingLayersAfterScrolling() can affect scrollPosition(), the new current position will be different.  Are you confident they cannot?

> Source/WebCore/svg/SVGSVGElement.cpp:419
> +                LayoutPoint scrollOffset = view->scrollPosition();

Given that you are now using a position here, it might make sense to rename the local variable.

[Simon Fraser (smfr)]

(In reply to comment #3)
> Comment on attachment 267992 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267992&action=review
> 
> > Source/WebCore/page/FrameView.cpp:2170
> > +    IntPoint newPosition = scrollPosition();
> > +    if (oldPosition != newPosition) {
> >          updateLayerPositionsAfterScrolling();
> >          if (frame().settings().acceleratedCompositingForFixedPositionEnabled())
> >              updateCompositingLayersAfterScrolling();
> > -        IntPoint newPosition = scrollPosition();
> > -        scrollAnimator().setCurrentPosition(scrollPosition());
> > +        scrollAnimator().setCurrentPosition(newPosition);
> 
> This subtly changes the logic of this function such that if
> updateLayerPositionsAfterScrolling() or
> updateCompositingLayersAfterScrolling() can affect scrollPosition(), the new
> current position will be different.  Are you confident they cannot?

I am!

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/194438