Bug 92362 – [EFL][WK2] Add fullscreen enter / exit signals to the ewk_view API

• Component: WebKit2
• Status: RESOLVED FIXED

[Alexander Shalamov]

FullScreenClient implementation is missing in EFL WK2 port. This feature is required to control full screen mode in ewk_view.

[Alexander Shalamov]

Created attachment 156286 [details]
Patch

Added 2 signals to ewk_view API and handling of those signals in EFL MiniBrowser

[Alexander Shalamov]

Created attachment 156666 [details]
Patch

rebased to master

[Alexander Shalamov]

Created attachment 162199 [details]
Patch 3

[Gyuyoung Kim]

Comment on attachment 162199 [details]
Patch 3

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:722
> +#if ENABLE(FULLSCREEN_API)

For code readability, I think "priv->pageProxy->XXX" needs to be placed to below priv->xxx setting list,

For example, like this.

#if USE(COORDINATED_GRAPHICS)
    priv->viewportHandler = EflViewportHandler::create(ewkView);
#endif

#if ENABLE(FULLSCREEN_API)
    priv->pageProxy->fullScreenManager()->setWebView(ewkView);
    priv->pageProxy->pageGroup()->preferences()->setFullScreenEnabled(true);
#endif

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1169
> +#if ENABLE(FULLSCREEN_API)

I think we don't need to use #if ENABLE in function inside for internal function.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:45
> + * - "fullscreen,enter", emitted when view is requested to enter full screen mode.

Missing *void* compared to other signal.

* - "load,provisional,redirect", void: view received redirect for provisional load.

> Tools/MiniBrowser/efl/main.c:25
>  #include <Evas.h>

Generally, we have used a new line before #ifdef in include file list.

[Alexander Shalamov]

Created attachment 162731 [details]
Patch 4

- Fixed review comments
- Added fullscreen settings and unit test

[Chris Dumez]

Comment on attachment 162731 [details]
Patch 4

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

Doesn't it allow to unskip any test?

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:34
> +Eina_Bool ewk_settings_fullscreen_enabled_set(Ewk_Settings *settings, Eina_Bool enable)

Star on wrong side.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:36
> +    EINA_SAFETY_ON_NULL_RETURN_VAL(settings, false);

Can be moved inside the #if.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:46
> +Eina_Bool ewk_settings_fullscreen_enabled_get(const Ewk_Settings *settings)

Star on wrong side.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:48
> +    EINA_SAFETY_ON_NULL_RETURN_VAL(settings, false);

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1170
> +    evas_object_smart_callback_call(ewkView, "fullscreen,enter", 0);

What's the benefit / use case for sending signals to the client?
Why not call ecore_evas_fullscreen_set() directly on the view?

[Chris Dumez]

Comment on attachment 162731 [details]
Patch 4

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

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:54
> + *

You should document what the default state is.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:61
> + *

Ditto.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_settings.cpp:39
> +    ASSERT_TRUE(ewk_settings_fullscreen_enabled_set(settings, EINA_TRUE));

You should test the default state.

[Alexander Shalamov]

Created attachment 163078 [details]
Patch 5

- Fixed review comments
- Added two callbacks to ewk_view for UI behaviour customisation
- Added two unit tests to ewk_view suite

[Gyuyoung Kim]

Comment on attachment 163078 [details]
Patch 5

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

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:51
> + * Default value for Javascript Fullscreen API setting is @c EINA_TRUE

Nit : Missing . at the end of line.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:53
> + * @param settings settings object to enable Javascript Fullscreen API

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:54
> + * @param enable @c EINA_TRUE to enable Javascript Fullscreen API

I think it is better to add "or," to at the end of line.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:55
> + *               @c EINA_FALSE to disable

Nit : Missing . at the end of line.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:57
> + * @return @c EINA_TRUE on success or @c EINA_FALSE on failure

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:64
> + * @param settings settings object to query whether Javascript Fullscreen API is enabled

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:66
> + * @return @c EINA_TRUE if the Javascript Fullscreen API is enabled

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:67
> + *         @c EINA_FALSE if not or on failure

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1165
> + * Calls fullscreen_enter callback or falls back to default behavior and enables fullscreen mode

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1179
> + * Calls fullscreen_exit callback or falls back to default behavior and disables fullscreen mode

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:108
> +    Eina_Bool (*fullscreen_enter)(Ewk_View_Smart_Data *sd);

Move fullscreen_xxx to focus_out below for code readability.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:257
> +        Ewk_View_Smart_Data* smartData = (Ewk_View_Smart_Data*)evas_object_smart_data_get(webView);

Use static_cast<> instead of C type casting.

[Grzegorz Czajkowski]

(In reply to comment #9)
> (From update of attachment 163078 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163078&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_settings.h:51
> > + * Default value for Javascript Fullscreen API setting is @c EINA_TRUE
> 
> Nit : Missing . at the end of line.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_settings.h:53
> > + * @param settings settings object to enable Javascript Fullscreen API
> 
> ditto.
> 
I think it's better to do not add '.' at the end of line for @param, @return doxygen tags. Those tags don't start the sentence (it's some kind of enumeration) so adding commas it's not recommended.

[Gyuyoung Kim]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 163078 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=163078&action=review
> > 
> > > Source/WebKit2/UIProcess/API/efl/ewk_settings.h:51
> > > + * Default value for Javascript Fullscreen API setting is @c EINA_TRUE
> > 
> > Nit : Missing . at the end of line.
> > 
> > > Source/WebKit2/UIProcess/API/efl/ewk_settings.h:53
> > > + * @param settings settings object to enable Javascript Fullscreen API
> > 
> > ditto.
> > 
> I think it's better to do not add '.' at the end of line for @param, @return doxygen tags. Those tags don't start the sentence (it's some kind of enumeration) so adding commas it's not recommended.

Grzegorz, thank you for your point out. I compared to local Tizen source. Sorry for noise.

Alexander, please fix except for '.'.

[Alexander Shalamov]

Created attachment 163087 [details]
Patch 6

- Added dots according to the comments (didn't add for @param, @return doxygen tags)
- Removed dependency to ecore_x_*

[Gyuyoung Kim]

Comment on attachment 163087 [details]
Patch 6

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

Looks make sense now.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:36
> +bool full_screen_callback_called;

Please use webkit style for flag naming.

[Chris Dumez]

Comment on attachment 163087 [details]
Patch 6

LGTM (besides Gyuyoung's nit). Thanks.

[Alexander Shalamov]

Created attachment 163112 [details]
Patch 7

- Reviewed by NOBODY (OOPS!) => Reviewed by Gyuyoung Kim.
- full_screen_callback_called => fullScreenCallbackCalled

[WebKit Review Bot]

Comment on attachment 163112 [details]
Patch 7

Clearing flags on attachment: 163112

Committed r128056: <http://trac.webkit.org/changeset/128056>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.