Bug 125463 – [GTK] Add API to allow setting the process model in WebKitWebContext

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Adrian Perez]

We need a new API to allow setting the process model in the GTK port.

This depends on https://bugs.webkit.org/show_bug.cgi?id=108832

[Adrian Perez]

Created attachment 218785 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Adrian Perez]

Unit test is missing, I will be updated the patch soon to add an unit test
for the process model setting.

[Adrian Perez]

Created attachment 218982 [details]
Patch

[WebKit Commit Bot]

Attachment 218982 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Tools/MiniBrowser/gtk/main.c', '--commit-queue']" exit_code: 1
ERROR: Tools/MiniBrowser/gtk/main.c:245:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h"
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adrian Perez]

Created attachment 218989 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 218989 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218989&action=review

Overall looks good but we need unit tests for new API, check http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

> Source/WebKit2/ChangeLog:7
> +

Short description needed here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:843
> + * %WEBKIT_PROCESS_MODEL_SHARED_SECONDARY_PROCESS is suitable for most

s/is/which is/ ?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:852
> + *

In general I think the API description shouldn't be like this. We should explain the difference of having just one or multiple web processes instead of describing the benefits, Also one process per view is not totally correct, new processes are created for new browsing contexts, we might explain what it means in the docs as well.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:859
> +void webkit_web_context_set_process_model(WebKitWebContext* context, WebKitProcessModel process_model)

processModel you have to use CamelCase. You should use a different name for the local variable as well.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:78
> + *

OK so this is the description I was looking for. Then I guess you should link this somehow in the webkit_web_context_set_process_model() docs.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:234
> +                                                     WebKitProcessModel             process_model);

processModel

> Tools/MiniBrowser/gtk/main.c:244
> +    const gchar *minibrowser = g_getenv("MINIBROWSER_MULTIPROCESS");

It'd be nice to know if Mac is using something like this and use the same variable name.

[Adrian Perez]

Changed dependencies. To make the unit tests with multiple web
processes what it is better is to be able to coordinate setting
up the ui-process <-> web-processes communications. For that, the
patch in bug #125990 is more suitable.

Sergio: I will be addressing the issues you comment and adding
the unit tests, thanks for the review!

[Adrian Perez]

