[Webkit-unassigned] [Bug 108005] [GTK][WK2] MiniBrowser fullscreen signals support

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=108005


Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #185774|review?                     |review-
               Flag|                            |




--- Comment #6 from Carlos Garcia Campos <cgarcia at igalia.com>  2013-02-01 00:45:09 PST ---
(From update of attachment 185774)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.