[webkit-reviews] review denied: [Bug 108005] [GTK][WK2] MiniBrowser fullscreen signals support : [Attachment 185774] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Manuel Rego Casasnovas
<rego at igalia.com>'s request for review:
Bug 108005: [GTK][WK2] MiniBrowser fullscreen signals support
https://bugs.webkit.org/show_bug.cgi?id=108005

Attachment 185774: Patch
https://bugs.webkit.org/attachment.cgi?id=185774&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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.