Bug 38286

Summary: Need to call FrameView::scrollPositionChanged when changing the scroll position when the ScrollView does not have a platformWidget
Product: WebKit Reporter: James Robinson <jamesr>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: CLOSED FIXED    
Severity: Normal CC: abarth, benjamin, eric, fishd, hausmann, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 35784    
Attachments:
Description Flags
Patch
darin: review+
Patch
none
Patch
none
Patch simon.fraser: review+

Description James Robinson 2010-04-28 14:46:28 PDT
Need to call FrameView::scrollPositionChanged when changing the scroll position when the ScrollView does not have a platformWidget
Comment 1 James Robinson 2010-04-28 14:51:45 PDT
The (very helpful) tests in http://trac.webkit.org/changeset/57971 exposed a repaint issue in ports that do not give ScrollViews a platformWidget (Safari Win, Chromium, etc).  See http://trac.webkit.org/export/58366/trunk/LayoutTests/fast/repaint/fixed-child-move-after-scroll.html.  The problem is that if there is not a platformWidget, FrameView::scrollPositionChanged() is not called every time the scroll changes and so repaint invalidations are not generated for all fixed position elements that might need them.
Comment 2 James Robinson 2010-04-28 14:54:31 PDT
Here's what the callstack looks like from the Mac port, which ends up calling FrameView::scrollPositionChanged through the platform layers:


#0  WebCore::FrameView::scrollPositionChanged (this=0x8184800)
#1  0x0034c02f in -[WebHTMLView(WebPrivate) _frameOrBoundsChanged] 
#2  0x9153042a in _nsnote_callback ()
#3  0x905bd47a in __CFXNotificationPost ()
#4  0x905bd753 in _CFXNotificationPostNotification ()
#5  0x9152d680 in -[NSNotificationCenter postNotificationName:object:userInfo:] ()
#6  0x91536ed8 in -[NSNotificationCenter postNotificationName:object:] ()
#7  0x9660f024 in -[NSView _postBoundsChangeNotification] ()
#8  0x967451a0 in -[NSView translateOriginToPoint:] ()
#9  0x96744435 in -[NSClipView _immediateScrollToPoint:] ()
#10 0x96743efe in -[NSClipView scrollToPoint:] ()
#11 0x967b10af in -[NSScrollView scrollClipView:toPoint:] ()
#12 0x966de4de in -[NSClipView _scrollTo:animate:] ()
#13 0x966de091 in -[NSClipView _scrollTo:] ()
#14 0x968701de in -[NSClipView _scrollPoint:fromView:] ()
#15 0x96870154 in -[NSView scrollPoint:] ()
#16 0x048bff9a in WebCore::ScrollView::platformSetScrollPosition
#17 0x048bce4e in WebCore::ScrollView::setScrollPosition
#18 0x041c59a9 in WebCore::ScrollView::scrollBy
#19 0x041be8aa in WebCore::DOMWindow::scrollBy
#20 0x04449e3c in WebCore::jsDOMWindowPrototypeFunctionScrollBy
(javascript)
Comment 3 James Robinson 2010-04-28 15:23:15 PDT
Created attachment 54628 [details]
Patch
Comment 4 James Robinson 2010-04-28 15:25:59 PDT
I've verified that this fixes all of the pixel tests in http://trac.webkit.org/changeset/57971 on the Chromium linux port (which doesn't use a platformWidget for the ScrollView).
Comment 5 Darin Adler 2010-04-28 16:11:13 PDT
Comment on attachment 54628 [details]
Patch

r=me

> +    virtual void scrollPositionChanged() {}

I'd prefer that this be private. There's no need for code outside the class to ever call this and a derived class can override a function even if it's private.
Comment 6 James Robinson 2010-04-28 17:33:29 PDT
Comment on attachment 54628 [details]
Patch

This patch is wrong, it doesn't handle scrolls driven by the keyboard (down arrow or page down) since they go through ScrollView::scroll() and not setScrollPosition().  I'll upload a new patch with a test for this momentarily and also address Darin's feedback.
Comment 7 James Robinson 2010-04-28 17:46:35 PDT
Created attachment 54644 [details]
Patch
Comment 8 James Robinson 2010-04-28 17:49:45 PDT
Created attachment 54648 [details]
Patch
Comment 9 Benjamin Poulain 2010-04-28 23:00:23 PDT
@James

Awesome, I was going to work on this today. :)

