[webkit-reviews] review denied: [Bug 125463] [GTK] Add API to allow setting the process model in WebKitWebContext : [Attachment 221848] Patch

Fri Jan 24 00:48:08 PST 2014

Carlos Garcia Campos <cgarcia at igalia.com> has denied Adrian Perez
<aperez at igalia.com>'s request for review:
Bug 125463: [GTK] Add API to allow setting the process model in
WebKitWebContext
https://bugs.webkit.org/show_bug.cgi?id=125463

Attachment 221848: Patch
https://bugs.webkit.org/attachment.cgi?id=221848&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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>