Bug 89840 – [WK2][EFL] Implement accelerated compositing on WK2 Efl port

• Component: WebKit EFL
• Status: RESOLVED FIXED

[YoungTaeck Song]

Implement accelerated composition with TiledBackingStore on WK2 Efl port.

[YoungTaeck Song]

Created attachment 149213 [details]
patch

[Raphael Kubo da Costa (:rakuco)]

What a huge patch. It'd be good if Igor could take a look at the AC-specific parts. As for the rest, you shouldn't need to add those new files to the build system conditionally, as their code should already be protected by #ifdefs. Plus, isn't it possible to make all this conditional and not require OpenGL and the rest?

[Gyuyoung Kim]

Comment on attachment 149213 [details]
patch

Attachment 149213 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13029940

[Ryuan Choi]

Comment on attachment 149213 [details]
patch

It depends other bugs.
So clear the flags.

[Gyuyoung Kim]

Though it seems this patch is to support accelerated compositing, this patch touches many files. I think this patch is able to be separated into more smaller patches in order to be reviewed by us. If possible, it would good to file a meta bug for this feature.

[Ryuan Choi]

Comment on attachment 149213 [details]
patch

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.

[Gyuyoung Kim]

Comment on attachment 149213 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149213&action=review

Set r- because of mentioned comments.

> Source/WebKit2/UIProcess/API/efl/ViewportProcessor.cpp:33
> +    , m_scaleFactor(1)

Is 1.0 correct ?

> Source/cmake/FindOpenGL.cmake:41
> +  IF (APPLE)

Is this correct macro ? I didn't see (APPLE) macro in CMake before.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 149213 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149213&action=review

>> Source/cmake/FindOpenGL.cmake:41
>> +  IF (APPLE)
> 
> Is this correct macro ? I didn't see (APPLE) macro in CMake before.

Yes, it's defined by CMake itself. See `cmake --help-variables' or cmakevars(1).

[YoungTaeck Song]

Created attachment 152931 [details]
patch

[YoungTaeck Song]

