[webkit-reviews] review canceled: [Bug 109794] [WK2][EFL] Eliminate access to WK2 C++ internals from ewk_view functions : [Attachment 188293] patch

[bugzilla-daemon at webkit.org]

Mikhail Pozdnyakov <mikhail.pozdnyakov at intel.com> has canceled Mikhail
Pozdnyakov <mikhail.pozdnyakov at intel.com>'s request for review:
Bug 109794: [WK2][EFL] Eliminate access to WK2 C++ internals from ewk_view
functions
https://bugs.webkit.org/show_bug.cgi?id=109794

Attachment 188293: patch
https://bugs.webkit.org/attachment.cgi?id=188293&action=review

------- Additional Comments from Mikhail Pozdnyakov
<mikhail.pozdnyakov at intel.com>
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.