[Webkit-unassigned] [Bug 99850] [EFL][WK2] Use the port independent PageViewportController

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=99850





--- Comment #8 from Yael <yael.aharon.m at gmail.com>  2012-10-22 08:09:39 PST ---
(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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.