Bug 108005

Summary: [GTK][WK2] MiniBrowser fullscreen signals support
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: cgarcia, pnormand, rego, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Fullscreen signals support in MiniBrowser
Fullscreen signals support in MiniBrowser
Patch none

Description Philippe Normand 2013-01-26 07:15:42 PST
Comment 1 Philippe Normand 2013-01-26 07:18:05 PST
Created attachment 184865 [details]
Fullscreen signals support in MiniBrowser
Comment 2 WebKit Review Bot 2013-01-26 07:21:01 PST
Attachment 184865 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserWindow.c']" exit_code: 1
Tools/MiniBrowser/gtk/BrowserWindow.c:272:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Tools/MiniBrowser/gtk/BrowserWindow.c:278:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/MiniBrowser/gtk/BrowserWindow.c:279:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/MiniBrowser/gtk/BrowserWindow.c:280:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/MiniBrowser/gtk/BrowserWindow.c:281:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/MiniBrowser/gtk/BrowserWindow.c:293:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/MiniBrowser/gtk/BrowserWindow.c:294:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/MiniBrowser/gtk/BrowserWindow.c:295:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/MiniBrowser/gtk/BrowserWindow.c:296:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 9 in 2 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Philippe Normand 2013-01-26 07:42:12 PST
Created attachment 184867 [details]
Fullscreen signals support in MiniBrowser
Comment 4 Philippe Normand 2013-01-28 07:28:12 PST
Comment on attachment 184867 [details]
Fullscreen signals support in MiniBrowser

This needs some more work
Comment 5 Manuel Rego Casasnovas 2013-01-31 06:56:26 PST
Created attachment 185774 [details]
Comment 6 Carlos Garcia Campos 2013-02-01 00:45:09 PST
Comment on attachment 185774 [details]

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

Looks great, I have just a few minor comments. Thanks!

> Tools/MiniBrowser/gtk/BrowserWindow.c:55
> +    GtkWidget *fullScreenMessageLabel;

This is only used for the overlay, so add the GTK_CHECK_VERSION here too.

> Tools/MiniBrowser/gtk/BrowserWindow.c:67
> +static guint fullScreenMessageLabelId = 0;

Don't use a global variable, move this to the BrowserWindow struct.

> Tools/MiniBrowser/gtk/BrowserWindow.c:266
> +static gboolean webViewFullScreenMessageHide(GtkWidget *fullScreenMessageLabel)

This method has nothing to do with a webView, so a better name might be browserWindowHideFullScreenMessage(BrowserWindow *window) receiving a window to use both the label and the source id from the BrowserWindow struct.

> Tools/MiniBrowser/gtk/BrowserWindow.c:268
> +#if GTK_CHECK_VERSION(3, 2, 0)

This function is only called when GTK >= 3.2, so you can move the #if out of the function and define it only in that case.

> Tools/MiniBrowser/gtk/BrowserWindow.c:271
> +        g_source_remove(fullScreenMessageLabelId);

I'm not sure it's correct to remove the source inside the callback, the source is going to be removed anyway because the calback return FALSE. The problem is that you are using this function as the timeout callback and to hide the label manually. Here I would simply hide the label and return FALSE, and move the source remove to the leave callback.

> Tools/MiniBrowser/gtk/BrowserWindow.c:283
> +    const gchar *uri = webkit_web_view_get_uri(window->webView);
> +    gchar *message = g_strdup_printf("%s is now full screen. Press ESC or f to exit.", uri);

uri is used only once, so you could use webkit_web_view_get_uri directly in the g_strdup_printf. Maybe we could use the title instead of the URI? and the URI only as a fallback when there's not title?

> Tools/MiniBrowser/gtk/BrowserWindow.c:289
> +    fullScreenMessageLabelId = g_timeout_add_seconds(2, (GSourceFunc) webViewFullScreenMessageHide, window->fullScreenMessageLabel);

Remove the space between the cast and the function. If we are going to use it only as a callback we could call it fullScreenMessageTimeoutCallback, or something like that.

> Tools/MiniBrowser/gtk/BrowserWindow.c:300
> +    webViewFullScreenMessageHide(window->fullScreenMessageLabel);

I would check here if the id > 0, and remove the source in such case. Then simply call gtk_widget_hide() to hide the label.

> Tools/MiniBrowser/gtk/BrowserWindow.c:318
> +    g_signal_connect(newWebView, "enter-fullscreen", G_CALLBACK(webViewEnterFullScreen), newWindow);
> +    g_signal_connect(newWebView, "leave-fullscreen", G_CALLBACK(webViewLeaveFullScreen), newWindow);

You don't need to connect this here, when the new view is creatd the signals are already connected by the constructor.

> Tools/MiniBrowser/gtk/BrowserWindow.c:544
> +

Remove this extra line.
Comment 7 Manuel Rego Casasnovas 2013-02-01 03:54:01 PST
Created attachment 186002 [details]
Comment 8 Carlos Garcia Campos 2013-02-01 04:06:12 PST
Comment on attachment 186002 [details]

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

Looks much better, there are still a few minor issues. You should also check in the window fialize if there's a hide message callback scheduled and remove the source in that case, otherwise the callback might be called with a window already destroyed.

> Tools/MiniBrowser/gtk/BrowserWindow.c:271
> +    gtk_widget_hide(fullScreenMessageLabel);

Here you still need to set the fullScreenMessageLabelId to 0 again, so pass the BrowserWindow instead of the label to the callback.

> Tools/MiniBrowser/gtk/BrowserWindow.c:279
> +    gchar *titleOrUri = webkit_web_view_get_title(window->webView);

This should be const, and the name titleOrURI
Comment 9 Manuel Rego Casasnovas 2013-02-01 05:10:37 PST
Created attachment 186017 [details]
Comment 10 Carlos Garcia Campos 2013-02-01 05:19:20 PST
Comment on attachment 186017 [details]

Excellent. Thank you.
Comment 11 WebKit Review Bot 2013-02-01 06:12:51 PST
Comment on attachment 186017 [details]

Clearing flags on attachment: 186017

Committed r141585: <http://trac.webkit.org/changeset/141585>
Comment 12 WebKit Review Bot 2013-02-01 06:12:54 PST
All reviewed patches have been landed.  Closing bug.