[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 #12 from YoungTaeck Song <youngtaeck.song at samsung.com>  2012-07-17 23:38:18 PST ---
(In reply to comment #6)
Thank you for kind review.

> (From update of attachment 149213 [details])
> 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 ?

removed.

> 
> > 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

fixed.

> 
> > 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.
> 

fixed.

> > 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?
> 

I followed Qt ports. But I removed this code in patch 152931.

> > 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?
> 

Qt port is using this code for UI_SIDDE_COMPOSITING and platform specific purpose.
Efl is just using for UI_SIDDE_COMPOSITING.

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

fixed.

> 
> > 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?

Thank. I didn't know that cmake has FindOpenGl.cmake as default.
removed this code.

> 
> > 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.
> 

fixed

> > Source/cmake/OptionsEfl.cmake:174
> > +  FIND_PACKAGE(OpenGL REQUIRED)
> 
> Indeed, I am not sure whether we should have opengl dependency as a default.

I moved this code inside WTF_USE_UI_SIDE_COMPOSITING.
> 
> > 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.

I added a MiniBrowser's option for evas engine.

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