Bug 109794 – [WK2][EFL] Eliminate access to WK2 C++ internals from ewk_view functions

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Mikhail Pozdnyakov]

SSIA.

[Mikhail Pozdnyakov]

Created attachment 188291 [details]
patch

[Mikhail Pozdnyakov]

Created attachment 188293 [details]
patch

Removed extra line and pending space

[Chris Dumez]

Comment on attachment 188293 [details]
patch

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

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:113
> +    return toImpl(viewRef)->requestExitFullScreen();

return statement in void function.

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:116
> +    return 0;

Ditto.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:746
> +void EwkView::feedTouchEvent(Ewk_Touch_Event_Type type, const Eina_List* points, const Evas_Modifier* modifiers)

Feels like this may be a WebView method, except for the argument types.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:748
> +    page()->handleTouchEvent(NativeWebTouchEvent(type, points, modifiers, transformFromScene(), transformToScreen(), ecore_time_get()));

Page proxy should really not be used in EwkView, which is why I think this should be on WebView side.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:446
> +    toImpl(impl->wkPage())->setPaginationMode(static_cast<WebCore::Pagination::Mode>(mode));

WKPageSetPaginationMode() ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:455
> +    return static_cast<Ewk_Pagination_Mode>(toImpl(impl->wkPage())->paginationMode());

WKPageGetPaginationMode() ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:-472
> -    return false;

We should keep returning false if FULLSCREEN_API is disabled.

[Kenneth Rohde Christiansen]

Comment on attachment 188293 [details]
patch

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

> Source/WebKit2/ChangeLog:24
> +            Added feedTouchEvent() method so that: at fisrt ewk_view_feed_touch_event()

first*

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:108
> +
> +bool WKViewGetMainFrameInViewSourceMode(WKViewRef viewRef)
> +{
> +    return toImpl(viewRef)->mainFrameInViewSourceMode();
> +}

Does "mainframe" add any value here? 

WKViewSetShowAsSource?

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:111
> +
> +void WKViewRequestExitFullScreen(WKViewRef viewRef)
> +{

How does this integrate with the WK C api for fullscreen?

[Mikhail Pozdnyakov]

Comment on attachment 188293 [details]
patch

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

>> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:113
>> +    return toImpl(viewRef)->requestExitFullScreen();
> 
> return statement in void function.

oops. my bad :/

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:746
>> +void EwkView::feedTouchEvent(Ewk_Touch_Event_Type type, const Eina_List* points, const Evas_Modifier* modifiers)
> 
> Feels like this may be a WebView method, except for the argument types.

yeah, but this has to be solved further, together with the rest event handling.

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:748
>> +    page()->handleTouchEvent(NativeWebTouchEvent(type, points, modifiers, transformFromScene(), transformToScreen(), ecore_time_get()));
> 
> Page proxy should really not be used in EwkView, which is why I think this should be on WebView side.

We do not have C API for event objects so far..

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:446
>> +    toImpl(impl->wkPage())->setPaginationMode(static_cast<WebCore::Pagination::Mode>(mode));
> 
> WKPageSetPaginationMode() ?

I did not dare to use this as this function as WKPageSetPaginationMode is from WKPagePrivate, there is probably a reason for not exposing it.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:455
>> +    return static_cast<Ewk_Pagination_Mode>(toImpl(impl->wkPage())->paginationMode());
> 
> WKPageGetPaginationMode() ?

ditto.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:-472
>> -    return false;
> 
> We should keep returning false if FULLSCREEN_API is disabled.

yeah.

[Mikhail Pozdnyakov]

> > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:108
> > +
> > +bool WKViewGetMainFrameInViewSourceMode(WKViewRef viewRef)
> > +{
> > +    return toImpl(viewRef)->mainFrameInViewSourceMode();
> > +}
> 
> Does "mainframe" add any value here? 
> 
> WKViewSetShowAsSource?
could be WKViewGetShowsAsSource, to be consistent with WKViewGetDrawsBackground and so on.

[Mikhail Pozdnyakov]

> 
> > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:111
> > +
> > +void WKViewRequestExitFullScreen(WKViewRef viewRef)
> > +{
> 
> How does this integrate with the WK C api for fullscreen?
Do we have C api for fullscreen? I can see only WKFullScreenClientGtk, however RequestExitFullScreen seems to be rather functionality of view I guess.

[Mikhail Pozdnyakov]

Created attachment 188319 [details]
patch v2

Took comments from Chris and Kenneth into consideration.

[Kenneth Rohde Christiansen]

Comment on attachment 188319 [details]
patch v2

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

Looks fine

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:111
> +
> +void WKViewRequestExitFullScreen(WKViewRef viewRef)
> +{

when does the UA/browser request to exit the fullscreen? How is this supposed to work?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:445
> +    // FIXME: move to exported C WKPage API when it appears.

appears?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:517
> +    WKPageGetContentsAsMHTMLData(impl->wkPage(), false, context, ewkViewPageContentsCallback);

Content* no?

[Chris Dumez]

Comment on attachment 188319 [details]
patch v2

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

>> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:111
>> +{
> 
> when does the UA/browser request to exit the fullscreen? How is this supposed to work?

When the content requested full screen mode via full screen API, and we switch to full screen. If the user press "ESC" key, the browser will request to exit fullscreen using this API. You can test in our MiniBrowser.

[Kenneth Rohde Christiansen]

(In reply to comment #10)
> (From update of attachment 188319 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188319&action=review
> 
> >> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:111
> >> +{
> > 
> > when does the UA/browser request to exit the fullscreen? How is this supposed to work?
> 
> When the content requested full screen mode via full screen API, and we switch to full screen. If the user press "ESC" key, the browser will request to exit fullscreen using this API. You can test in our MiniBrowser.

As it is a request can the content actually ignore this and not exit? or is this just so that JS etc can act on exiting?

[Chris Dumez]

Comment on attachment 188319 [details]
patch v2

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

>>>> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:111
>>>> +{
>>> 
>>> when does the UA/browser request to exit the fullscreen? How is this supposed to work?
>> 
>> When the content requested full screen mode via full screen API, and we switch to full screen. If the user press "ESC" key, the browser will request to exit fullscreen using this API. You can test in our MiniBrowser.
> 
> As it is a request can the content actually ignore this and not exit? or is this just so that JS etc can act on exiting?

To my knowledge, this is so that doc.fullscreenEnabled is updated and so that a fullscreenchange event is fired. CSS style may also change depending on fullscreen state. It is not technically a request, we should probably call it "WKViewExitFullScreen()".

[Mikhail Pozdnyakov]

Created attachment 188855 [details]
patch v3

WKViewRequestExitFullScreen -> WKViewExitFullScreen

[EFL EWS Bot]

Comment on attachment 188855 [details]
patch v3

Attachment 188855 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16613156

[Mikhail Pozdnyakov]

Created attachment 188858 [details]
patch v4

rebased

[Kenneth Rohde Christiansen]

Comment on attachment 188858 [details]
patch v4

LGTM

[WebKit Review Bot]

Comment on attachment 188858 [details]
patch v4

Clearing flags on attachment: 188858

Committed r143339: <http://trac.webkit.org/changeset/143339>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.