Bug 97421 – [EFL] Add fullscreen set/get APIs and callbacks

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Jinwoo Song]

Add setting API for JavaScript Fullscreen API and also add two callbacks to ewk_view API, 
so that UI could customise behavior when fullscreen mode is requested.

[Jinwoo Song]

Created attachment 165300 [details]
patch

[Gyuyoung Kim]

Comment on attachment 165300 [details]
patch

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

> Source/WebKit/efl/ewk/ewk_view.cpp:340
> +        bool fullscreen : 1;

To be consistent with existing API function name style, I think enableFullscreen is better.

> Source/WebKit/efl/ewk/ewk_view.cpp:4580
> +Eina_Bool ewk_view_setting_fullscreen_get(const Evas_Object* ewkView)

To be consistent with existing API function name style, I think ti is good to use ewk_view_setting_enable_fullscreen_get()

> Source/WebKit/efl/ewk/ewk_view.cpp:4592
> +Eina_Bool ewk_view_setting_fullscreen_set(Evas_Object* ewkView, Eina_Bool enable)

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:4596
> +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, false);

Missing double not operation. enable = !!enable;

Look at webkit efl coding style.
http://trac.webkit.org/wiki/EFLWebKitCodingStyle

[Jinwoo Song]

Created attachment 165309 [details]
patch

[Jinwoo Song]

(In reply to comment #2)
> (From update of attachment 165300 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165300&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:340
> > +        bool fullscreen : 1;
> 
> To be consistent with existing API function name style, I think enableFullscreen is better.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:4580
> > +Eina_Bool ewk_view_setting_fullscreen_get(const Evas_Object* ewkView)
> 
> To be consistent with existing API function name style, I think ti is good to use ewk_view_setting_enable_fullscreen_get()
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:4592
> > +Eina_Bool ewk_view_setting_fullscreen_set(Evas_Object* ewkView, Eina_Bool enable)
> 
> ditto.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:4596
> > +    EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, false);
> 
> Missing double not operation. enable = !!enable;
> 
> Look at webkit efl coding style.
> http://trac.webkit.org/wiki/EFLWebKitCodingStyle

All done.

[Gyuyoung Kim]

Comment on attachment 165309 [details]
patch

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

> Source/WebKit/efl/ewk/ewk_view.cpp:339
> +#if ENABLE(FULLSCREEN_API)

It would be good if you move this to enableXXX variable list side.

> Source/WebKit/efl/ewk/ewk_view.h:2753
> + * Enables/disables the Javascript Fullscreen API. The Javascript API allows

It looks "Enables/disables fullscreen mode." is a little clear. BTW, generally, we use lower case for comment. s/Javascript/javascript/g, s/Fullscreen/fullscreen/g

> Source/WebKit/efl/ewk/ewk_view.h:2768
> + * Returns whether the Javascript Fullscreen API is enabled or not.

To be consistent with existing API description, it looks "Queries if the fullscreen mode is enabled." is a little better.

[Jinwoo Song]

Created attachment 165314 [details]
patch

Modified the comments and relocate the setting variable.

[Gyuyoung Kim]

Comment on attachment 165314 [details]
patch

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

> Source/WebKit/efl/ewk/ewk_view.cpp:889
> +    priv->settings.enableFullscreen = priv->pageSettings->fullScreenEnabled();

Plz move this one to enableXXX list as well.

[Jinwoo Song]

Created attachment 165316 [details]
patch

[Jinwoo Song]

(In reply to comment #7)
> (From update of attachment 165314 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165314&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:889
> > +    priv->settings.enableFullscreen = priv->pageSettings->fullScreenEnabled();
> 
> Plz move this one to enableXXX list as well.

Done!

[Gyuyoung Kim]

Comment on attachment 165316 [details]
patch

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

> Source/WebKit/efl/ewk/ewk_view.h:2765
> +EAPI Eina_Bool ewk_view_setting_enable_fullscreen_set(Evas_Object *o, Eina_Bool enable);

Patch looks fine to me. But, as you know, we have landed public API with unit test together. Please add unit test as well.

[Jinwoo Song]

Created attachment 165356 [details]
Patch

[Jinwoo Song]

(In reply to comment #10)
> (From update of attachment 165316 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165316&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.h:2765
> > +EAPI Eina_Bool ewk_view_setting_enable_fullscreen_set(Evas_Object *o, Eina_Bool enable);
> 
> Patch looks fine to me. But, as you know, we have landed public API with unit test together. Please add unit test as well.

Sure, I added unit test.

[Gyuyoung Kim]

Comment on attachment 165356 [details]
Patch

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

> Source/WebKit/efl/tests/test_ewk_view.cpp:59
> +static void ewkViewEnableFullscreenCb(void* eventInfo, Evas_Object* o, void* data)

Use webView instead of o. We have used full name except for public APIs. WK2 test case also use it.

> Source/WebKit/efl/tests/test_ewk_view.cpp:64
> +    ASSERT_TRUE(ewk_view_setting_enable_fullscreen_set(o, EINA_TRUE));

In WebKit efl mailing list, we almost decide to use standard boolean type. Please land this patch after using standard boolean.

[Jinwoo Song]

Created attachment 165673 [details]
Patch

[Jinwoo Song]

(In reply to comment #13)
> (From update of attachment 165356 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165356&action=review
> 
> > Source/WebKit/efl/tests/test_ewk_view.cpp:59
> > +static void ewkViewEnableFullscreenCb(void* eventInfo, Evas_Object* o, void* data)
> 
> Use webView instead of o. We have used full name except for public APIs. WK2 test case also use it.
> 
> > Source/WebKit/efl/tests/test_ewk_view.cpp:64
> > +    ASSERT_TRUE(ewk_view_setting_enable_fullscreen_set(o, EINA_TRUE));
> 
> In WebKit efl mailing list, we almost decide to use standard boolean type. Please land this patch after using standard boolean.

All done. :)

[WebKit Review Bot]

Comment on attachment 165673 [details]
Patch

Clearing flags on attachment: 165673

Committed r129594: <http://trac.webkit.org/changeset/129594>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Comment on attachment 165673 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_view.cpp:4634
> +    if (!smartData->api->fullscreen_enter || !smartData->api->fullscreen_enter(smartData)) {

eh.. that's bad. I guess you meant fullscreen_exit() ?

[Jinwoo Song]

(In reply to comment #18)
> (From update of attachment 165673 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165673&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:4634
> > +    if (!smartData->api->fullscreen_enter || !smartData->api->fullscreen_enter(smartData)) {
> 
> eh.. that's bad. I guess you meant fullscreen_exit() ?

Oh, that's terrible mistake. :( 
I'll fix it ASAP. Thanks for notifying me.