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.
Created attachment 165300 [details] patch
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
Created attachment 165309 [details] patch
(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 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.
Created attachment 165314 [details] patch Modified the comments and relocate the setting variable.
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.
Created attachment 165316 [details] patch
(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 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.
Created attachment 165356 [details] Patch
(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 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.
Created attachment 165673 [details] Patch
(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 on attachment 165673 [details] Patch Clearing flags on attachment: 165673 Committed r129594: <http://trac.webkit.org/changeset/129594>
All reviewed patches have been landed. Closing bug.
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() ?
(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.