Bug 130969

Created attachment 228179 [details] Patch

Comment on attachment 228179 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228179&action=review Thanks, I really wait this. > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:49 > + // FIXME: falling back to software mode > + ASSERT(m_evasGLContext); I am not sure whether it is better to cut the software mode first and push back in another bug. We normally use software mode in EWebLauncher and elm_web as a default. > Source/WebKit/efl/ewk/ewk_view.cpp:-276 > size_t count; > size_t allocated; > } repaints; > - WTF::Vector<WebCore::IntRect> m_rectsToScroll; > - WTF::Vector<WebCore::IntSize> m_scrollOffsets; Can we remove repaints too? > Source/WebKit/efl/ewk/ewk_view.cpp:873 > + Ewk_View_Private_Data* priv = static_cast<Ewk_View_Private_Data*>(data); > + priv->acceleratedCompositingContext->flushAndRenderLayers(); We can just pass acceleratedCompositingContext. > Source/WebKit/efl/ewk/ewk_view.cpp:938 > + if (!priv->acceleratedCompositingContext) { > + ERR("Could not create accelerated compositing context."); > + return; > + } Unnecessary check routine.

Created attachment 228359 [details] Patch

Comment on attachment 228359 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228359&action=review > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:40 > + , m_rootCompositingLayer(0) nullptr > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:42 > + , m_isAccelerated(true) Nit, after setting false as a default, we can turn on it at 52 line. It reduces one if statment. > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:76 > + return true; > + m_viewSize = size; Empty line here. By the way, should we return boolean? > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:181 > void AcceleratedCompositingContext::attachRootGraphicsLayer(GraphicsLayer* rootLayer) > { From now, this is not to attach root graphics layer. How about changing method name properly? > Source/WebKit/efl/ewk/ewk_view.cpp:870 > +void _ewk_view_accelerated_compositing_cb(void* data, Evas_Object*) static ?

(In reply to comment #2) > (From update of attachment 228179 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=228179&action=review > > Thanks, I really wait this. > > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:49 > > + // FIXME: falling back to software mode > > + ASSERT(m_evasGLContext); > > I am not sure whether it is better to cut the software mode first and push back in another bug. > > We normally use software mode in EWebLauncher and elm_web as a default. Done. I've added the fall back to software mode. > > Source/WebKit/efl/ewk/ewk_view.cpp:-276 > > size_t count; > > size_t allocated; > > } repaints; > > - WTF::Vector<WebCore::IntRect> m_rectsToScroll; > > - WTF::Vector<WebCore::IntSize> m_scrollOffsets; > > Can we remove repaints too? Yes, but to avoid making the patch size bigger, let's handle that in a new bug later. > > Source/WebKit/efl/ewk/ewk_view.cpp:873 > > + Ewk_View_Private_Data* priv = static_cast<Ewk_View_Private_Data*>(data); > > + priv->acceleratedCompositingContext->flushAndRenderLayers(); > > We can just pass acceleratedCompositingContext. > > > Source/WebKit/efl/ewk/ewk_view.cpp:938 > > + if (!priv->acceleratedCompositingContext) { > > + ERR("Could not create accelerated compositing context."); > > + return; > > + } > > Unnecessary check routine. Done.

Created attachment 228366 [details] Patch

(In reply to comment #6) > Created an attachment (id=228366) [details] > Patch LGTM

(In reply to comment #4) > (From update of attachment 228359 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=228359&action=review > > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:40 > > + , m_rootCompositingLayer(0) > > nullptr Done. > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:42 > > + , m_isAccelerated(true) > > Nit, after setting false as a default, we can turn on it at 52 line. > It reduces one if statment. > > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:76 > > + return true; > > + m_viewSize = size; > > Empty line here. > > By the way, should we return boolean? The caller was not checking the return value, so I replaced bool with void and added some error logs. > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:181 > > void AcceleratedCompositingContext::attachRootGraphicsLayer(GraphicsLayer* rootLayer) > > { > > From now, this is not to attach root graphics layer. > > How about changing method name properly? I found out some unnecessary code from your comment. We will not create a dummy root, m_rootLayer, instead m_rootLayer is set as an actual compositing root layer. > > Source/WebKit/efl/ewk/ewk_view.cpp:870 > > +void _ewk_view_accelerated_compositing_cb(void* data, Evas_Object*) > > static ? Done.

(In reply to comment #7) > (In reply to comment #6) > > Created an attachment (id=228366) [details] [details] > > Patch > > LGTM Thanks for your review.

Comment on attachment 228366 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228366&action=review > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:57 > + m_isAccelerated = false; Looks wrong indentation. > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:74 > + m_viewSize = size; Is there any problem to update *m_viewSize* with *size* when m_isAccelerated is false ?

(In reply to comment #10) > (From update of attachment 228366 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=228366&action=review > > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:57 > > + m_isAccelerated = false; > > Looks wrong indentation. It looks correct. > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:74 > > + m_viewSize = size; > > Is there any problem to update *m_viewSize* with *size* when m_isAccelerated is false ? No. m_viewSize is used to keep webview size regardless of acceleration.

Comment on attachment 228366 [details] Patch Ok, LGTM.

Comment on attachment 228366 [details] Patch Clearing flags on attachment: 228366 Committed r166637: <http://trac.webkit.org/changeset/166637>

All reviewed patches have been landed. Closing bug.