Bug 62363 - [EFL][WK2] Add PageClientImpl and WebPageProxyEfl for efl port
Summary: [EFL][WK2] Add PageClientImpl and WebPageProxyEfl for efl port
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 61838
  Show dependency treegraph
 
Reported: 2011-06-09 02:08 PDT by Eunmi Lee
Modified: 2011-06-09 22:55 PDT (History)
9 users (show)

See Also:


Attachments
Patch (17.92 KB, patch)
2011-06-09 02:12 PDT, Eunmi Lee
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eunmi Lee 2011-06-09 02:08:02 PDT
I've added PageClientImpl and WebPageProxyEfl for efl port.
Comment 1 Eunmi Lee 2011-06-09 02:12:51 PDT
Created attachment 96565 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2011-06-09 02:47:01 PDT
Comment on attachment 96565 [details]
Patch

LGTM as an initial commit
Comment 3 WebKit Review Bot 2011-06-09 03:27:01 PDT
Comment on attachment 96565 [details]
Patch

Clearing flags on attachment: 96565

Committed r88440: <http://trac.webkit.org/changeset/88440>
Comment 4 WebKit Review Bot 2011-06-09 03:27:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Leandro Pereira 2011-06-09 07:03:32 PDT
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.
Comment 6 Eunmi Lee 2011-06-09 22:55:30 PDT
(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