Bug 77063

Summary: [Qt][WK2] Child layers appear in wrong position when scrolling
Product: WebKit Reporter: Noam Rosenthal <noam>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, gustavo, hausmann, hyatt, jturcotte, kenneth, simon.fraser, webkit.review.bot, xan.lopez, zoltan
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
gustavo: commit-queue-
Patch
simon.fraser: review+
Patch none

Description Noam Rosenthal 2012-01-25 17:11:26 PST
Since adding the fixedVisibleContentRect to WebKit2, child layers are displaced when scrolling or zooming. This is easily testable by running the http://webkit.org/blog-files/3d-transforms/morphing-cubes.html in touch mode.
Comment 1 Noam Rosenthal 2012-01-25 17:13:37 PST
Created attachment 124037 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-01-26 02:53:55 PST
Comment on attachment 124037 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:795
>      Frame* frame = m_page->mainFrame();
> +    m_fixedVisibleContentRect = rect;
>  
>      frame->view()->setFixedVisibleContentRect(rect);
>  }

can't you receive it from the FrameView instead? or is that a performance issue?
Comment 3 Jocelyn Turcotte 2012-01-26 04:22:29 PST
The scrolling of the layers seems to be done by a scroll layer.
It would probably be better to prevent the use of this layer instead, RenderLayerCompositor::requiresScrollLayer seems to do exactly that.
Comment 4 Noam Rosenthal 2012-01-26 05:59:29 PST
Comment on attachment 124037 [details]
Patch

Will try another approach based on comments.
Comment 5 Noam Rosenthal 2012-01-26 10:29:10 PST
Created attachment 124137 [details]
Patch
Comment 6 Noam Rosenthal 2012-01-26 10:30:30 PST
Created attachment 124138 [details]
Patch
Comment 7 Gustavo Noronha (kov) 2012-01-26 13:54:51 PST
Comment on attachment 124138 [details]
Patch

Attachment 124138 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11163476
Comment 8 Noam Rosenthal 2012-01-26 16:39:54 PST
Created attachment 124212 [details]
Patch
Comment 9 Simon Fraser (smfr) 2012-01-26 22:22:46 PST
Comment on attachment 124212 [details]
Patch

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

> Source/WebCore/platform/ScrollView.h:127
> +    virtual void delegatesScrollingDidChange() { }

Why is this public?
Comment 10 Noam Rosenthal 2012-01-26 22:34:19 PST
(In reply to comment #9)
> (From update of attachment 124212 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124212&action=review
> 
> > Source/WebCore/platform/ScrollView.h:127
> > +    virtual void delegatesScrollingDidChange() { }
> 
> Why is this public?

Oversight :) 
Will make protected when committing.
Comment 11 Noam Rosenthal 2012-01-27 06:09:16 PST
Created attachment 124302 [details]
Patch
Comment 12 WebKit Review Bot 2012-01-27 07:35:57 PST
Comment on attachment 124302 [details]
Patch

Clearing flags on attachment: 124302

Committed r106121: <http://trac.webkit.org/changeset/106121>
Comment 13 WebKit Review Bot 2012-01-27 07:36:03 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Andras Becsi 2012-01-30 05:49:41 PST
Since this change MiniBrowser does not print the new tiles after the kinetic animation finishes.

If I remove:

// This applies when the application UI handles scrolling, in which case RenderLayerCompositor doesn't need to manage it.
if (m_renderView->frameView()->delegatesScrolling())
    return false;

from WebCore/rendering/RenderLayerCompositor.cpp: RenderLayerCompositor::requiresScrollLayer, it works again.
This might be because !m_renderView->frameView()->platformWidget() is actually true.

Since I'm not familiar with this code, I do not know if this is actually a revealed bug somewhere else, or is that return not needed.
Comment 15 Noam Rosenthal 2012-01-30 06:22:46 PST
(In reply to comment #14)
> Since this change MiniBrowser does not print the new tiles after the kinetic animation finishes.
> 
> If I remove:
> 
> // This applies when the application UI handles scrolling, in which case RenderLayerCompositor doesn't need to manage it.
> if (m_renderView->frameView()->delegatesScrolling())
>     return false;
> 
> from WebCore/rendering/RenderLayerCompositor.cpp: RenderLayerCompositor::requiresScrollLayer, it works again.
> This might be because !m_renderView->frameView()->platformWidget() is actually true.
!m_renderView->frameView()->platformWidget() is always true in WebKit2.

> 
> Since I'm not familiar with this code, I do not know if this is actually a revealed bug somewhere else, or is that return not needed.

We delegate scrolling in WebKit2, so we shouldn't have the compositor control these layers... I will investigate, but I have less experience with the scrolling code, so if someone else wants to have a look be my guest.
Comment 16 Andras Becsi 2012-01-30 07:20:00 PST
(In reply to comment #15)
Jocelyn helped finding the cause of this issue, I opened a bug for it:
https://bugs.webkit.org/show_bug.cgi?id=77338