Bug 100232 – [EFL][WK2] Clean up construction/destruction code in Ewk_view

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Mikhail Pozdnyakov]

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.

[Mikhail Pozdnyakov]

Created attachment 170882 [details]
patch

[Mikhail Pozdnyakov]

Created attachment 170890 [details]
rebased

[Chris Dumez]

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

[Chris Dumez]

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 :)

[Mikhail Pozdnyakov]

> > 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 :)

[Mikhail Pozdnyakov]

Created attachment 170893 [details]
patch v2

added default value for pageGroupRef in createEwkView.

[Chris Dumez]

Comment on attachment 170893 [details]
patch v2

LGTM.

[Kenneth Rohde Christiansen]

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

[Chris Dumez]

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.

[Kenneth Rohde Christiansen]

(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

[Mikhail Pozdnyakov]

(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.

[Mikhail Pozdnyakov]

Created attachment 170904 [details]
patch v3

initialization order matters :)

[WebKit Review Bot]

Comment on attachment 170904 [details]
patch v3

Clearing flags on attachment: 170904

Committed r132647: <http://trac.webkit.org/changeset/132647>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

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?