Could you please wait a few hours before landing it? I need to check if it is always called before ScrollView::scrollContents() so I can use it to implement 33150.
Comment 10 Benjamin Poulain 2010-04-29 00:50:04 PDT
Comment on attachment 54648 [details]
Patch

>      scrollContents(scrollDelta);
> +    scrollPositionChanged();

For my follow up patch, it would be better if those calls could be the other way around:
+    scrollPositionChanged();
     scrollContents(scrollDelta);
So all the positions are valid in the code of scrollContents() and I can use the repaint rects to implement 33150.


>  void ScrollView::scrollRectIntoViewRecursively(const IntRect& r)
> @@ -338,6 +339,7 @@ void ScrollView::setScrollPosition(const IntPoint& scrollPoint)
>          return;
>  
>      updateScrollbars(IntSize(newScrollPosition.x(), newScrollPosition.y()));
> +    scrollPositionChanged();
>  }

I don't think you need this call if you already call scrollPositionChanged() in ScrollView::valueChanged(). UpdateScrollbar() will end up in ScrollView::valueChanged(), so scrollPositionChanged() will be called twice.

Thanks again for making this patch, I go to bed with a problem, I woke up with your patch :)
Comment 11 James Robinson 2010-04-29 12:23:34 PDT
Created attachment 54727 [details]
Patch
Comment 12 James Robinson 2010-04-29 12:32:54 PDT
Thanks for the feedback, Benjamin.  I've moved the scrollPositionChanged() call as you suggested.  I plan to grab new expectations for fast/repaint/fixed-move-after-keyboard-scroll.html off of the bots after landing.

Simon Fraser suggested that it might be better to  move the scrollPositionChanged() call from being public on FrameView to being public on ScrollView.  The current only callsite to this is the mac platform layer here: http://trac.webkit.org/browser/trunk/WebKit/mac/WebView/WebHTMLView.mm#L1162.  I'm happy to move the function if it makes sense, but I'm less familiar with how this code is supposed to interact with the port layer.
Comment 13 Benjamin Poulain 2010-04-29 12:43:37 PDT
(In reply to comment #12)
> Simon Fraser suggested that it might be better to  move the
> scrollPositionChanged() call from being public on FrameView to being public on
> ScrollView.  The current only callsite to this is the mac platform layer here:
> http://trac.webkit.org/browser/trunk/WebKit/mac/WebView/WebHTMLView.mm#L1162. 
> I'm happy to move the function if it makes sense, but I'm less familiar with
> how this code is supposed to interact with the port layer.

I am not sure to understand what you mean. 

You cannot implement scrollPositionChanged() in ScrollView because you need to access the frame.

If you mean have scrollPositionChanged() public in ScrollView instead of Private, the I don't have anything against it :)
Comment 14 James Robinson 2010-04-30 15:26:59 PDT
Committed r58615: <http://trac.webkit.org/changeset/58615>
Comment 15 WebKit Review Bot 2010-04-30 16:04:37 PDT
http://trac.webkit.org/changeset/58615 might have broken SnowLeopard Intel Release (Tests)
Comment 16 Eric Seidel (no email) 2010-05-01 21:55:59 PDT
Totally lame to leave the tree broken :(
Comment 17 Benjamin Poulain 2010-05-02 14:19:04 PDT
(In reply to comment #16)
> Totally lame to leave the tree broken :(

I am surprised this patch breaks Mac, I think this path is never taken on this platform (the scrollbar of frame being native).

I can have a look tomorrow morning.
Comment 18 Simon Hausmann 2010-05-12 03:38:23 PDT
Revision r58615 cherry-picked into qtwebkit-2.0 with commit 57d10d5c05e59bbf7de8189ff47dd18d1be996dc
Comment 19 James Robinson 2010-05-12 12:36:30 PDT
Committed r59251: <http://trac.webkit.org/changeset/59251>
Comment 20 Simon Hausmann 2010-05-14 01:21:22 PDT
Revision r59251 cherry-picked into qtwebkit-2.0 with commit 3673bcff281e46da4b1d6829b8ff3c10939c6890
Comment 21 Simon Hausmann 2010-05-14 01:23:10 PDT
(In reply to comment #20)
> Revision r59251 cherry-picked into qtwebkit-2.0 with commit 3673bcff281e46da4b1d6829b8ff3c10939c6890

Oops, I reverted that. Doesn't look like an intentional change :)