Currently construction/destruction code in Ewk_view is really overcomplicated, it comprises of multiple stages and multiple functions to be invoked, this should be simplified as much as possible. Most of the code should go to destructor/constructor of EwkViewImpl class see bug#100228.
Created attachment 170882 [details] patch
Created attachment 170890 [details] rebased
Comment on attachment 170890 [details] rebased View in context: https://bugs.webkit.org/attachment.cgi?id=170890&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:537 > + smartData->priv = new EwkViewImpl(ewkView, context, pageGroup); I would add a ASSERT(!smartData->priv); right before this line to be safe > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:553 > + return createEwkView(canvas, smart, context, 0); Maybe you can add an inline comment to clarify what the 0 stands for
Comment on attachment 170890 [details] rebased View in context: https://bugs.webkit.org/attachment.cgi?id=170890&action=review >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:537 >> + smartData->priv = new EwkViewImpl(ewkView, context, pageGroup); > > I would add a ASSERT(!smartData->priv); right before this line to be safe Oh, it's there already, sorry :)
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:553 > > + return createEwkView(canvas, smart, context, 0); > > Maybe you can add an inline comment to clarify what the 0 stands for would better make it default value :)
Created attachment 170893 [details] patch v2 added default value for pageGroupRef in createEwkView.
Comment on attachment 170893 [details] patch v2 LGTM.
Comment on attachment 170893 [details] patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=170893&action=review > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:111 > + WebKit::WebPageProxy* page() { return m_pageProxy.get(); } why remove the inline? > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:230 > + // Note, initialization order is important. initialization order matters :-) it is not just important > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:249 > + Evas_GL* m_evasGl; > + Evas_GL_Context* m_evasGlContext; > + Evas_GL_Surface* m_evasGlSurface; maybe we should add OwnPtr support for these
Comment on attachment 170893 [details] patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=170893&action=review >> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:111 >> + WebKit::WebPageProxy* page() { return m_pageProxy.get(); } > > why remove the inline? They are defined in the header so they are inlined anyway. >> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:249 >> + Evas_GL_Surface* m_evasGlSurface; > > maybe we should add OwnPtr support for these I thought about this but it is not easy because the free functions for Evas_GL_Context and Evas_GL_Surface require the Evas_GL object as well.
(In reply to comment #9) > (From update of attachment 170893 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170893&action=review > > >> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:111 > >> + WebKit::WebPageProxy* page() { return m_pageProxy.get(); } > > > > why remove the inline? > > They are defined in the header so they are inlined anyway. Only when called from the same class no? which is mostly the case right
(In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 170893 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=170893&action=review > > > > >> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:111 > > >> + WebKit::WebPageProxy* page() { return m_pageProxy.get(); } > > > > > > why remove the inline? > > > > They are defined in the header so they are inlined anyway. > > Only when called from the same class no? which is mostly the case right As far as I know member functions are implicit inline provided they are defined inside their class independently from where they called. Anyway methods should be consistent with each other: either all of them (defined in header) have 'inline' or all not have.
Created attachment 170904 [details] patch v3 initialization order matters :)
Comment on attachment 170904 [details] patch v3 Clearing flags on attachment: 170904 Committed r132647: <http://trac.webkit.org/changeset/132647>
All reviewed patches have been landed. Closing bug.
Comment on attachment 170904 [details] patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=170904&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:-576 > - impl->pageProxy->pageGroup()->preferences()->setWebGLEnabled(true); It seems like you lost some code here. This new WebGL code is removed from ewk_view.cpp but wasn't added to EwkViewImpl.cpp. Rebasing problem?