Bug 99850

Summary: [EFL][WK2] Use the port independent PageViewportController
Product: WebKit Reporter: Yael <yael>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, cdumez, cmarcelo, gyuyoung.kim, joone, jturcotte, kenneth, laszlo.gombos, lucas.de.marchi, menard, noam, rakuco, tmpsantos, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
gyuyoung.kim: commit-queue-
Patch
jturcotte: review-
Patch
none
Patch none

Yael
Reported 2012-10-19 10:11:30 PDT
WebKit EFL should start using PageViewportController to support viewport meta tag. A patch is coming soon.
Attachments
Patch (21.41 KB, patch)
2012-10-19 19:47 PDT, Yael
gyuyoung.kim: commit-queue-
Patch (25.90 KB, patch)
2012-10-22 10:50 PDT, Yael
jturcotte: review-
Patch (28.80 KB, patch)
2012-10-23 07:45 PDT, Yael
no flags
Patch (28.83 KB, patch)
2012-10-23 08:15 PDT, Yael
no flags
Yael
Comment 1 2012-10-19 19:47:28 PDT
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 2 2012-10-19 19:55:45 PDT
Kenneth Rohde Christiansen
Comment 3 2012-10-21 02:57:49 PDT
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
Comment 4 2012-10-21 02:58:44 PDT
Andras, Jocelyn, please have a look as well
Yael
Comment 5 2012-10-21 05:14:10 PDT
(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 6 2012-10-22 05:05:38 PDT
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 7 2012-10-22 06:05:30 PDT
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
Comment 8 2012-10-22 08:09:39 PDT
(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
Comment 9 2012-10-22 08:20:47 PDT
(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
Comment 10 2012-10-22 08:25:37 PDT
(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
Comment 11 2012-10-22 10:50:58 PDT
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 12 2012-10-22 12:41:10 PDT
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
Comment 13 2012-10-22 13:43:09 PDT
(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
Comment 14 2012-10-22 14:42:07 PDT
Created https://bugs.webkit.org/show_bug.cgi?id=100035 for converting the loader client to C++.
Jocelyn Turcotte
Comment 15 2012-10-23 03:46:06 PDT
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
Comment 16 2012-10-23 06:43:17 PDT
(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
Comment 17 2012-10-23 07:08:51 PDT
(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
Comment 18 2012-10-23 07:45:11 PDT
Created attachment 170164 [details] Patch Updates based on comment #15.
WebKit Review Bot
Comment 19 2012-10-23 07:47:26 PDT
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
Comment 20 2012-10-23 08:15:57 PDT
Created attachment 170170 [details] Patch Fix style
WebKit Review Bot
Comment 21 2012-10-23 08:44:34 PDT
Comment on attachment 170170 [details] Patch Clearing flags on attachment: 170170 Committed r132228: <http://trac.webkit.org/changeset/132228>
WebKit Review Bot
Comment 22 2012-10-23 08:44:40 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.