SSIA
Created attachment 184865 [details] Fullscreen signals support in MiniBrowser
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.
Created attachment 184867 [details] Fullscreen signals support in MiniBrowser
Comment on attachment 184867 [details] Fullscreen signals support in MiniBrowser This needs some more work
Created attachment 185774 [details] Patch
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.
Created attachment 186002 [details] Patch
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
Created attachment 186017 [details] Patch
Comment on attachment 186017 [details] Patch Excellent. Thank you.
Comment on attachment 186017 [details] Patch Clearing flags on attachment: 186017 Committed r141585: <http://trac.webkit.org/changeset/141585>
All reviewed patches have been landed. Closing bug.