[webkit-reviews] review denied: [Bug 99850] [EFL][WK2] Use the port independent PageViewportController : [Attachment 169940] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Oct 23 03:45:01 PDT 2012


Jocelyn Turcotte <jocelyn.turcotte at digia.com> has denied Yael
<yael.aharon.m at gmail.com>'s request for review:
Bug 99850: [EFL][WK2] Use the port independent PageViewportController
https://bugs.webkit.org/show_bug.cgi?id=99850

Attachment 169940: Patch
https://bugs.webkit.org/attachment.cgi?id=169940&action=review

------- Additional Comments from Jocelyn Turcotte <jocelyn.turcotte at digia.com>
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.


More information about the webkit-reviews mailing list