| Summary: | [WK2][EFL] Add an ACCELERATED_COMPOSITING implementation for Efl WebKit2 | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | YoungTaeck Song <youngtaeck.song> | ||||||||||||||||
| Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | cdumez, gyuyoung.kim, gyuyoung.kim, jaepark, lucas.de.marchi, noam, rakuco, sw0524.lee, webkit.review.bot, yoon | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Other | ||||||||||||||||||
| OS: | Linux | ||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||
| Bug Blocks: | 61838 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
Attachment 152921 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5]
Tools/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5]
Total errors found: 2 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 152925 [details]
patch
miss bug number
Comment on attachment 152925 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=152925&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:76 > ~_Ewk_View_Private_Data() You probably need Evas_GL clean up code in the destructor. I'm assuming the view can be destroyed without ewk_view_exit_accelerated_compositing_mode() being called beforehand. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:383 > +IntSize ewk_view_size_get(Evas_Object* ewkView) argument should be const. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:403 > + How about an ASSERT(!priv->evasGlSurface); to make sure we don't leak memory? > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:415 > +void ewk_view_enter_accelerated_compositing_mode(Evas_Object* ewkView) How about returning a bool so that the client know if it was successful? > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:421 > + priv->evasGl = evas_gl_new(evas); I think we need an EINA_SAFETY check to make sure priv->evasGl is NULL when this function is called. What if the client calls this function several times without calling the ewk_view_exit_accelerated_compositing_mode()? We should print a warning instead of leaking memory. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:441 > +void ewk_view_exit_accelerated_compositing_mode(Evas_Object* ewkView) How about returning a bool so that the client know if it was successful? > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:446 > + if (!priv->evasGl) Should it be an if check or an EINA_SAFETY? I guess this mean this means the client called ewk_view_exit_accelerated_compositing_mode() without calling ewk_view_enter_accelerated_compositing_mode() first, which would be wrong API usage. Also, if ewk_view_enter_accelerated_compositing_mode() starts returning a boolean, the client has not excuse. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:483 > + evas_gl_surface_destroy(priv->evasGl, priv->evasGlSurface); I would feel better with a "priv->evasGlSurface = 0;" right after this line. I know it is not strictly needed but we might experience issues later if the ewk_view_create_gl_surface() starts checking the value of priv->evasGlSurface. > Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:72 > +WebCore::IntSize ewk_view_size_get(Evas_Object* ewkView); Argument should be const. > Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:75 > +void ewk_view_enter_accelerated_compositing_mode(Evas_Object* ewkView); the verb is usually at the end: "ewk_view_accelerated_compositing_mode_enter()". > Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:76 > +void ewk_view_exit_accelerated_compositing_mode(Evas_Object* ewkView); Ditto. > Tools/ChangeLog:8 > + This patch is a subset of Efl's UI_SIDE_COMPOSITING implementation. This changelog should reflect the actual change to the MiniBrowser instead of being so generic. Created attachment 153157 [details]
Patch
(In reply to comment #3) Thank you for kind review. > (From update of attachment 152925 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152925&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:76 > > ~_Ewk_View_Private_Data() > > You probably need Evas_GL clean up code in the destructor. I'm assuming the view can be destroyed without ewk_view_exit_accelerated_compositing_mode() being called beforehand. When the view is destroyed, ewk_view_exit_accelerated_compositing_mode is called like below. ~_Ewk_View_Private_Data() => ~PageClientImpl() => ~WebPageProxy() => ~DrawingAreaProxyImpl() => WebPageProxy::exitAcceleratedCompositingMode => PageClientImpl::exitAcceleratedCompositingMode => ewk_view_exit_accelerated_compositing_mode > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:383 > > +IntSize ewk_view_size_get(Evas_Object* ewkView) > > argument should be const. > fixed. > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:403 > > + > > How about an ASSERT(!priv->evasGlSurface); to make sure we don't leak memory? > fixed. > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:415 > > +void ewk_view_enter_accelerated_compositing_mode(Evas_Object* ewkView) > > How about returning a bool so that the client know if it was successful? > fixed. > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:421 > > + priv->evasGl = evas_gl_new(evas); > > I think we need an EINA_SAFETY check to make sure priv->evasGl is NULL when this function is called. What if the client calls this function several times without calling the ewk_view_exit_accelerated_compositing_mode()? We should print a warning instead of leaking memory. > fixed. > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:441 > > +void ewk_view_exit_accelerated_compositing_mode(Evas_Object* ewkView) > > How about returning a bool so that the client know if it was successful? > fixed. > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:446 > > + if (!priv->evasGl) > > Should it be an if check or an EINA_SAFETY? I guess this mean this means the client called ewk_view_exit_accelerated_compositing_mode() without calling ewk_view_enter_accelerated_compositing_mode() first, which would be wrong API usage. Also, if ewk_view_enter_accelerated_compositing_mode() starts returning a boolean, the client has not excuse. > fixed. > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:483 > > + evas_gl_surface_destroy(priv->evasGl, priv->evasGlSurface); > > I would feel better with a "priv->evasGlSurface = 0;" right after this line. I know it is not strictly needed but we might experience issues later if the ewk_view_create_gl_surface() starts checking the value of priv->evasGlSurface. > fixed. > > Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:72 > > +WebCore::IntSize ewk_view_size_get(Evas_Object* ewkView); > > Argument should be const. > fixed. > > Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:75 > > +void ewk_view_enter_accelerated_compositing_mode(Evas_Object* ewkView); > > the verb is usually at the end: "ewk_view_accelerated_compositing_mode_enter()". > > > Source/WebKit2/UIProcess/API/efl/ewk_view_private.h:76 > > +void ewk_view_exit_accelerated_compositing_mode(Evas_Object* ewkView); > > Ditto. > fixed. > > Tools/ChangeLog:8 > > + This patch is a subset of Efl's UI_SIDE_COMPOSITING implementation. > > This changelog should reflect the actual change to the MiniBrowser instead of being so generic. fixed. Comment on attachment 153157 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153157&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:393 > + EWK_VIEW_SD_GET(ewkView, smartData); Use EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, 0); > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:394 > + EWK_VIEW_PRIV_GET(smartData, priv); ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:404 > + ASSERT(!priv->evasGlSurface); Though below condition check this again, should we use this ? > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:416 > +bool ewk_view_accelerated_compositing_mode_enter(Evas_Object* ewkView) If possible, it is good to use *const* keyword. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:418 > + EWK_VIEW_SD_GET(ewkView, smartData); ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:419 > + EWK_VIEW_PRIV_GET(smartData, priv); ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:442 > + } Add an empty line. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:446 > +bool ewk_view_accelerated_compositing_mode_exit(Evas_Object* ewkView) If possible, it is good to use *const* keyword. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:448 > + EWK_VIEW_SD_GET(ewkView, smartData); ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:449 > + EWK_VIEW_PRIV_GET(smartData, priv); ditto. > Tools/MiniBrowser/efl/main.c:51 > + "GPL", AFAIK, WebKit is based on LGPL and Apple BSD. So, LGPL is more proper. > Tools/MiniBrowser/efl/main.c:224 > + ecore_app_args_set(argc, (const char**) argv); s/(const char**)/(const char **)/g Created attachment 153959 [details]
Patch
(In reply to comment #6) Thanks for kind review. > (From update of attachment 153157 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153157&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:393 > > + EWK_VIEW_SD_GET(ewkView, smartData); > > Use EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, 0); fixed. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:394 > > + EWK_VIEW_PRIV_GET(smartData, priv); > > ditto. fixed. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:404 > > + ASSERT(!priv->evasGlSurface); > > Though below condition check this again, should we use this ? It's Christophe Dumez's suggestion. It's for memory leak when the client calls this function several times. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:416 > > +bool ewk_view_accelerated_compositing_mode_enter(Evas_Object* ewkView) > > If possible, it is good to use *const* keyword. fixed. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:418 > > + EWK_VIEW_SD_GET(ewkView, smartData); > > ditto. fixed. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:419 > > + EWK_VIEW_PRIV_GET(smartData, priv); > > ditto. fixed. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:442 > > + } > > Add an empty line. fixed. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:446 > > +bool ewk_view_accelerated_compositing_mode_exit(Evas_Object* ewkView) > > If possible, it is good to use *const* keyword. fixed. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:448 > > + EWK_VIEW_SD_GET(ewkView, smartData); > > ditto. fixed. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:449 > > + EWK_VIEW_PRIV_GET(smartData, priv); > > ditto. fixed. > > > Tools/MiniBrowser/efl/main.c:51 > > + "GPL", > > AFAIK, WebKit is based on LGPL and Apple BSD. So, LGPL is more proper. fixed. > > > Tools/MiniBrowser/efl/main.c:224 > > + ecore_app_args_set(argc, (const char**) argv); > > s/(const char**)/(const char **)/g fixed. Created attachment 153970 [details]
Patch
Created attachment 154626 [details]
Patch
Comment on attachment 154626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154626&action=review > Source/WebKit2/Shared/efl/LayerTreeContextEfl.cpp:2 > + Copyright (C) 2012 Samsung Electronics I was told BSD license is more useful. So, could you change LGPL with BSD ? > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:434 > + EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, 0); Change 0 with false. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:435 > + EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, 0); ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:459 > + EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, 0); ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:460 > + EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, 0); ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:490 > + EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, 0); ditto. > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:491 > + EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, 0); ditto. Created attachment 154860 [details]
Patch
(In reply to comment #11) Thanks for kind review. > (From update of attachment 154626 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154626&action=review > > > Source/WebKit2/Shared/efl/LayerTreeContextEfl.cpp:2 > > + Copyright (C) 2012 Samsung Electronics > > I was told BSD license is more useful. So, could you change LGPL with BSD ? fixed. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:434 > > + EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, 0); > > Change 0 with false. fixed. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:435 > > + EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, 0); > > ditto. fixed. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:459 > > + EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, 0); > > ditto. fixed. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:460 > > + EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, 0); > > ditto. fixed. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:490 > > + EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, 0); > > ditto. fixed. > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:491 > > + EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, 0); > > ditto. fixed. Comment on attachment 154860 [details]
Patch
As a step to support UI_SIDE_COMPOSITING for EFL port, looks good to me now.
Comment on attachment 154860 [details] Patch Clearing flags on attachment: 154860 Committed r123872: <http://trac.webkit.org/changeset/123872> All reviewed patches have been landed. Closing bug. |
Created attachment 152921 [details] patch This patch is a subset of Efl's UI_SIDE_COMPOSITING implementation. When enter accelerated compositing mode, create evas_gl, evas_gl_context, and evas_gl_surface.