[webkit-reviews] review denied: [Bug 76166] [WK2][GTK] FullScreen signals : [Attachment 124730] FullScreen signals

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 76166: [WK2][GTK] FullScreen signals
https://bugs.webkit.org/show_bug.cgi?id=76166

Attachment 124730: FullScreen signals
https://bugs.webkit.org/attachment.cgi?id=124730&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=124730&action=review


> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:645
> +    signals[ENTERING_FULLSCREEN] =
> +	       g_signal_new("entering-fullscreen",
> +			    G_TYPE_FROM_CLASS(webViewClass),
> +			    G_SIGNAL_RUN_LAST,
> +			    G_STRUCT_OFFSET(WebKitWebViewClass,
entering_fullscreen),
> +			    g_signal_accumulator_first_wins, 0,
> +			    webkit_marshal_BOOLEAN__VOID,
> +			    G_TYPE_BOOLEAN, 0);
> +

I guess you gave up on doing this asynchronously?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:143
>      gboolean   (* decide_policy)  (WebKitWebView		*web_view,
>				      WebKitPolicyDecision	*decision,
>				      WebKitPolicyDecisionType	type);
> +    gboolean   (* entering_fullscreen)  (WebKitWebView  *web_view);
> +    gboolean   (* leaving_fullscreen)  (WebKitWebView  *web_view);

Please keep these aligned.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:292
> +    static gboolean enteringFullScreen(WebKitWebView*, UIClientTest* test)
> +    {
> +	   if (test->m_fullScreenAllowed) {
> +	       test->m_webViewEvents.append(EnteringFullScreen);
> +	       g_timeout_add(200, (GSourceFunc) emitKeyStroke, test);
> +	   } else {
> +	      
gtk_widget_destroy(gtk_widget_get_toplevel(GTK_WIDGET(test->m_webView)));
> +	       g_main_loop_quit(test->m_mainLoop);
> +	   }
> +	   return test->m_fullScreenAllowed;
> +    }
> +
> +    static gboolean leavingFullScreen(WebKitWebView*, UIClientTest* test)
> +    {
> +	   test->m_webViewEvents.append(LeavingFullScreen);
> +	  
gtk_widget_destroy(gtk_widget_get_toplevel(GTK_WIDGET(test->m_webView)));
> +	   g_main_loop_quit(test->m_mainLoop);
> +	   return TRUE;
> +    }

You should subclass this fixture instead of just adding on to it. Why do you
destroy the WebView here? The test fixture cleans up the WebView.  Please do
not use C style casts. 

You should be careful to keep the knowedge of the fixture internal to the
fixture. If the fixture needs to call emitKeyStroke, then emitKeyStroke should
be a static method of the fixture. Why do you call g_timeout_add instead of
something like g_idle_add? 200 milliseconds seems like a long time to wait.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:335
> +    // create and send the event

Comments should be complete sentences that start with a capital letter and end
with a period.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:336
> +    GdkEvent* pressEvent = gdk_event_new(GDK_KEY_PRESS);

You should use GOwnPtr to hold the pressEven.t

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:347
> +    GdkKeymapKey* keys;

You should use GOwnPtr here.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:354
> +    GdkEvent* releaseEvent = gdk_event_copy(pressEvent);

You should use GOwnPtr here too.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:386
> +		      "function thunk() {"
> +		      "    document.removeEventListener(eventName, thunk,
false);"
> +		      "    document.documentElement.webkitRequestFullScreen();"

> +		      "}"
> +		      "document.addEventListener(eventName, thunk, false);"

Just use an anonymous function inline.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:390
> +    GtkWidget* window;
> +    window = gtk_window_new(GTK_WINDOW_POPUP);

This should be one line.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:394
> +    g_timeout_add(100, (GSourceFunc) emitKeyStroke, test);

C++ style cast.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:425
> +    test->loadHtml("<html><body>"
> +		      "<script>"
> +		      "var eventName = 'keypress';"
> +		      "function thunk() {"
> +		      "    document.removeEventListener(eventName, thunk,
false);"
> +		      "    document.documentElement.webkitRequestFullScreen();"

> +		      "}"
> +		      "document.addEventListener(eventName, thunk, false);"
> +		      "</script></body></html>", 0);
> +
> +    GtkWidget* window;
> +    window = gtk_window_new(GTK_WINDOW_POPUP);
> +    gtk_container_add(GTK_CONTAINER(window), GTK_WIDGET(test->m_webView));
> +
> +    gtk_widget_show_all(window);
> +    g_timeout_add(100, (GSourceFunc) emitKeyStroke, test);
> +
> +    test->waitUntilMainLoopFinishes();

It seems like this is very similar to the code above. Can you refactor it so
that it's shared?

> Source/WebKit2/UIProcess/gtk/FullScreenWindowController.cpp:78
> +    gboolean returnValue;
> +    g_signal_emit_by_name(WEBKIT_WEB_VIEW(m_webViewBase.get()),
"entering-fullscreen", &returnValue);
> +    if (!returnValue)
> +	   return;
> +
>      this->fullScreenManagerProxy()->willEnterFullScreen();
>      this->fullScreenManagerProxy()->beginEnterFullScreenAnimation(10);

A WebKitWebViewBase is not a WebKitWebView when you are using the C API.
Perhaps we should only implement this default behavior when using GObject API.
I think it makes sense to put the default behavior in the virtual method that
you add above. This way, extending the WebView and chaining up works properly.

> Tools/MiniBrowser/gtk/BrowserWindow.c:225
> +static gboolean webViewFullscreenMessageWindowClose(GtkWidget *dialog)
> +{
> +    if (GTK_IS_WIDGET (dialog))
> +	   g_signal_emit_by_name (GTK_DIALOG (dialog), "close");
> +    return FALSE;
> +}

You are using Gnome coding style in this file. check-webkit-style should catch
all of these errors.