Bug 89840 - [WK2][EFL] Implement accelerated compositing on WK2 Efl port
Summary: [WK2][EFL] Implement accelerated compositing on WK2 Efl port
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 89837 89838 89839
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-24 17:28 PDT by YoungTaeck Song
Modified: 2012-08-08 17:34 PDT (History)
12 users (show)

See Also:


Attachments
patch (48.38 KB, patch)
2012-06-24 17:30 PDT, YoungTaeck Song
gyuyoung.kim: review-
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
patch (28.38 KB, patch)
2012-07-17 22:46 PDT, YoungTaeck Song
no flags Details | Formatted Diff | Diff
Patch (30.12 KB, patch)
2012-07-18 19:40 PDT, YoungTaeck Song
no flags Details | Formatted Diff | Diff
Patch (30.74 KB, patch)
2012-07-19 05:34 PDT, YoungTaeck Song
no flags Details | Formatted Diff | Diff
Patch (30.79 KB, patch)
2012-07-23 19:39 PDT, YoungTaeck Song
no flags Details | Formatted Diff | Diff
Patch (31.79 KB, patch)
2012-07-29 18:52 PDT, YoungTaeck Song
no flags Details | Formatted Diff | Diff
Patch (30.76 KB, patch)
2012-08-05 23:55 PDT, YoungTaeck Song
no flags Details | Formatted Diff | Diff
Patch (32.88 KB, patch)
2012-08-06 02:47 PDT, YoungTaeck Song
no flags Details | Formatted Diff | Diff
Patch (32.72 KB, patch)
2012-08-07 20:06 PDT, YoungTaeck Song
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description YoungTaeck Song 2012-06-24 17:28:56 PDT
Implement accelerated composition with TiledBackingStore on WK2 Efl port.
Comment 1 YoungTaeck Song 2012-06-24 17:30:05 PDT
Created attachment 149213 [details]
patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-06-24 17:54:54 PDT
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?
Comment 3 Gyuyoung Kim 2012-06-24 18:07:24 PDT
Comment on attachment 149213 [details]
patch

Attachment 149213 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13029940
Comment 4 Ryuan Choi 2012-06-24 18:49:42 PDT
Comment on attachment 149213 [details]
patch

It depends other bugs.
So clear the flags.
Comment 5 Gyuyoung Kim 2012-06-26 18:28:35 PDT
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.
Comment 6 Ryuan Choi 2012-06-27 16:54:12 PDT
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.
Comment 7 Gyuyoung Kim 2012-07-01 19:15:20 PDT
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.
Comment 8 Raphael Kubo da Costa (:rakuco) 2012-07-02 06:58:27 PDT
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).
Comment 9 YoungTaeck Song 2012-07-17 22:46:53 PDT
Created attachment 152931 [details]
patch
Comment 10 YoungTaeck Song 2012-07-17 23:27:45 PDT
(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.
Comment 11 YoungTaeck Song 2012-07-17 23:28:35 PDT
(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.
Comment 12 YoungTaeck Song 2012-07-17 23:38:18 PDT
(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.
Comment 13 YoungTaeck Song 2012-07-17 23:41:11 PDT
(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.
Comment 14 Gyuyoung Kim 2012-07-18 03:41:33 PDT
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.
Comment 15 YoungTaeck Song 2012-07-18 19:40:31 PDT
Created attachment 153162 [details]
Patch
Comment 16 YoungTaeck Song 2012-07-18 19:52:30 PDT
(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.
Comment 17 Simon Hausmann 2012-07-19 01:56:21 PDT
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?
Comment 18 YoungTaeck Song 2012-07-19 05:34:31 PDT
Created attachment 153234 [details]
Patch
Comment 19 YoungTaeck Song 2012-07-19 05:39:46 PDT
(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.
Comment 20 Kenneth Rohde Christiansen 2012-07-20 03:18:15 PDT
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
Comment 21 Kenneth Rohde Christiansen 2012-07-20 03:19:31 PDT
Noam can you please look at this?
Comment 22 Noam Rosenthal 2012-07-20 08:20:48 PDT
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.
Comment 23 YoungTaeck Song 2012-07-23 19:39:34 PDT
Created attachment 153940 [details]
Patch
Comment 24 YoungTaeck Song 2012-07-23 19:58:57 PDT
(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.
Comment 25 YoungTaeck Song 2012-07-23 19:59:43 PDT
(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.
Comment 26 Gyuyoung Kim 2012-07-23 20:30:31 PDT
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.
Comment 27 YoungTaeck Song 2012-07-23 23:11:17 PDT
(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.
Comment 28 YoungTaeck Song 2012-07-29 18:52:30 PDT
Created attachment 155194 [details]
Patch
Comment 29 Noam Rosenthal 2012-07-29 23:01:46 PDT
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.
Comment 30 YoungTaeck Song 2012-07-30 23:57:53 PDT
(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.
Comment 31 Gyuyoung Kim 2012-08-05 19:01:47 PDT
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)"
Comment 32 YoungTaeck Song 2012-08-05 23:55:07 PDT
Created attachment 156606 [details]
Patch
Comment 33 YoungTaeck Song 2012-08-06 00:06:11 PDT
(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.
Comment 34 Gyuyoung Kim 2012-08-06 00:11:45 PDT
Comment on attachment 156606 [details]
Patch

Attachment 156606 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13435704
Comment 35 YoungTaeck Song 2012-08-06 02:47:45 PDT
Created attachment 156639 [details]
Patch
Comment 36 Gyuyoung Kim 2012-08-06 18:38:36 PDT
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.
Comment 37 Noam Rosenthal 2012-08-07 07:25:16 PDT
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
Comment 38 YoungTaeck Song 2012-08-07 20:06:15 PDT
Created attachment 157093 [details]
Patch
Comment 39 YoungTaeck Song 2012-08-07 20:12:25 PDT
(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.
Comment 40 Noam Rosenthal 2012-08-07 20:23:24 PDT
(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...
Comment 41 WebKit Review Bot 2012-08-07 22:38:44 PDT
Comment on attachment 157093 [details]
Patch

Clearing flags on attachment: 157093

Committed r124989: <http://trac.webkit.org/changeset/124989>
Comment 42 WebKit Review Bot 2012-08-07 22:38:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 43 YoungTaeck Song 2012-08-07 23:01:15 PDT
(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!
Comment 44 Kenneth Rohde Christiansen 2012-08-08 05:28:08 PDT
(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
Comment 45 YoungTaeck Song 2012-08-08 17:34:11 PDT
(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.