Bug 108005 – [GTK][WK2] MiniBrowser fullscreen signals support

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Philippe Normand]

SSIA

[Philippe Normand]

Created attachment 184865 [details]
Fullscreen signals support in MiniBrowser

[WebKit Review Bot]

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.

[Philippe Normand]

Created attachment 184867 [details]
Fullscreen signals support in MiniBrowser

[Philippe Normand]

Comment on attachment 184867 [details]
Fullscreen signals support in MiniBrowser

This needs some more work

[Manuel Rego Casasnovas]

Created attachment 185774 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 185774 [details]
Patch

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.

[Manuel Rego Casasnovas]

Created attachment 186002 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 186002 [details]
Patch

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

[Manuel Rego Casasnovas]

Created attachment 186017 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 186017 [details]
Patch

Excellent. Thank you.

[WebKit Review Bot]

Comment on attachment 186017 [details]
Patch

Clearing flags on attachment: 186017

Committed r141585: <http://trac.webkit.org/changeset/141585>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.