Summary: | [EFL][WK2] Clean up construction/destruction code in Ewk_view | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mikhail Pozdnyakov <mikhail.pozdnyakov> | ||||||||||
Component: | WebKit EFL | Assignee: | Mikhail Pozdnyakov <mikhail.pozdnyakov> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cdumez, gyuyoung.kim, kenneth, kondapallykalyan, laszlo.gombos, lucas.de.marchi, rakuco, tonikitoo, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 100228, 100552 | ||||||||||||
Bug Blocks: | 100226, 100273 | ||||||||||||
Attachments: |
|
Description
Mikhail Pozdnyakov
2012-10-24 04:39:25 PDT
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? |