Bug 109794

Created attachment 188291 [details] patch

Created attachment 188293 [details] patch Removed extra line and pending space

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.

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?

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.

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

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

Created attachment 188319 [details] patch v2 Took comments from Chris and Kenneth into consideration.

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?

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.

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

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

Created attachment 188855 [details] patch v3 WKViewRequestExitFullScreen -> WKViewExitFullScreen

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

Created attachment 188858 [details] patch v4 rebased

Comment on attachment 188858 [details] patch v4 LGTM

Comment on attachment 188858 [details] patch v4 Clearing flags on attachment: 188858 Committed r143339: <http://trac.webkit.org/changeset/143339>

All reviewed patches have been landed. Closing bug.