(In reply to comment #2)
> What a huge patch. It'd be good if Igor could take a look at the AC-specific parts. As for the rest, you shouldn't need to add those new files to the build system conditionally, as their code should already be protected by #ifdefs. Plus, isn't it possible to make all this conditional and not require OpenGL and the rest?

I'm Sorry about a huge patch. 
I splited this patch into 6 patches.
New patch 152931 is still big.
But Most are definitions or cmake work.
And removed conditional code in cmake files.

[YoungTaeck Song]

(In reply to comment #5)
> Though it seems this patch is to support accelerated compositing, this patch touches many files. I think this patch is able to be separated into more smaller patches in order to be reviewed by us. If possible, it would good to file a meta bug for this feature.

I'm Sorry about a huge patch. 
I splited this patch into 6 patches.

[YoungTaeck Song]

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

[YoungTaeck Song]

(In reply to comment #7)

Thank you for kind review.

> (From update of attachment 149213 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149213&action=review
> 
> Set r- because of mentioned comments.
> 
> > Source/WebKit2/UIProcess/API/efl/ViewportProcessor.cpp:33
> > +    , m_scaleFactor(1)
> 
> Is 1.0 correct ?
> 

Yes, it is.

> > Source/cmake/FindOpenGL.cmake:41
> > +  IF (APPLE)
> 
> Is this correct macro ? I didn't see (APPLE) macro in CMake before.
I removed this file.

[Gyuyoung Kim]

Comment on attachment 152931 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152931&action=review

> Source/WebKit2/PlatformEfl.cmake:51
> +    UIProcess/API/efl/ViewportProcessor.cpp

Nit: Upper-case file has placed above lower-case file.

> Source/WebKit2/PlatformEfl.cmake:145
> +    ${OPENGL_LIBRARIES}

Nit : It looks OPENGL_LIBRARIES needs to be moved to above SQLITE_LIBRARIES for alphabetical order.

> Source/WebKit2/PlatformEfl.cmake:161
> +    ${OPENGL_LIBRARIES}

ditto.

> Source/WebKit2/UIProcess/API/efl/PageClientImpl.cpp:54
> +    page()->pageGroup()->preferences()->setAcceleratedDrawingEnabled(true);

Is it better to use m_page instead of page() ? Is there any reason ?

> Source/WebKit2/UIProcess/API/efl/PageClientImpl.cpp:296
> +#if USE(UI_SIDE_COMPOSITING)

Why do you use this macro function inside? Is this public API? I think we have to use macro function outside except for public APIs.

> Source/WebKit2/UIProcess/API/efl/PageClientImpl.h:109
> +    virtual void didChangeContentsSize(const WebCore::IntSize&);

Don't you need to use USE(UI_SIDE_COMPOSITING) as well ?

> Source/WebKit2/UIProcess/API/efl/ViewportProcessor.cpp:67
> +    // adjust VisibleContentRect

Nit : s/adjust/Adjust/g

> Source/WebKit2/UIProcess/API/efl/ViewportProcessor.h:47
> +    ViewportProcessor(PageClientImpl*);

Use *explicit* keyword.

> Source/WebKit2/UIProcess/API/efl/ViewportProcessor.h:51
> +    float m_scaleFactor;

Nit : I think it is better to move to below m_viewportSize because of gathering similar type variables.

> Tools/MiniBrowser/efl/CMakeLists.txt:33
> +    ${OPENGL_LIBRARIES}

ditto.

> Tools/WebKitTestRunner/PlatformEfl.cmake:46
> +    ${OPENGL_LIBRARIES}

ditto.

[YoungTaeck Song]

Created attachment 153162 [details]
Patch

[YoungTaeck Song]

(In reply to comment #14)

Thanks for your review.

> (From update of attachment 152931 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152931&action=review
> 
> > Source/WebKit2/PlatformEfl.cmake:51
> > +    UIProcess/API/efl/ViewportProcessor.cpp
> 
> Nit: Upper-case file has placed above lower-case file.
> 

fixed.

> > Source/WebKit2/PlatformEfl.cmake:145
> > +    ${OPENGL_LIBRARIES}
> 
> Nit : It looks OPENGL_LIBRARIES needs to be moved to above SQLITE_LIBRARIES for alphabetical order.
> 
> > Source/WebKit2/PlatformEfl.cmake:161
> > +    ${OPENGL_LIBRARIES}
> 
> ditto.
> 

fixed.

> > Source/WebKit2/UIProcess/API/efl/PageClientImpl.cpp:54
> > +    page()->pageGroup()->preferences()->setAcceleratedDrawingEnabled(true);
> 
> Is it better to use m_page instead of page() ? Is there any reason ?
> 

fixed.

> > Source/WebKit2/UIProcess/API/efl/PageClientImpl.cpp:296
> > +#if USE(UI_SIDE_COMPOSITING)
> 
> Why do you use this macro function inside? Is this public API? I think we have to use macro function outside except for public APIs.
> 
> > Source/WebKit2/UIProcess/API/efl/PageClientImpl.h:109
> > +    virtual void didChangeContentsSize(const WebCore::IntSize&);
> 
> Don't you need to use USE(UI_SIDE_COMPOSITING) as well ?
> 

didChangeContentsSize is port specific and not just for USE(UI_SIDE_COMPOSITING). 
We shared Qt's didChangeContentsSize, and Qt's didChangeContentsSize has both  UI_SIDE_COMPOSITING and port specific.
So I removed  USE(UI_SIDE_COMPOSITING) at PageClientImpl.cpp:296 and outside ewk_view_contents_size_changed.

> > Source/WebKit2/UIProcess/API/efl/ViewportProcessor.cpp:67
> > +    // adjust VisibleContentRect
> 
> Nit : s/adjust/Adjust/g
> 

fixed.

> > Source/WebKit2/UIProcess/API/efl/ViewportProcessor.h:47
> > +    ViewportProcessor(PageClientImpl*);
> 
> Use *explicit* keyword.
> 

fixed.

> > Source/WebKit2/UIProcess/API/efl/ViewportProcessor.h:51
> > +    float m_scaleFactor;
> 
> Nit : I think it is better to move to below m_viewportSize because of gathering similar type variables.

fixed.

> 
> > Tools/MiniBrowser/efl/CMakeLists.txt:33
> > +    ${OPENGL_LIBRARIES}
> 
> ditto.
> 
> > Tools/WebKitTestRunner/PlatformEfl.cmake:46
> > +    ${OPENGL_LIBRARIES}
> 
> ditto.

fixed.

[Simon Hausmann]

Comment on attachment 153162 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153162&action=review

> Source/WebKit2/Shared/WebCoreArgumentCoders.h:70
> +#if PLATFORM(QT) || PLATFORM(EFL)

It looks like the en/de-coders for types like FloatPoint3D that this section forward-declares are wrapped in #if USE(UI_SIDE_COMPOSITING), so perhaps
this #if PLATFORM(QT) should also be changed to use UI_SIDE_COMPOSITING instead?

[YoungTaeck Song]

Created attachment 153234 [details]
Patch

[YoungTaeck Song]

(In reply to comment #17)
> (From update of attachment 153162 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153162&action=review
> 
> > Source/WebKit2/Shared/WebCoreArgumentCoders.h:70
> > +#if PLATFORM(QT) || PLATFORM(EFL)
> 
> It looks like the en/de-coders for types like FloatPoint3D that this section forward-declares are wrapped in #if USE(UI_SIDE_COMPOSITING), so perhaps
> this #if PLATFORM(QT) should also be changed to use UI_SIDE_COMPOSITING instead?

Thank you for kind review.
I changed PLATFORM(QT) to UI_SIDE_COMPOSITING.
And removed some classes that don't be used.

[Kenneth Rohde Christiansen]

Comment on attachment 153234 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153234&action=review

> Source/WebKit2/UIProcess/API/efl/ViewportProcessor.cpp:33
> +ViewportProcessor::ViewportProcessor(PageClientImpl* pageClientImpl)

I don't like the name :-) plus it seems to be slightly related to the QtViewportHandler which though does much more. Anyway there are plans to make the QtViewportHandler cross platform

[Kenneth Rohde Christiansen]

Noam can you please look at this?

[Noam Rosenthal]

Comment on attachment 153234 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153234&action=review

the changes to UI-side compositing look good to me. If this was peer reviewed you can have my r+ (minus the nitpicks), or kenneth can r+ it :)

> Source/WebKit2/UIProcess/API/efl/PageClientImpl.cpp:54
> +    m_page->pageGroup()->preferences()->setAcceleratedDrawingEnabled(true);

I don't think this is right.

[YoungTaeck Song]

Created attachment 153940 [details]
Patch

[YoungTaeck Song]

(In reply to comment #20)
> (From update of attachment 153234 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153234&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ViewportProcessor.cpp:33
> > +ViewportProcessor::ViewportProcessor(PageClientImpl* pageClientImpl)
> 
> I don't like the name :-) plus it seems to be slightly related to the QtViewportHandler which though does much more. Anyway there are plans to make the QtViewportHandler cross platform

Thank you for kind review.
And sorry too late.

I changed this class name to EflViewportProcessor.
About making the QtViewportHandler cross platform, Almost are filled with Qt specific code and Qt related codes in QtViewportHandler.cpp.
So I think it is good to do not extracting common code from QtViewportHandler.

[YoungTaeck Song]

(In reply to comment #22)
> (From update of attachment 153234 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153234&action=review
> 
> the changes to UI-side compositing look good to me. If this was peer reviewed you can have my r+ (minus the nitpicks), or kenneth can r+ it :)
> 
> > Source/WebKit2/UIProcess/API/efl/PageClientImpl.cpp:54
> > +    m_page->pageGroup()->preferences()->setAcceleratedDrawingEnabled(true);
> 
> I don't think this is right.

Thank you for kind review.
And sorry too late.

I removed this.

[Gyuyoung Kim]

Comment on attachment 153940 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153940&action=review

> Source/WebKit2/UIProcess/API/efl/EflViewportHandler.cpp:18
> + */

It looks UIProcess/API/efl/ has used Apple BSD license.

> Source/WebKit2/UIProcess/API/efl/EflViewportHandler.h:4
> +    This library is free software; you can redistribute it and/or

ditto.

[YoungTaeck Song]

(In reply to comment #26)
> (From update of attachment 153940 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153940&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EflViewportHandler.cpp:18
> > + */
> 
> It looks UIProcess/API/efl/ has used Apple BSD license.
We decided that use this licence to new file.
Please see ewk_view.cpp.

> 
> > Source/WebKit2/UIProcess/API/efl/EflViewportHandler.h:4
> > +    This library is free software; you can redistribute it and/or
> 
> ditto.
ditto.

[YoungTaeck Song]

Created attachment 155194 [details]
Patch

[Noam Rosenthal]

Comment on attachment 155194 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155194&action=review

> Source/WebKit2/UIProcess/API/efl/EflViewportHandler.cpp:83
> +    if (m_visibleContentRect.x() > contentsSize.width() - m_visibleContentRect.width())
> +        m_visibleContentRect.setX(contentsSize.width() - m_visibleContentRect.width());
> +    if (m_visibleContentRect.x() < 0)
> +        m_visibleContentRect.setX(0);
> +    if (m_visibleContentRect.y() > contentsSize.height() - m_visibleContentRect.height())
> +        m_visibleContentRect.setY(contentsSize.height() - m_visibleContentRect.height());
> +    if (m_visibleContentRect.y() < 0)
> +        m_visibleContentRect.setY(0);

This code is hard to read. Is this an intersect? Or is this a copy from some Qt code?
If it's an intersection, please use IntRect::intersect. If not, please add some comments to clarify.

[YoungTaeck Song]

(In reply to comment #29)
> (From update of attachment 155194 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155194&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EflViewportHandler.cpp:83
> > +    if (m_visibleContentRect.x() > contentsSize.width() - m_visibleContentRect.width())
> > +        m_visibleContentRect.setX(contentsSize.width() - m_visibleContentRect.width());
> > +    if (m_visibleContentRect.x() < 0)
> > +        m_visibleContentRect.setX(0);
> > +    if (m_visibleContentRect.y() > contentsSize.height() - m_visibleContentRect.height())
> > +        m_visibleContentRect.setY(contentsSize.height() - m_visibleContentRect.height());
> > +    if (m_visibleContentRect.y() < 0)
> > +        m_visibleContentRect.setY(0);
> 
> This code is hard to read. Is this an intersect? Or is this a copy from some Qt code?
> If it's an intersection, please use IntRect::intersect. If not, please add some comments to clarify.

Thanks for kind review.
And I'm very sorry too late.

This code is for moving visibleContentRect inside content rect when visibleContentRect is out of the content rect.
Efl's webview doesn't know contents size, so We have to use this code.

I'll use more clear comments at next patch.

[Gyuyoung Kim]

Comment on attachment 155194 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155194&action=review

> Source/WebKit2/UIProcess/API/efl/EflViewportHandler.cpp:58
> +    renderer->paintToCurrentGLContext(matrix, 1.0f, rect);

Though there was a little argument in EFL port, webkit coding style guides that floating point literals aren't appeneded .0, .f and .0f to .

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1328
> +    EWK_VIEW_PRIV_GET(smartData, priv);

To prevent crash, I think you need to use "EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv)"

[YoungTaeck Song]

Created attachment 156606 [details]
Patch

[YoungTaeck Song]

(In reply to comment #31)
Thanks for kind review.
> (From update of attachment 155194 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155194&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EflViewportHandler.cpp:58
> > +    renderer->paintToCurrentGLContext(matrix, 1.0f, rect);
> 
> Though there was a little argument in EFL port, webkit coding style guides that floating point literals aren't appeneded .0, .f and .0f to .
fixed.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1328
> > +    EWK_VIEW_PRIV_GET(smartData, priv);
> 
> To prevent crash, I think you need to use "EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv)"
fixed.

[Gyuyoung Kim]

Comment on attachment 156606 [details]
Patch

Attachment 156606 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13435704

[YoungTaeck Song]

Created attachment 156639 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 156639 [details]
Patch

As a basic implementation for accelerated compositing for WK2, looks good to me. But, I think this patch needs to be reviewed by proper reviewers again. I think Kenneth and Noam are them.

[Noam Rosenthal]

Comment on attachment 156639 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156639&action=review

Looks good! Please fix Changelog wording before landing.

> Source/WebKit2/ChangeLog:9
> +        These codes are based on COORDINATED_GRAPHICS.

These codes -> This implementation

[YoungTaeck Song]

Created attachment 157093 [details]
Patch

[YoungTaeck Song]

(In reply to comment #37)
Thank you for everything you've done for me. :)

> (From update of attachment 156639 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156639&action=review
> 
> Looks good! Please fix Changelog wording before landing.
> 
> > Source/WebKit2/ChangeLog:9
> > +        These codes are based on COORDINATED_GRAPHICS.
> 
> These codes -> This implementation
fixed.

[Noam Rosenthal]

(In reply to comment #39)
> (In reply to comment #37)
> Thank you for everything you've done for me. :)
Thanks for considering to use coordinated graphics and making it work for EFL!
The more code we share the better, and if what it takes is a couple of reviews I'm fine with it...

[WebKit Review Bot]

Comment on attachment 157093 [details]
Patch

Clearing flags on attachment: 157093

Committed r124989: <http://trac.webkit.org/changeset/124989>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[YoungTaeck Song]

(In reply to comment #40)
> (In reply to comment #39)
> > (In reply to comment #37)
> > Thank you for everything you've done for me. :)
> Thanks for considering to use coordinated graphics and making it work for EFL!
> The more code we share the better, and if what it takes is a couple of reviews I'm fine with it...

It is a pleasure and honor for me to use your great work!
This patch is first step of WK2 EFL COORDINATED_GRAPHICS.
I'll advance this feature step by step.

And I'll request your review about new patches after I accept r+ from Efl committers, :)

Thank you again!

[Kenneth Rohde Christiansen]

(In reply to comment #43)
> (In reply to comment #40)
> > (In reply to comment #39)
> > > (In reply to comment #37)
> > > Thank you for everything you've done for me. :)
> > Thanks for considering to use coordinated graphics and making it work for EFL!
> > The more code we share the better, and if what it takes is a couple of reviews I'm fine with it...
> 
> It is a pleasure and honor for me to use your great work!
> This patch is first step of WK2 EFL COORDINATED_GRAPHICS.
> I'll advance this feature step by step.
> 
> And I'll request your review about new patches after I accept r+ from Efl committers, :)
> 
> Thank you again!

Did you see the nice documentation? http://trac.webkit.org/wiki/CoordinatedGraphicsSystem

Kenneth

[YoungTaeck Song]

(In reply to comment #44)
> (In reply to comment #43)
> > (In reply to comment #40)
> > > (In reply to comment #39)
> > > > (In reply to comment #37)
> > > > Thank you for everything you've done for me. :)
> > > Thanks for considering to use coordinated graphics and making it work for EFL!
> > > The more code we share the better, and if what it takes is a couple of reviews I'm fine with it...
> > 
> > It is a pleasure and honor for me to use your great work!
> > This patch is first step of WK2 EFL COORDINATED_GRAPHICS.
> > I'll advance this feature step by step.
> > 
> > And I'll request your review about new patches after I accept r+ from Efl committers, :)
> > 
> > Thank you again!
> 
> Did you see the nice documentation? http://trac.webkit.org/wiki/CoordinatedGraphicsSystem
> 
> Kenneth

Yes! I already saw it.
Thanks for the great documentation.
My co-workers also read with effect.
If we have some questions or suggestions, we will attach this documentation soon.