Bug 97421 - [EFL] Add fullscreen set/get APIs and callbacks
Summary: [EFL] Add fullscreen set/get APIs and callbacks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Jinwoo Song
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-23 19:12 PDT by Jinwoo Song
Modified: 2012-09-25 23:07 PDT (History)
6 users (show)

See Also:


Attachments
patch (8.67 KB, patch)
2012-09-23 19:28 PDT, Jinwoo Song
no flags Details | Formatted Diff | Diff
patch (8.75 KB, patch)
2012-09-23 21:42 PDT, Jinwoo Song
no flags Details | Formatted Diff | Diff
patch (8.79 KB, patch)
2012-09-23 23:20 PDT, Jinwoo Song
no flags Details | Formatted Diff | Diff
patch (9.02 KB, patch)
2012-09-23 23:50 PDT, Jinwoo Song
no flags Details | Formatted Diff | Diff
Patch (10.85 KB, patch)
2012-09-24 04:43 PDT, Jinwoo Song
no flags Details | Formatted Diff | Diff
Patch (10.88 KB, patch)
2012-09-25 14:22 PDT, Jinwoo Song
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jinwoo Song 2012-09-23 19:12:11 PDT
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.
Comment 1 Jinwoo Song 2012-09-23 19:28:27 PDT
Created attachment 165300 [details]
patch
Comment 2 Gyuyoung Kim 2012-09-23 21:19:10 PDT
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
Comment 3 Jinwoo Song 2012-09-23 21:42:16 PDT
Created attachment 165309 [details]
patch
Comment 4 Jinwoo Song 2012-09-23 21:42:53 PDT
(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.
Comment 5 Gyuyoung Kim 2012-09-23 22:55:07 PDT
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.
Comment 6 Jinwoo Song 2012-09-23 23:20:27 PDT
Created attachment 165314 [details]
patch

Modified the comments and relocate the setting variable.
Comment 7 Gyuyoung Kim 2012-09-23 23:42:48 PDT
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.
Comment 8 Jinwoo Song 2012-09-23 23:50:48 PDT
Created attachment 165316 [details]
patch
Comment 9 Jinwoo Song 2012-09-23 23:51:33 PDT
(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!
Comment 10 Gyuyoung Kim 2012-09-24 00:15:36 PDT
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.
Comment 11 Jinwoo Song 2012-09-24 04:43:48 PDT
Created attachment 165356 [details]
Patch
Comment 12 Jinwoo Song 2012-09-24 04:44:54 PDT
(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.
Comment 13 Gyuyoung Kim 2012-09-24 18:06:47 PDT
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.
Comment 14 Jinwoo Song 2012-09-25 14:22:15 PDT
Created attachment 165673 [details]
Patch
Comment 15 Jinwoo Song 2012-09-25 14:23:07 PDT
(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. :)
Comment 16 WebKit Review Bot 2012-09-25 22:39:36 PDT
Comment on attachment 165673 [details]
Patch

Clearing flags on attachment: 165673

Committed r129594: <http://trac.webkit.org/changeset/129594>
Comment 17 WebKit Review Bot 2012-09-25 22:39:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Chris Dumez 2012-09-25 22:55:26 PDT
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() ?
Comment 19 Jinwoo Song 2012-09-25 23:07:18 PDT
(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.