Bug 99850 – [EFL][WK2] Use the port independent PageViewportController

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Yael]

WebKit EFL should start using PageViewportController to support viewport meta tag.

A patch is coming soon.

[Yael]

Created attachment 169748 [details]
Patch

Take PageViewportController into use, and rely on it to calculate  scroll position and zoom level. With this patch, we can do intra page navigation and use the scrollwheel to scroll, when WTF_USE_COORDINATED_GRAPHICS and all related flags are enabled.

With this patch touch events are broken since they do not yet take the scale into account (They work only with scale = 1.0)
Also, mouse events do not work after scrolling.
There is no impact when WTF_USE_COORDINATED_GRAPHICS is turned off, and I am still working on these issues, but would like to start the review going.

[Gyuyoung Kim]

Comment on attachment 169748 [details]
Patch

Attachment 169748 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14459745

[Kenneth Rohde Christiansen]

Comment on attachment 169748 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.messages.in:76
> +#if PLATFORM(QT) || PLATFORM(EFL) && USE(COORDINATED_GRAPHICS)

Maybe we should have a different define?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:281
> +static Evas_Coord_Point adjustedPointForScaling(Evas_Coord_Point point, Ewk_View_Private_Data* priv)

Can't we follow Qt naming?

QPointF QQuickWebView::mapToWebContent(const QPointF& pointInViewCoordinates) const
QRectF QQuickWebView::mapRectToWebContent(const QRectF& rectInViewCoordinates) const

Also I would add priv before point as it is a hard requirement

[Kenneth Rohde Christiansen]

Andras, Jocelyn, please have a look as well

[Yael]

