Summary: | [EFL][WK2] Add fullscreen enter / exit signals to the ewk_view API | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexander Shalamov <alexander.shalamov> | ||||||||||||||||
Component: | WebKit2 | Assignee: | Alexander Shalamov <alexander.shalamov> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | cdumez, g.czajkowski, gyuyoung.kim, rakuco, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | |||||||||||||||||||
Bug Blocks: | 61838 | ||||||||||||||||||
Attachments: |
|
Description
Alexander Shalamov
2012-07-26 03:26:04 PDT
Created attachment 156286 [details]
Patch
Added 2 signals to ewk_view API and handling of those signals in EFL MiniBrowser
Created attachment 156666 [details]
Patch
rebased to master
Created attachment 162199 [details]
Patch 3
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. Created attachment 162731 [details]
Patch 4
- Fixed review comments
- Added fullscreen settings and unit test
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? 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. 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
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. (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. (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 '.'. 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_*
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. Comment on attachment 163087 [details]
Patch 6
LGTM (besides Gyuyoung's nit). Thanks.
Created attachment 163112 [details]
Patch 7
- Reviewed by NOBODY (OOPS!) => Reviewed by Gyuyoung Kim.
- full_screen_callback_called => fullScreenCallbackCalled
Comment on attachment 163112 [details] Patch 7 Clearing flags on attachment: 163112 Committed r128056: <http://trac.webkit.org/changeset/128056> All reviewed patches have been landed. Closing bug. |