[Webkit-unassigned] [Bug 76166] [WK2][GTK] FullScreen signals
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jan 31 14:10:44 PST 2012
https://bugs.webkit.org/show_bug.cgi?id=76166
Martin Robinson <mrobinson at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #124730|review? |review-
Flag| |
--- Comment #6 from Martin Robinson <mrobinson at webkit.org> 2012-01-31 14:10:44 PST ---
(From update of attachment 124730)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list