(In reply to comment #7)
> (From update of attachment 218989 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218989&action=review
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:234
> > +                                                     WebKitProcessModel             process_model);
> 
> processModel

This is a public API header, so it follow the GObject convention of
using “lowercase_with_underscores” for argument names.
 
> > Tools/MiniBrowser/gtk/main.c:244
> > +    const gchar *minibrowser = g_getenv("MINIBROWSER_MULTIPROCESS");
> 
> It'd be nice to know if Mac is using something like this and use the same variable name.

It seems that multi-process mode is the default for Mac — there is no
toggle for changing the process mode in the Mac MiniBrowser.

[Adrian Perez]

Created attachment 221792 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 221792 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221792&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:910
> + * This method <strong>must be called before any other functions</string>,

Typo: </string>.

[Adrian Perez]

Created attachment 221848 [details]
Patch

[Adrian Perez]

(In reply to comment #11)
> (From update of attachment 221792 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221792&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:910
> > + * This method <strong>must be called before any other functions</string>,
> 
> Typo: </string>.

This is fixed in the new version of the patch I have just uploaded.
Also, this new version uses WTF::getCurrentProcessID() instead of
getpid().

[Carlos Garcia Campos]

Comment on attachment 221848 [details]
Patch

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>

[Adrian Perez]

(In reply to comment #14)
>
> > 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.

I would rather keep the switches, read on below…

> > 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?

Right, …_SECONDARY_PROCESS_PER_WEB_VIEW better describes what the
mode implies, so I also think it is a good idea to go with that name.
Provided that the names of process models exposed in the API are not
going to match the names used internally, and that the mapping is not
exactly 1:1 (example: SECONDARY_PROCESS_PER_WEB_VIEW also enables the
network process), I think that keeping the switches better represents
that — even when at the moment the mapping quite direct right now.

Apart that for the moment I am keeping the switches, the rest of the
issues should be solved in the new version of the patch I am uploading
in a moment.

[Adrian Perez]

Created attachment 222273 [details]
Patch

[Carlos Garcia Campos]

(In reply to comment #15)
> (In reply to comment #14)
> >
> > > 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.
> 
> I would rather keep the switches, read on below…
> 
> > > 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?
> 
> Right, …_SECONDARY_PROCESS_PER_WEB_VIEW better describes what the
> mode implies, so I also think it is a good idea to go with that name.
> Provided that the names of process models exposed in the API are not
> going to match the names used internally, and that the mapping is not
> exactly 1:1 (example: SECONDARY_PROCESS_PER_WEB_VIEW also enables the
> network process), I think that keeping the switches better represents
> that — even when at the moment the mapping quite direct right now.

That's why I suggested to add a compile assert, I don't think it's that important that the name matches exactly as long as it's obvious it's the same thing. The mapping is indeed 1:1, the network process is not a process model. WebKit internally expects the network process to be enabled when multiple web process model is set, there are several asserts to ensure that, see WebContext::networkingProcessConnection() for example.

> Apart that for the moment I am keeping the switches, the rest of the
> issues should be solved in the new version of the patch I am uploading
> in a moment.

[Adrian Perez]

(In reply to comment #17)
> (In reply to comment #15)
> > (In reply to comment #14)
> > >
> > > > 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.
> > 
> > I would rather keep the switches, read on below…
> > 
> > > > 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?
> > 
> > Right, …_SECONDARY_PROCESS_PER_WEB_VIEW better describes what the
> > mode implies, so I also think it is a good idea to go with that name.
> > Provided that the names of process models exposed in the API are not
> > going to match the names used internally, and that the mapping is not
> > exactly 1:1 (example: SECONDARY_PROCESS_PER_WEB_VIEW also enables the
> > network process), I think that keeping the switches better represents
> > that — even when at the moment the mapping quite direct right now.
> 
> That's why I suggested to add a compile assert, I don't think it's that important that the name matches exactly as long as it's obvious it's the same thing. The mapping is indeed 1:1, the network process is not a process model. WebKit internally expects the network process to be enabled when multiple web process model is set, there are several asserts to ensure that, see WebContext::networkingProcessConnection() for example.

Well, it's not like I have a strong preference for the switches, so I am
removing them. If they would be needed in the future, they would be added
later on, I presume.

[Adrian Perez]

Created attachment 222275 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 222275 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=222275&action=review

This looks great to me. We need the approval from another WebKitGTK+ reviewer, since this introduces new public API.

> Source/WebKit2/ChangeLog:10
> +        used. Setting WEBKIT_PROCESS_MODEL_MULTIPLE_SECONDARY_PROCESSES

WEBKIT_PROCESS_MODEL_SECONDARY_PROCESS_PER_WEB_VIEW

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:895
> +static_assert(WEBKIT_PROCESS_MODEL_SHARED_SECONDARY_PROCESS == ProcessModelSharedSecondaryProcess,
> +    "ProcessModel and WebKitProcessModel enum values do not have the same integral value");
> +static_assert(WEBKIT_PROCESS_MODEL_SECONDARY_PROCESS_PER_WEB_VIEW == ProcessModelMultipleSecondaryProcesses,
> +    "ProcessModel and WebKitProcessModel enum values do not have the same integral value");

We have macros for this, see COMPILE_ASSERT_MATCHING_ENUM in WebKitPrivate.h

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:910
> + * %WEBKIT_PROCESS_MODEL_MULTIPLE_SECONDARY_PROCESSES to use one auxiliary

WEBKIT_PROCESS_MODEL_SECONDARY_PROCESS_PER_WEB_VIEW

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:925
> +        context->priv->context->setUsesNetworkProcess(processModel == ProcessModelMultipleSecondaryProcesses);

We should probably remove the WEBKIT_USE_NETWORK_PROCESS env var.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:70
> + *   #WebKitWebView instances created by the application. If the secondary
> + *   This is the default process model, and it should suffice for most cases.

If the secondary This is the default?

> Tools/MiniBrowser/gtk/main.c:244
> +    const gchar *multiprocess= g_getenv("MINIBROWSER_MULTIPROCESS");

multiprocess= -> multiprocess =

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestMultiprocess.cpp:27
> +class MultiprocessTest;

Why do you need this?

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestMultiprocess.cpp:35
> +

Remove tis empty line.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestMultiprocess.cpp:105
> +    test->loadWebViewAndWaitUntilLoaded(0);
> +    g_assert(WEBKIT_IS_WEB_VIEW(test->m_webViews[0].get()));
> +    g_assert(test->m_webViewBusNames[0]);
> +
> +    test->loadWebViewAndWaitUntilLoaded(1);
> +    g_assert(WEBKIT_IS_WEB_VIEW(test->m_webViews[1].get()));
> +    g_assert(test->m_webViewBusNames[1]);

Now that I see this, I guess we can use a for loop.

[Gustavo Noronha (kov)]

Comment on attachment 222275 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=222275&action=review

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:910
>> + * %WEBKIT_PROCESS_MODEL_MULTIPLE_SECONDARY_PROCESSES to use one auxiliary
> 
> WEBKIT_PROCESS_MODEL_SECONDARY_PROCESS_PER_WEB_VIEW

API looks good to me, but can we add ONE before SECONDARY? Sounds a bit weird for me otherwise.

[Adrian Perez]

(In reply to comment #20)
> (From update of attachment 222275 [details])
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:895
> > +static_assert(WEBKIT_PROCESS_MODEL_SHARED_SECONDARY_PROCESS == ProcessModelSharedSecondaryProcess,
> > +    "ProcessModel and WebKitProcessModel enum values do not have the same integral value");
> > +static_assert(WEBKIT_PROCESS_MODEL_SECONDARY_PROCESS_PER_WEB_VIEW == ProcessModelMultipleSecondaryProcesses,
> > +    "ProcessModel and WebKitProcessModel enum values do not have the same integral value");
> 
> We have macros for this, see COMPILE_ASSERT_MATCHING_ENUM in WebKitPrivate.h

Certainly, I have changed the asserts to use the macro. Also, for
the sake of consistency with other files using those, I have moved
the asserts to the top of the “WebKitWebContext.cpp”.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:925
> > +        context->priv->context->setUsesNetworkProcess(processModel == ProcessModelMultipleSecondaryProcesses);
> 
> We should probably remove the WEBKIT_USE_NETWORK_PROCESS env var.

Indeed. Now that this is in place we can get rid of it. The next update
to the patch will have the environment variable check removed.

[Adrian Perez]

(In reply to comment #21)
> (From update of attachment 222275 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222275&action=review
> 
> >> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:910
> >> + * %WEBKIT_PROCESS_MODEL_MULTIPLE_SECONDARY_PROCESSES to use one auxiliary
> > 
> > WEBKIT_PROCESS_MODEL_SECONDARY_PROCESS_PER_WEB_VIEW
> 
> API looks good to me, but can we add ONE before SECONDARY? Sounds a bit weird for me otherwise.

Sure, no objection from me. Being it …ONE_SECONDARY_PROCESS_PER_WEB_VIEW
makes the meaning a bit more concrete, which is never a bad thing :)

[Adrian Perez]

Created attachment 222323 [details]
Patch

[Adrian Perez]

Created attachment 222421 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 222421 [details]
Patch

Clearing flags on attachment: 222421

Committed r162928: <http://trac.webkit.org/changeset/162928>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.