(In reply to comment #3)
Thanks for your review :)
> (From update of attachment 169748 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169748&action=review
> 
> > Source/WebKit2/UIProcess/WebPageProxy.messages.in:76
> > +#if PLATFORM(QT) || PLATFORM(EFL) && USE(COORDINATED_GRAPHICS)
> 
> Maybe we should have a different define?
> 
We already have so many flags, I would hate to introduce a new one.
I think USE(COORDINATED_GRAPHICS) is turned on by default in Qt port, so I will try to keep only that and see if something breaks.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:281
> > +static Evas_Coord_Point adjustedPointForScaling(Evas_Coord_Point point, Ewk_View_Private_Data* priv)
> 
> Can't we follow Qt naming?
> 
> QPointF QQuickWebView::mapToWebContent(const QPointF& pointInViewCoordinates) const
> QRectF QQuickWebView::mapRectToWebContent(const QRectF& rectInViewCoordinates) const
> 
ok

> Also I would add priv before point as it is a hard requirement
ok

[Andras Becsi]

Comment on attachment 169748 [details]
Patch

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

The general direction looks good. Also let me know if you think something is missing from the client API.

> Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:100
> +        ViewportUpdateDeferrer deferrer(m_pageViewportController);

I'm in the process of removing the ViewportUpdateDeferrer because with the current implementation of PageViewportContrloller its purpose for Qt is not present any more. This code should be changed not to need the deferrer either.

[Jocelyn Turcotte]

Comment on attachment 169748 [details]
Patch

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

>>> Source/WebKit2/UIProcess/WebPageProxy.messages.in:76
>>> +#if PLATFORM(QT) || PLATFORM(EFL) && USE(COORDINATED_GRAPHICS)
>> 
>> Maybe we should have a different define?
> 
> We already have so many flags, I would hate to introduce a new one.
> I think USE(COORDINATED_GRAPHICS) is turned on by default in Qt port, so I will try to keep only that and see if something breaks.

This is not tied to COORDINATED_GRAPHICS in any way except for EFL, I would rather put it under USE(TILED_BACKING_STORE) in which case you also wouldn't have to restrict it to PLATFORM(EFL).
This however shows that we should probably rename/detach this define at some point, what it currently means is basically that we're doing UI process scrolling.

> Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:91
>      FloatRect mapRectToWebContent = m_visibleContentRect;
>      mapRectToWebContent.scale(1 / m_scaleFactor);
>      drawingArea()->setVisibleContentsRect(enclosingIntRect(mapRectToWebContent), m_scaleFactor, trajectory);

This suggests that setVisibleContentsRect is passed a rect in Viewport coordinates (zoom applied), but you now use m_visibleContentRect everywhere directly to pass coordinates to PageViewportController which expects page coordinates. Something is fishy, make sure that you test this code properly by panning and navigating pages while the page is zoomed.

> Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:109
> +    setVisibleContentsRect(IntPoint(contentsPoint.x(), contentsPoint.y()), m_scaleFactor, FloatPoint());
> +    m_pageViewportController->didChangeContentsVisibility(contentsPoint, m_scaleFactor, FloatPoint());

Both of them are calling drawingArea()->setVisibleContentsRect, this is wrong, only the viewportcontroller should be dealing with it.

> Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:117
> +    drawingArea()->layerTreeCoordinatorProxy()->setContentsSize(WebCore::FloatSize(m_contentsSize.width(), m_contentsSize.height()));

What is the goal of that call here?

> Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:122
> +    m_pageViewportController->didChangeContentsVisibility(m_visibleContentRect.location(), m_scaleFactor);

Viewport coordinates or page coordinates here?

> Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:128
> +    IntRect rect = IntRect(IntPoint(), m_visibleContentRect.size());
> +    ewk_view_display(m_viewWidget, rect);

I would use m_viewportSize directly instead of m_visibleContentRect.size().

[Yael]

(In reply to comment #7)
> (From update of attachment 169748 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169748&action=review
> 
Thanks for your review :)
> >>> Source/WebKit2/UIProcess/WebPageProxy.messages.in:76
> >>> +#if PLATFORM(QT) || PLATFORM(EFL) && USE(COORDINATED_GRAPHICS)
> >> 
> >> Maybe we should have a different define?
> > 
> > We already have so many flags, I would hate to introduce a new one.
> > I think USE(COORDINATED_GRAPHICS) is turned on by default in Qt port, so I will try to keep only that and see if something breaks.
> 
> This is not tied to COORDINATED_GRAPHICS in any way except for EFL, I would rather put it under USE(TILED_BACKING_STORE) in which case you also wouldn't have to restrict it to PLATFORM(EFL).
> This however shows that we should probably rename/detach this define at some point, what it currently means is basically that we're doing UI process scrolling.
> 
Doesn't gtk port use USE(TILED_BACKING_STORE) also? I would not want to break anything for them.

> > Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:91
> >      FloatRect mapRectToWebContent = m_visibleContentRect;
> >      mapRectToWebContent.scale(1 / m_scaleFactor);
> >      drawingArea()->setVisibleContentsRect(enclosingIntRect(mapRectToWebContent), m_scaleFactor, trajectory);
> 
> This suggests that setVisibleContentsRect is passed a rect in Viewport coordinates (zoom applied), but you now use m_visibleContentRect everywhere directly to pass coordinates to PageViewportController which expects page coordinates. Something is fishy, make sure that you test this code properly by panning and navigating pages while the page is zoomed.
> 
I'd like to change m_visibleContentRect so it keeps the scaled size, and not the actual viewport size, as it does now. Once I do that' I think this will look better. BTW, with COORDINATED_GRAPHICS turned on, almost every web page is zoomed :)

> > Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:109
> > +    setVisibleContentsRect(IntPoint(contentsPoint.x(), contentsPoint.y()), m_scaleFactor, FloatPoint());
> > +    m_pageViewportController->didChangeContentsVisibility(contentsPoint, m_scaleFactor, FloatPoint());
> 
> Both of them are calling drawingArea()->setVisibleContentsRect, this is wrong, only the viewportcontroller should be dealing with it.
> 
Yes, it is redundant, and I'll remove it.

> > Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:117
> > +    drawingArea()->layerTreeCoordinatorProxy()->setContentsSize(WebCore::FloatSize(m_contentsSize.width(), m_contentsSize.height()));
> 
> What is the goal of that call here?
> 
It was there before, and I should have removed it. It will be gone in the next iteration.

> > Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:122
> > +    m_pageViewportController->didChangeContentsVisibility(m_visibleContentRect.location(), m_scaleFactor);
> 
> Viewport coordinates or page coordinates here?
> 
There is some confusion about the coordinates of m_visibleContentRect, and I'd like to change it so it is always in page coordinates.

> > Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:128
> > +    IntRect rect = IntRect(IntPoint(), m_visibleContentRect.size());
> > +    ewk_view_display(m_viewWidget, rect);
> 
> I would use m_viewportSize directly instead of m_visibleContentRect.size().
ok

[Yael]

(In reply to comment #6)
Thanks for your comments :)
> (From update of attachment 169748 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169748&action=review
> 
> The general direction looks good. Also let me know if you think something is missing from the client API.
> 
> > Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:100
> > +        ViewportUpdateDeferrer deferrer(m_pageViewportController);
> 
> I'm in the process of removing the ViewportUpdateDeferrer because with the current implementation of PageViewportContrloller its purpose for Qt is not present any more. This code should be changed not to need the deferrer either.
Thanks for letting me know about removing the deferrer class.
I only added it because it triggers setting some flags in PageViewController. Once you remove the class, I'll adjust my implementation.

[Jocelyn Turcotte]

(In reply to comment #8)
> Doesn't gtk port use USE(TILED_BACKING_STORE) also? I would not want to break anything for them.

By judging at who are implementing the pageDidRequestScroll pure virtual in PageClient, only EFL and Qt in WebKit2.

[Yael]

Created attachment 169940 [details]
Patch

Address comment #3 and comment #7.
With this patch mouse events work as expected. I am not able to test touch events right now, so I added a FIXME to re-visit touch events later. 
There is no impact when WTF_USE_TILED_BACKING_STORE is turned off.

[Kenneth Rohde Christiansen]

Comment on attachment 169940 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1562
> +#if USE(COORDINATED_GRAPHICS)
> +void ewk_view_load_committed(Evas_Object* ewkView)

private method? We are trying to move away from free functions for private api

[Yael]

(In reply to comment #12)
> (From update of attachment 169940 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169940&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1562
> > +#if USE(COORDINATED_GRAPHICS)
> > +void ewk_view_load_committed(Evas_Object* ewkView)
> 
> private method? We are trying to move away from free functions for private api
We should convert the load client to C++ in one go, and I will be happy to do that as a follow-up to this patch, but I don't think we should do it one method at a time. Would that be ok?

[Yael]

Created https://bugs.webkit.org/show_bug.cgi?id=100035 for converting the loader client to C++.

[Jocelyn Turcotte]

Comment on attachment 169940 [details]
Patch

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

> Source/WebKit2/UIProcess/PageViewportController.h:26
> +#if USE(COORDINATED_GRAPHICS)
> +

Why not change all of them to USE(TILED_BACKING_STORE)?
COORDINATED_GRAPHICS is about rendering while most of this patch is about viewport and event handling.

> Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:-86
> -    // Move visibleContentRect inside contentsRect when visibleContentRect goes outside contentsRect. 

Are you sure you don't need that code anymore?

> Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:86
> +    m_scrollPosition = newScrollPosition;

Shouldn't didChangeContentsVisibility get called here too?

> Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:94
> +    ViewportUpdateDeferrer deferrer(m_pageViewportController);

Is there a reason why you need a deferrer here?

> Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:110
> +    drawingArea()->layerTreeCoordinatorProxy()->setContentsSize(WebCore::FloatSize(m_contentsSize.width(), m_contentsSize.height()));

You forgot to remove this...

> Source/WebKit2/UIProcess/efl/WebPageProxyEfl.cpp:107
> +#if USE(TILED_BACKING_STORE) 
> +void WebPageProxy::didRenderFrame(const WebCore::IntSize& contentsSize, const WebCore::IntRect& coveredRect)
> +{
> +    m_pageClient->didRenderFrame(contentsSize, coveredRect);
> +}
> +
> +void WebPageProxy::pageTransitionViewportReady()
> +{
> +    m_pageClient->pageTransitionViewportReady();
> +}
> +#endif
> +

I would prefer that you remove that code from WebPageProxyQt.cpp and move it to WebPageProxy.cpp so that we share it instead of duplicating it.

> Source/WebKit2/UIProcess/WebPageProxy.h:861
> +#if USE(TILED_BACKING_STORE) 
> +    void pageTransitionViewportReady();
> +#endif

There is another USE(TILED_BACKING_STORE) block 2 lines above... please use it.

[Chris Dumez]

(In reply to comment #12)
> (From update of attachment 169940 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169940&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1562
> > +#if USE(COORDINATED_GRAPHICS)
> > +void ewk_view_load_committed(Evas_Object* ewkView)
> 
> private method? We are trying to move away from free functions for private api

I think this is OK to land this for now as we haven't ported Ewk_View to C++ style yet. We still need to give it some thought since Ewk_View is an Evas_Object.

[Yael]

(In reply to comment #15)
> (From update of attachment 169940 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169940&action=review
> 
Thanks for continuing to review this :)
> > Source/WebKit2/UIProcess/PageViewportController.h:26
> > +#if USE(COORDINATED_GRAPHICS)
> > +
> 
> Why not change all of them to USE(TILED_BACKING_STORE)?
> COORDINATED_GRAPHICS is about rendering while most of this patch is about viewport and event handling.
> 
Thanks for the explanation, I was very confused about the flags.
> > Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:-86
> > -    // Move visibleContentRect inside contentsRect when visibleContentRect goes outside contentsRect. 
> 
> Are you sure you don't need that code anymore?
> 
WebCore already handles this. Once we add support for gestures and scroll animation, we might need to bring some logic back, Right now it is not needed. The UI side never sees negative coordinated when running script like window.scrollTo(-100, -100), or when trying to over-scroll.

> > Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:86
> > +    m_scrollPosition = newScrollPosition;
> 
> Shouldn't didChangeContentsVisibility get called here too?
> 
will fix in next version
> > Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:94
> > +    ViewportUpdateDeferrer deferrer(m_pageViewportController);
> 
> Is there a reason why you need a deferrer here?
> 
Removing it does not seem to have any effect. Will remove in next version.
> > Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:110
> > +    drawingArea()->layerTreeCoordinatorProxy()->setContentsSize(WebCore::FloatSize(m_contentsSize.width(), m_contentsSize.height()));
> 
> You forgot to remove this...
>
Oops... 
> > Source/WebKit2/UIProcess/efl/WebPageProxyEfl.cpp:107
> > +#if USE(TILED_BACKING_STORE) 
> > +void WebPageProxy::didRenderFrame(const WebCore::IntSize& contentsSize, const WebCore::IntRect& coveredRect)
> > +{
> > +    m_pageClient->didRenderFrame(contentsSize, coveredRect);
> > +}
> > +
> > +void WebPageProxy::pageTransitionViewportReady()
> > +{
> > +    m_pageClient->pageTransitionViewportReady();
> > +}
> > +#endif
> > +
> 
> I would prefer that you remove that code from WebPageProxyQt.cpp and move it to WebPageProxy.cpp so that we share it instead of duplicating it.
> 
ok

> > Source/WebKit2/UIProcess/WebPageProxy.h:861
> > +#if USE(TILED_BACKING_STORE) 
> > +    void pageTransitionViewportReady();
> > +#endif
> 
> There is another USE(TILED_BACKING_STORE) block 2 lines above... please use it.
ok

[Yael]

Created attachment 170164 [details]
Patch

Updates based on comment #15.

[WebKit Review Bot]

Attachment 170164 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebKit2/UIProcess/efl/PageLoadClientEfl.h:59:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yael]

Created attachment 170170 [details]
Patch

Fix style

[WebKit Review Bot]

Comment on attachment 170170 [details]
Patch

Clearing flags on attachment: 170170

Committed r132228: <http://trac.webkit.org/changeset/132228>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.