[Webkit-unassigned] [Bug 125463] [GTK] Add API to allow setting the process model in WebKitWebContext
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jan 24 00:48:10 PST 2014
https://bugs.webkit.org/show_bug.cgi?id=125463
Carlos Garcia Campos <cgarcia at igalia.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #221848|review?, commit-queue? |review-, commit-queue-
Flag| |
--- Comment #14 from Carlos Garcia Campos <cgarcia at igalia.com> 2014-01-24 00:45:37 PST ---
(From update of attachment 221848)
View in context: https://bugs.webkit.org/attachment.cgi?id=221848&action=review
This looks great in general, I have a few comments.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:900
> + * determine how auxiliary processes are handled. The default setting is
> + * %WEBKIT_PROCESS_MODEL_SHARED_SECONDARY_PROCESS is suitable for most
The default setting is - is
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:902
> + * display documents which is considered safe -- like local files.
is -> are
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:931
> + switch (processModel) {
> + case WEBKIT_PROCESS_MODEL_SHARED_SECONDARY_PROCESS:
> + newProcessModel = ProcessModelSharedSecondaryProcess;
> + break;
> + case WEBKIT_PROCESS_MODEL_MULTIPLE_SECONDARY_PROCESSES:
> + newProcessModel = ProcessModelMultipleSecondaryProcesses;
> + break;
> + default:
> + g_assert_not_reached();
> + }
I think we can use the same values and add a compile assert macro to make sure they match. That way we don't need the switches.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:945
> + * check the documentation of the function
> + * webkit_web_context_set_process_model().
the function will be a link in the doc, so you can simply say
For more information about this value see webkit_web_context_set_process_model().
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:948
> + */
Since: 2.4
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:80
> + */
Since: 2.4
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:84
> +typedef enum {
> + WEBKIT_PROCESS_MODEL_SHARED_SECONDARY_PROCESS,
> + WEBKIT_PROCESS_MODEL_MULTIPLE_SECONDARY_PROCESSES
> +} WebKitProcessModel;
I have some doubts about the names. This is actually about web processes, so maybe it should be called WebKitWebProcessModel, but it's true that all other processes are transparent from the API point of view, so it's probably fine to use WebKitProcessModel. MULTIPLE_SECONDARY_PROCESSES is too generic, I'm thinking of a possible future model of one process per security domain or something like that. So we might call the current one WEBKIT_PROCESS_MODEL_SECONDARY_PROCESS_PER_WEB_VIEW or something like that. What do you think?
> Tools/MiniBrowser/gtk/main.c:244
> + const gchar *minibrowser = g_getenv("MINIBROWSER_MULTIPROCESS");
The name of the variable is a bit confusing, I would use multiprocess instead.
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestMultiprocess.cpp:27
> +class MultiprocessTest;
> +static void loadChanged(WebKitWebView*, WebKitLoadEvent, MultiprocessTest*);
Add the callback as a static method of the class and you don't need this forward declarations.
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestMultiprocess.cpp:31
> +static guint initializeWebExtensionsSignalCount = 0;
> +static WebKitTestBus* bus = nullptr;
You don't need to initialize globals to 0. Use unsigned instead of guint.
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestMultiprocess.cpp:35
> + static const guint NUM_VIEWS = 2;
Use unsigned. The name of the variable should be numViews or viewsCount but in camel case.
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestMultiprocess.cpp:41
> + : m_mainLoop(g_main_loop_new(0, TRUE))
Use nullptr
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestMultiprocess.cpp:46
> + for (guint i = 0; i < NUM_VIEWS; i++) {
> + m_webView[i] = nullptr;
> + m_webViewId[i] = 0;
> + }
If you use Vectors, you don't need this.
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestMultiprocess.cpp:52
> + for (guint i = 0; i < NUM_VIEWS; i++)
> + g_object_unref(m_webView[i]);
Nor this either.
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestMultiprocess.cpp:56
> + void setupWebView(guint index)
Use unsigned. This is called setupWebView, but loads something and spins a main loop. I would call this loadWebViewAndWaitUntilFinished()
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestMultiprocess.cpp:58
> + g_assert(index < NUM_VIEWS);
g_assert_cmpuint(index, <, maxViewsCount);
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestMultiprocess.cpp:60
> + m_webView[index] = WEBKIT_WEB_VIEW(g_object_ref_sink(webkit_web_view_new()));
Use assertObjectIsDeletedWhenTestFinishes() to make sure we don't leak the views. We can use GrefPtr without adoptGRef to sink the floating ref.
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestMultiprocess.cpp:66
> + guint webProcessPid(guint index)
Use unsigned.
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestMultiprocess.cpp:68
> + g_assert(index < NUM_VIEWS);
g_assert_cmpuint(index, <, maxViewsCount);
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestMultiprocess.cpp:72
> + GOwnPtr<gchar> busName(g_strdup_printf("org.webkit.gtk.WebExtensionTest%u", m_webViewId[index]));
The web view id is only used for this, we could save the bus name directly. Vector<GUniquePtr<char>, 2> m_webViewBusNames;
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestMultiprocess.cpp:81
> + -1, 0, 0));
Use nullptr
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestMultiprocess.cpp:90
> + guint m_webViewId[2];
This could be Vector<GUniquePtr<char>, 2> m_webViewBusNames; as I suggested before.
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestMultiprocess.cpp:91
> + WebKitWebView* m_webView[2];
And this Vector<GRefPtr<WebKitWebView>, 2> m_webViews;
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestMultiprocess.cpp:101
> +static void loadChanged(WebKitWebView* webView, WebKitLoadEvent loadEvent, MultiprocessTest* test)
> +{
> + if (loadEvent != WEBKIT_LOAD_FINISHED)
> + return;
> + g_signal_handlers_disconnect_by_func(webView, reinterpret_cast<void*>(loadChanged), test);
> + g_main_loop_quit(test->m_mainLoop);
> +}
Move this to the class
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestMultiprocess.cpp:113
> + test->setupWebView(0);
> + test->setupWebView(1);
Move the asserts you are doing in webProcessPid here.
test->loadWebViewAndWaitUntilFinished(0);
g_assert(WEBKIT_IS_WEB_VIEW(test->m_webViews[0].get()));
g_assert(test->m_webViewBusNames[0]);
test->loadWebViewAndWaitUntilFinished(1);
g_assert(WEBKIT_IS_WEB_VIEW(test->m_webViews[1].get()));
g_assert(test->m_webViewBusNames[1]);
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestMultiprocess.cpp:115
> + g_assert_cmpuint(initializeWebExtensionsSignalCount, ==, 2);
We can make NUM_VIEWS constant global and use it here too.
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp:272
> +static gchar* makeBusName(GVariant* userData)
GUniquePtr<char>
> Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp:291
> + GOwnPtr<gchar> busName(makeBusName(userData));
GUniquePtr<char>
--
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