Bug 137775 - [GTK] Minibrowser : Add window fullscreen support for Minibrowser
Summary: [GTK] Minibrowser : Add window fullscreen support for Minibrowser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-16 02:52 PDT by Rohit
Modified: 2014-10-30 06:45 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.58 KB, patch)
2014-10-16 03:00 PDT, Rohit
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rohit 2014-10-16 02:52:35 PDT
Add support for window fullscreen toggling in Minibrowser using keyboard F11 key.
Comment 1 Rohit 2014-10-16 03:00:18 PDT
Created attachment 239941 [details]
Patch
Comment 2 Carlos Garcia Campos 2014-10-22 00:26:30 PDT
Comment on attachment 239941 [details]
Patch

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

> Tools/MiniBrowser/gtk/BrowserWindow.c:575
> +    if (!window->fullScreenIsEnabled) {
> +        gtk_window_fullscreen(GTK_WINDOW(window));
> +        gtk_widget_hide(window->toolbar);
> +        window->fullScreenIsEnabled = TRUE;
> +    } else {
> +        gtk_window_unfullscreen(GTK_WINDOW(window));
> +        gtk_widget_show(window->toolbar);
> +        window->fullScreenIsEnabled = FALSE;
> +    }

We already have code to enter/leave fullscreen when requested by wk, maybe we can reuse that code to make it work also when requested by the user, because there are some inconsistencies. The other code shows a message explaining the user how to leave fullscreen mode, and the keybindings are f or ESC, not F11. Also the other code takes care of showing/hide the findbar.
Comment 3 Rohit 2014-10-22 02:07:15 PDT
(In reply to comment #2)
> Comment on attachment 239941 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=239941&action=review
> 
> > Tools/MiniBrowser/gtk/BrowserWindow.c:575
> > +    if (!window->fullScreenIsEnabled) {
> > +        gtk_window_fullscreen(GTK_WINDOW(window));
> > +        gtk_widget_hide(window->toolbar);
> > +        window->fullScreenIsEnabled = TRUE;
> > +    } else {
> > +        gtk_window_unfullscreen(GTK_WINDOW(window));
> > +        gtk_widget_show(window->toolbar);
> > +        window->fullScreenIsEnabled = FALSE;
> > +    }
> 
> We already have code to enter/leave fullscreen when requested by wk, maybe
> we can reuse that code to make it work also when requested by the user,
> because there are some inconsistencies. The other code shows a message
> explaining the user how to leave fullscreen mode, and the keybindings are f
> or ESC, not F11. Also the other code takes care of showing/hide the findbar.

I think the code to enter/leave fullscreen is for HTML element/webview fullscreen and not for the minibrowser window itself. We don't need message and title for window to be displayed on fullscreen in this case. Most of the browsers (Chrome, Firefox, IE) uses F11 key to toggle browser window fullscreen and f or ESC for HTML elements. Please correct me if I am wrong.
Comment 4 Carlos Garcia Campos 2014-10-22 02:32:06 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 239941 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=239941&action=review
> > 
> > > Tools/MiniBrowser/gtk/BrowserWindow.c:575
> > > +    if (!window->fullScreenIsEnabled) {
> > > +        gtk_window_fullscreen(GTK_WINDOW(window));
> > > +        gtk_widget_hide(window->toolbar);
> > > +        window->fullScreenIsEnabled = TRUE;
> > > +    } else {
> > > +        gtk_window_unfullscreen(GTK_WINDOW(window));
> > > +        gtk_widget_show(window->toolbar);
> > > +        window->fullScreenIsEnabled = FALSE;
> > > +    }
> > 
> > We already have code to enter/leave fullscreen when requested by wk, maybe
> > we can reuse that code to make it work also when requested by the user,
> > because there are some inconsistencies. The other code shows a message
> > explaining the user how to leave fullscreen mode, and the keybindings are f
> > or ESC, not F11. Also the other code takes care of showing/hide the findbar.
> 
> I think the code to enter/leave fullscreen is for HTML element/webview
> fullscreen and not for the minibrowser window itself.

Right.

> We don't need message
> and title for window to be displayed on fullscreen in this case. Most of the
> browsers (Chrome, Firefox, IE) uses F11 key to toggle browser window
> fullscreen and f or ESC for HTML elements. Please correct me if I am wrong.

Yes, Epiphany also uses F11, but also shows a message. In any case, my point is that the code that enters/leaves fullscreen could be common in both cases. Move the common logic to a function, and call it from both places.
Comment 5 Rohit 2014-10-23 03:06:56 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Comment on attachment 239941 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=239941&action=review
> > > 
> > > > Tools/MiniBrowser/gtk/BrowserWindow.c:575
> > > > +    if (!window->fullScreenIsEnabled) {
> > > > +        gtk_window_fullscreen(GTK_WINDOW(window));
> > > > +        gtk_widget_hide(window->toolbar);
> > > > +        window->fullScreenIsEnabled = TRUE;
> > > > +    } else {
> > > > +        gtk_window_unfullscreen(GTK_WINDOW(window));
> > > > +        gtk_widget_show(window->toolbar);
> > > > +        window->fullScreenIsEnabled = FALSE;
> > > > +    }
> > > 
> > > We already have code to enter/leave fullscreen when requested by wk, maybe
> > > we can reuse that code to make it work also when requested by the user,
> > > because there are some inconsistencies. The other code shows a message
> > > explaining the user how to leave fullscreen mode, and the keybindings are f
> > > or ESC, not F11. Also the other code takes care of showing/hide the findbar.
> > 
> > I think the code to enter/leave fullscreen is for HTML element/webview
> > fullscreen and not for the minibrowser window itself.
> 
> Right.
> 
> > We don't need message
> > and title for window to be displayed on fullscreen in this case. Most of the
> > browsers (Chrome, Firefox, IE) uses F11 key to toggle browser window
> > fullscreen and f or ESC for HTML elements. Please correct me if I am wrong.
> 
> Yes, Epiphany also uses F11, but also shows a message. In any case, my point
> is that the code that enters/leaves fullscreen could be common in both
> cases. Move the common logic to a function, and call it from both places.

There is no common code in BrowserWindow.c file for webkitView and window fullscreen except findbar hide. Are you suggesting to implement it in WebKitWebViewBase.cpp file to handle the F11 keypress event similar to ESC and F key handle?
Comment 6 Carlos Garcia Campos 2014-10-23 03:53:11 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > Comment on attachment 239941 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=239941&action=review
> > > > 
> > > > > Tools/MiniBrowser/gtk/BrowserWindow.c:575
> > > > > +    if (!window->fullScreenIsEnabled) {
> > > > > +        gtk_window_fullscreen(GTK_WINDOW(window));
> > > > > +        gtk_widget_hide(window->toolbar);
> > > > > +        window->fullScreenIsEnabled = TRUE;
> > > > > +    } else {
> > > > > +        gtk_window_unfullscreen(GTK_WINDOW(window));
> > > > > +        gtk_widget_show(window->toolbar);
> > > > > +        window->fullScreenIsEnabled = FALSE;
> > > > > +    }
> > > > 
> > > > We already have code to enter/leave fullscreen when requested by wk, maybe
> > > > we can reuse that code to make it work also when requested by the user,
> > > > because there are some inconsistencies. The other code shows a message
> > > > explaining the user how to leave fullscreen mode, and the keybindings are f
> > > > or ESC, not F11. Also the other code takes care of showing/hide the findbar.
> > > 
> > > I think the code to enter/leave fullscreen is for HTML element/webview
> > > fullscreen and not for the minibrowser window itself.
> > 
> > Right.
> > 
> > > We don't need message
> > > and title for window to be displayed on fullscreen in this case. Most of the
> > > browsers (Chrome, Firefox, IE) uses F11 key to toggle browser window
> > > fullscreen and f or ESC for HTML elements. Please correct me if I am wrong.
> > 
> > Yes, Epiphany also uses F11, but also shows a message. In any case, my point
> > is that the code that enters/leaves fullscreen could be common in both
> > cases. Move the common logic to a function, and call it from both places.
> 
> There is no common code in BrowserWindow.c file for webkitView and window
> fullscreen except findbar hide. Are you suggesting to implement it in
> WebKitWebViewBase.cpp file to handle the F11 keypress event similar to ESC
> and F key handle?

Oh, you are right, sorry, I thought we were also doing the fullscreen/unfullscreen in MB when entering/leaving fs, but that's done by the web view. So, yes, it's enough to move the searchbar visibility code to their own functions and call those from both places. Something like browserWindowHideSearchBarForFullscreen() and browserWindowShowSearchBarAfterFullscreen(), for example
Comment 7 Rohit 2014-10-23 04:11:06 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (In reply to comment #3)
> > > > (In reply to comment #2)
> > > > > Comment on attachment 239941 [details]
> > > > > Patch
> > > > > 
> > > > > View in context:
> > > > > https://bugs.webkit.org/attachment.cgi?id=239941&action=review
> > > > > 
> > > > > > Tools/MiniBrowser/gtk/BrowserWindow.c:575
> > > > > > +    if (!window->fullScreenIsEnabled) {
> > > > > > +        gtk_window_fullscreen(GTK_WINDOW(window));
> > > > > > +        gtk_widget_hide(window->toolbar);
> > > > > > +        window->fullScreenIsEnabled = TRUE;
> > > > > > +    } else {
> > > > > > +        gtk_window_unfullscreen(GTK_WINDOW(window));
> > > > > > +        gtk_widget_show(window->toolbar);
> > > > > > +        window->fullScreenIsEnabled = FALSE;
> > > > > > +    }
> > > > > 
> > > > > We already have code to enter/leave fullscreen when requested by wk, maybe
> > > > > we can reuse that code to make it work also when requested by the user,
> > > > > because there are some inconsistencies. The other code shows a message
> > > > > explaining the user how to leave fullscreen mode, and the keybindings are f
> > > > > or ESC, not F11. Also the other code takes care of showing/hide the findbar.
> > > > 
> > > > I think the code to enter/leave fullscreen is for HTML element/webview
> > > > fullscreen and not for the minibrowser window itself.
> > > 
> > > Right.
> > > 
> > > > We don't need message
> > > > and title for window to be displayed on fullscreen in this case. Most of the
> > > > browsers (Chrome, Firefox, IE) uses F11 key to toggle browser window
> > > > fullscreen and f or ESC for HTML elements. Please correct me if I am wrong.
> > > 
> > > Yes, Epiphany also uses F11, but also shows a message. In any case, my point
> > > is that the code that enters/leaves fullscreen could be common in both
> > > cases. Move the common logic to a function, and call it from both places.
> > 
> > There is no common code in BrowserWindow.c file for webkitView and window
> > fullscreen except findbar hide. Are you suggesting to implement it in
> > WebKitWebViewBase.cpp file to handle the F11 keypress event similar to ESC
> > and F key handle?
> 
> Oh, you are right, sorry, I thought we were also doing the
> fullscreen/unfullscreen in MB when entering/leaving fs, but that's done by
> the web view. So, yes, it's enough to move the searchbar visibility code to
> their own functions and call those from both places. Something like
> browserWindowHideSearchBarForFullscreen() and
> browserWindowShowSearchBarAfterFullscreen(), for example

Do you mean to say hide 'toolbar' in place of searchbar? We dont need to hide 'searchbar' as this beahviour is not needed in case of window fullscreen and needed in case of webview fullscreen.(checked in firefox and chrome).
Comment 8 Carlos Garcia Campos 2014-10-23 04:16:03 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > (In reply to comment #3)
> > > > > (In reply to comment #2)
> > > > > > Comment on attachment 239941 [details]
> > > > > > Patch
> > > > > > 
> > > > > > View in context:
> > > > > > https://bugs.webkit.org/attachment.cgi?id=239941&action=review
> > > > > > 
> > > > > > > Tools/MiniBrowser/gtk/BrowserWindow.c:575
> > > > > > > +    if (!window->fullScreenIsEnabled) {
> > > > > > > +        gtk_window_fullscreen(GTK_WINDOW(window));
> > > > > > > +        gtk_widget_hide(window->toolbar);
> > > > > > > +        window->fullScreenIsEnabled = TRUE;
> > > > > > > +    } else {
> > > > > > > +        gtk_window_unfullscreen(GTK_WINDOW(window));
> > > > > > > +        gtk_widget_show(window->toolbar);
> > > > > > > +        window->fullScreenIsEnabled = FALSE;
> > > > > > > +    }
> > > > > > 
> > > > > > We already have code to enter/leave fullscreen when requested by wk, maybe
> > > > > > we can reuse that code to make it work also when requested by the user,
> > > > > > because there are some inconsistencies. The other code shows a message
> > > > > > explaining the user how to leave fullscreen mode, and the keybindings are f
> > > > > > or ESC, not F11. Also the other code takes care of showing/hide the findbar.
> > > > > 
> > > > > I think the code to enter/leave fullscreen is for HTML element/webview
> > > > > fullscreen and not for the minibrowser window itself.
> > > > 
> > > > Right.
> > > > 
> > > > > We don't need message
> > > > > and title for window to be displayed on fullscreen in this case. Most of the
> > > > > browsers (Chrome, Firefox, IE) uses F11 key to toggle browser window
> > > > > fullscreen and f or ESC for HTML elements. Please correct me if I am wrong.
> > > > 
> > > > Yes, Epiphany also uses F11, but also shows a message. In any case, my point
> > > > is that the code that enters/leaves fullscreen could be common in both
> > > > cases. Move the common logic to a function, and call it from both places.
> > > 
> > > There is no common code in BrowserWindow.c file for webkitView and window
> > > fullscreen except findbar hide. Are you suggesting to implement it in
> > > WebKitWebViewBase.cpp file to handle the F11 keypress event similar to ESC
> > > and F key handle?
> > 
> > Oh, you are right, sorry, I thought we were also doing the
> > fullscreen/unfullscreen in MB when entering/leaving fs, but that's done by
> > the web view. So, yes, it's enough to move the searchbar visibility code to
> > their own functions and call those from both places. Something like
> > browserWindowHideSearchBarForFullscreen() and
> > browserWindowShowSearchBarAfterFullscreen(), for example
> 
> Do you mean to say hide 'toolbar' in place of searchbar? We dont need to
> hide 'searchbar' as this beahviour is not needed in case of window
> fullscreen and needed in case of webview fullscreen.(checked in firefox and
> chrome).

I don't know why we hide the searchbar when entering fs TBH, I proposed it for consistency.
Comment 9 Rohit 2014-10-27 00:14:06 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > (In reply to comment #4)
> > > > > (In reply to comment #3)
> > > > > > (In reply to comment #2)
> > > > > > > Comment on attachment 239941 [details]
> > > > > > > Patch
> > > > > > > 
> > > > > > > View in context:
> > > > > > > https://bugs.webkit.org/attachment.cgi?id=239941&action=review
> > > > > > > 
> > > > > > > > Tools/MiniBrowser/gtk/BrowserWindow.c:575
> > > > > > > > +    if (!window->fullScreenIsEnabled) {
> > > > > > > > +        gtk_window_fullscreen(GTK_WINDOW(window));
> > > > > > > > +        gtk_widget_hide(window->toolbar);
> > > > > > > > +        window->fullScreenIsEnabled = TRUE;
> > > > > > > > +    } else {
> > > > > > > > +        gtk_window_unfullscreen(GTK_WINDOW(window));
> > > > > > > > +        gtk_widget_show(window->toolbar);
> > > > > > > > +        window->fullScreenIsEnabled = FALSE;
> > > > > > > > +    }
> > > > > > > 
> > > > > > > We already have code to enter/leave fullscreen when requested by wk, maybe
> > > > > > > we can reuse that code to make it work also when requested by the user,
> > > > > > > because there are some inconsistencies. The other code shows a message
> > > > > > > explaining the user how to leave fullscreen mode, and the keybindings are f
> > > > > > > or ESC, not F11. Also the other code takes care of showing/hide the findbar.
> > > > > > 
> > > > > > I think the code to enter/leave fullscreen is for HTML element/webview
> > > > > > fullscreen and not for the minibrowser window itself.
> > > > > 
> > > > > Right.
> > > > > 
> > > > > > We don't need message
> > > > > > and title for window to be displayed on fullscreen in this case. Most of the
> > > > > > browsers (Chrome, Firefox, IE) uses F11 key to toggle browser window
> > > > > > fullscreen and f or ESC for HTML elements. Please correct me if I am wrong.
> > > > > 
> > > > > Yes, Epiphany also uses F11, but also shows a message. In any case, my point
> > > > > is that the code that enters/leaves fullscreen could be common in both
> > > > > cases. Move the common logic to a function, and call it from both places.
> > > > 
> > > > There is no common code in BrowserWindow.c file for webkitView and window
> > > > fullscreen except findbar hide. Are you suggesting to implement it in
> > > > WebKitWebViewBase.cpp file to handle the F11 keypress event similar to ESC
> > > > and F key handle?
> > > 
> > > Oh, you are right, sorry, I thought we were also doing the
> > > fullscreen/unfullscreen in MB when entering/leaving fs, but that's done by
> > > the web view. So, yes, it's enough to move the searchbar visibility code to
> > > their own functions and call those from both places. Something like
> > > browserWindowHideSearchBarForFullscreen() and
> > > browserWindowShowSearchBarAfterFullscreen(), for example
> > 
> > Do you mean to say hide 'toolbar' in place of searchbar? We dont need to
> > hide 'searchbar' as this beahviour is not needed in case of window
> > fullscreen and needed in case of webview fullscreen.(checked in firefox and
> > chrome).
> 
> I don't know why we hide the searchbar when entering fs TBH, I proposed it
> for consistency.

I think we need to hide searchbar while entering fs because otherwise searchbar would be visible on top of fullscreen element.
Comment 10 WebKit Commit Bot 2014-10-30 06:45:05 PDT
Comment on attachment 239941 [details]
Patch

Clearing flags on attachment: 239941

Committed r175371: <http://trac.webkit.org/changeset/175371>
Comment 11 WebKit Commit Bot 2014-10-30 06:45:08 PDT
All reviewed patches have been landed.  Closing bug.