[Webkit-unassigned] [Bug 89840] [WK2][EFL] Implement accelerated compositing on WK2 Efl port

[bugzilla-daemon at webkit.org]

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





--- Comment #6 from Ryuan Choi <ryuan.choi at samsung.com>  2012-06-27 16:54:12 PST ---
(From update of attachment 149213)
View in context: https://bugs.webkit.org/attachment.cgi?id=149213&action=review

I agree with gyuyoung. But I write some comment for this patch.

> Source/WebKit2/PlatformEfl.cmake:98
> +    "${WTF_DIR}/wtf"

IS it already in WebKit2/CMakeLists.txt ?

> Source/WebKit2/PlatformEfl.cmake:161
> +    LIST(APPEND WebKit2_INCLUDE_DIRECTORIES
> +        "${WEBCORE_DIR}/platform/graphics/surfaces"
> +        "${WEBCORE_DIR}/platform/graphics/texmap"
> +        "${WEBKIT2_DIR}/WebProcess/WebPage/web"
> +        "${WEBKIT2_DIR}/WebProcess/WebPage/web/efl"
> +        ${OPENGL_INCLUDE_DIR}
> +    )
> +    LIST (APPEND WebKit2_LIBRARIES
> +        ${OPENGL_LIBRARIES}
> +    )
> +    LIST (APPEND WebProcess_LIBRARIES
> +        ${OPENGL_LIBRARIES}
> +    )

Some of them looks not platform specific. so can we move them to CMakeLists.txt

> Source/WebKit2/UIProcess/API/efl/PageClientImpl.cpp:306
> +    ewk_view_did_change_contents_size(m_viewWidget, size);

I am bad with naming.
But WebKit/Efl is using XXX_changed such as ewk_view_load progress_changed.

> Source/WebKit2/UIProcess/API/efl/PageClientImpl.h:46
> -private:
> -    PageClientImpl(WebContext*, WebPageGroup*, Evas_Object*);
> -
>      // PageClient
>      virtual PassOwnPtr<DrawingAreaProxy> createDrawingAreaProxy();

Why do you move many APIs including createDrawingAreaProxy to public section?

> Source/WebKit2/UIProcess/PageClient.h:136
> +#if PLATFORM(QT) || PLATFORM(EFL)
> +    virtual void didChangeContentsSize(const WebCore::IntSize&) = 0;
> +#endif

Is this for UI_SIDDE_COMPOSITING or platform specific codes?

> Source/WebKit2/WebProcess/WebPage/web/efl/LayerTreeHostEfl.cpp:21
> +

Unnecessary empty line.

> Source/cmake/FindOpenGL.cmake:2
> +# - Try to find OpenGL
> +# Once done this will define

I am not sure, but at least, cmake 2.8.7 have FindOpenGl.cmake as default.

Should we really have this?

> Source/cmake/OptionsEfl.cmake:153
> +IF (ENABLE_WEBKIT2)
> +  SET(WTF_USE_TILED_BACKING_STORE 1)
> +  ADD_DEFINITIONS(-DWTF_USE_TILED_BACKING_STORE=1)

We should not have webkit2 specific options.

Now we build both webkit1 and webkit2 as a default.

> Source/cmake/OptionsEfl.cmake:174
> +  FIND_PACKAGE(OpenGL REQUIRED)

Indeed, I am not sure whether we should have opengl dependency as a default.

> Tools/MiniBrowser/efl/main.c:102
> -    app->ee = ecore_evas_new(0, 0, 0, DEFAULT_WIDTH, DEFAULT_HEIGHT, 0);
> +    app->ee = ecore_evas_new("opengl_x11", 0, 0, DEFAULT_WIDTH, DEFAULT_HEIGHT, 0);

We should not fix the engine to opengl_x11.

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