Bug 62363 – [EFL][WK2] Add PageClientImpl and WebPageProxyEfl for efl port

• Component: WebKit2
• Status: RESOLVED FIXED

[Eunmi Lee]

I've added PageClientImpl and WebPageProxyEfl for efl port.

[Eunmi Lee]

Created attachment 96565 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 96565 [details]
Patch

LGTM as an initial commit

[WebKit Review Bot]

Comment on attachment 96565 [details]
Patch

Clearing flags on attachment: 96565

Committed r88440: <http://trac.webkit.org/changeset/88440>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Leandro Pereira]

Comment on attachment 96565 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96565&action=review

As with other WebKit2 patches, I'm mostly reviewing for style/EFL usage issues.

> Source/WebKit2/UIProcess/API/efl/PageClientImpl.cpp:73
> +#define USE_EWK_TOUCH 1
> +#if USE_EWK_TOUCH
> +    evas_object_move(m_viewObject, 0, 0);
> +#endif

Why this #define/#if?

> Source/WebKit2/UIProcess/API/efl/PageClientImpl.cpp:229
> +void PageClientImpl::findStringInCustomRepresentation(const String&, FindOptions, unsigned maxMatchCount)

maxMatchCount isn't used. Omit its name. There are other unused parameters that should have its name omitted as well, please review them.

> Source/WebKit2/UIProcess/API/efl/PageClientImpl.h:53
> +    virtual void scrollView(const WebCore::IntRect& scrollRect, const WebCore::IntSize& scrollOffset);

No need to add parameter names on prototypes.

> Source/WebKit2/UIProcess/WebPageProxy.h:668
> -#if PLATFORM(GTK)
> +#if PLATFORM(GTK) || PLATFORM(EFL)
>      void getEditorCommandsForKeyEvent(Vector<String>&);
>  #endif

This function is just a stub below; do we really need the stub and this change here?

> Source/WebKit2/UIProcess/efl/WebPageProxyEfl.cpp:51
> +#if PLATFORM(X11)
> +    platform = "X11";
> +#else
> +    platform = "Unknown";
> +#endif

Since Evas/Ecore allows you to use other backends without recompiling your program, this will fail. If the result of WebPageProxy::standardUserAgent() is cached somewhere, it would be nicer if the Evas backend could be queried in runtime. If not, you can leave it like this, as this is unlikely to impact anything anyway.

[Eunmi Lee]

(In reply to comment #5)
> (From update of attachment 96565 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96565&action=review
> 
> As with other WebKit2 patches, I'm mostly reviewing for style/EFL usage issues.
> 
> > Source/WebKit2/UIProcess/API/efl/PageClientImpl.cpp:73
> > +#define USE_EWK_TOUCH 1
> > +#if USE_EWK_TOUCH
> > +    evas_object_move(m_viewObject, 0, 0);
> > +#endif
> 
> Why this #define/#if?

Done.
I'm sorry it is my mistake and it is unnecessary codes.
I removed above codes.

> 
> > Source/WebKit2/UIProcess/API/efl/PageClientImpl.cpp:229
> > +void PageClientImpl::findStringInCustomRepresentation(const String&, FindOptions, unsigned maxMatchCount)
> 
> maxMatchCount isn't used. Omit its name. There are other unused parameters that should have its name omitted as well, please review them.

Done.
I removed all unused name from PageClientImpl.cpp.
> 
> > Source/WebKit2/UIProcess/API/efl/PageClientImpl.h:53
> > +    virtual void scrollView(const WebCore::IntRect& scrollRect, const WebCore::IntSize& scrollOffset);
> 
> No need to add parameter names on prototypes.

Done. 
I removed all parameter names from PageClientImpl.h

> 
> > Source/WebKit2/UIProcess/WebPageProxy.h:668
> > -#if PLATFORM(GTK)
> > +#if PLATFORM(GTK) || PLATFORM(EFL)
> >      void getEditorCommandsForKeyEvent(Vector<String>&);
> >  #endif
> 
> This function is just a stub below; do we really need the stub and this change here?

Done.
It is not used now, so I removed that function.

> 
> > Source/WebKit2/UIProcess/efl/WebPageProxyEfl.cpp:51
> > +#if PLATFORM(X11)
> > +    platform = "X11";
> > +#else
> > +    platform = "Unknown";
> > +#endif
> 
> Since Evas/Ecore allows you to use other backends without recompiling your program, this will fail. If the result of WebPageProxy::standardUserAgent() is cached somewhere, it would be nicer if the Evas backend could be queried in runtime. If not, you can leave it like this, as this is unlikely to impact anything anyway.

Yes, you are right.
And we can check the Evas/Ecore backends using ecore_evas_engine_name_get().
However, we can not get the Ecore_Evas because standardUserAgent() is static function.
So, I think I have to leave that codes.

You can see the patch in the below url:
https://bugs.webkit.org/show_bug.cgi?id=62429