Bug 110066 – [EFL][Qt][WK2] Meta bug for not using ScrollView::m_fixedVisibleContentRect.

• Component: Layout and Rendering
• Status: RESOLVED WONTFIX

[Dongseong Hwang]

Kenneth raised a question that we still need to use ScrollView::m_fixedVisibleContentRect.

Currently, we used fixedVisibleContentRect to make WebCore know actual scroll position and viewport size that PageViewportController deals with.
After Bug 105978 and Bug 107424, ScrollView can trace actual viewport size of PageViewportController. So if we make ScrollView know actual scroll position, we can remove fixedVisibleContentRect as well as delegates scrolling.

This huge changes have benefits as follow:
1. We share more code with other ports
2. It would be an intermediate step to go with scrolling coordinator (AFAIK cmarcelo and MPozdnyakov are investigating)

[Dongseong Hwang]

Created attachment 188786 [details]
Not for Review: WIP to share the direction

[Dongseong Hwang]

(In reply to comment #1)
> Created an attachment (id=188786) [details]
> Not for Review: WIP to share the direction

Could you feedback this proposal? I want to discuss about it to find the best direction :)

First of all, I'm curious that could I turn off delegates scroll. In my understanding, 'useFixedLayout' does not need 'delegates scroll'. I also want qt webkit1 to not use 'delegates scroll', which allows me remove all code related to 'delegates scroll'.

Second, I need to confirm I understand correctly efl's device scale factor policy.
If we launch MiniBrowser with -r 2 option, does 800x600 EwkView want to make PageViewportController and WebProcess know the viewport size 400x300? and then EwkView finally scale x2 when rendering?
In above example, which is fixedLayoutSize: 800x600 or 400x300?

If you have any question and opinion, please let me know! :)

[Kenneth Rohde Christiansen]

Comment on attachment 188786 [details]
Not for Review: WIP to share the direction

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

We really need to split this up... like maybe start by getting rid of the setViewportSize

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2135
>      // This applies when the application UI handles scrolling, in which case RenderLayerCompositor doesn't need to manage it.
> -    if (m_renderView->frameView()->delegatesScrolling())
> +    if (!m_renderView->frameView()->canHaveScrollbars())
>          return false;

I am not sure this is really correct... other platforms might have canHaveScrollbars being false.

Maybe this should be another patch

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2662
> -void WebPageProxy::pageDidScroll()
> +void WebPageProxy::pageDidScroll(const IntPoint& scrollPosition)
>  {
>      m_uiClient.pageDidScroll(this);
>  #if PLATFORM(MAC)
>      dismissCorrectionPanel(ReasonForDismissingAlternativeTextIgnored);
>  #endif
> +#if USE(TILED_BACKING_STORE)
> +    m_pageClient->pageDidRequestScroll(scrollPosition);
> +#endif
>  }

One is that it did scroll and the other is a request to do so... seems weird

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:447
> -#if PLATFORM(QT) || PLATFORM(EFL)
> -    if (m_page->useFixedLayout()) {
> -        // The below method updates the size().
> -        m_page->resizeToContentsIfNeeded();
> -        m_page->drawingArea()->layerTreeHost()->sizeDidChange(m_page->size());
> -    }
> +#if USE(COORDINATED_GRAPHICS)
> +    if (m_page->useFixedLayout())
> +        m_page->drawingArea()->layerTreeHost()->sizeDidChange(size);

You tested this on Qt?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1242
> -    m_page->settings()->setEnableScrollAnimator(!fixed);
> +//    m_page->settings()->setEnableScrollAnimator(!fixed);

No... :-) please dotn add outcommented code

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1250
> -    view->setDelegatesScrolling(fixed);
> +//    view->setDelegatesScrolling(fixed);

same

[Dongseong Hwang]

(In reply to comment #3)
> (From update of attachment 188786 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188786&action=review

Thank you for review!

> We really need to split this up... like maybe start by getting rid of the setViewportSize

I'll separate and file several bugs, so this bug's tile includes 'Meta'. :)

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:2135
> >      // This applies when the application UI handles scrolling, in which case RenderLayerCompositor doesn't need to manage it.
> > -    if (m_renderView->frameView()->delegatesScrolling())
> > +    if (!m_renderView->frameView()->canHaveScrollbars())
> >          return false;
> 
> I am not sure this is really correct... other platforms might have canHaveScrollbars being false.
> Maybe this should be another patch

yes

> > Source/WebKit2/UIProcess/WebPageProxy.cpp:2662
> > -void WebPageProxy::pageDidScroll()
> > +void WebPageProxy::pageDidScroll(const IntPoint& scrollPosition)
> >  {
> >      m_uiClient.pageDidScroll(this);
> >  #if PLATFORM(MAC)
> >      dismissCorrectionPanel(ReasonForDismissingAlternativeTextIgnored);
> >  #endif
> > +#if USE(TILED_BACKING_STORE)
> > +    m_pageClient->pageDidRequestScroll(scrollPosition);
> > +#endif
> >  }
> 
> One is that it did scroll and the other is a request to do so... seems weird

yes, I think so. I'll make code more consistent.

> > Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:447
> > -#if PLATFORM(QT) || PLATFORM(EFL)
> > -    if (m_page->useFixedLayout()) {
> > -        // The below method updates the size().
> > -        m_page->resizeToContentsIfNeeded();
> > -        m_page->drawingArea()->layerTreeHost()->sizeDidChange(m_page->size());
> > -    }
> > +#if USE(COORDINATED_GRAPHICS)
> > +    if (m_page->useFixedLayout())
> > +        m_page->drawingArea()->layerTreeHost()->sizeDidChange(size);
> 
> You tested this on Qt?

yes, I have tested both EFL and Qt.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1242
> > -    m_page->settings()->setEnableScrollAnimator(!fixed);
> > +//    m_page->settings()->setEnableScrollAnimator(!fixed);
> 
> No... :-) please dotn add outcommented code
> same

absolutely.

[Dongseong Hwang]

Created attachment 189033 [details]
I'll separate this patch and file several bug. I submitted this patch to share my WIP until now.

[Michael Catanzaro]

Closing this bug because the EFL port has been removed from trunk.

If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.