Summary: | [GTK] Add API to allow setting the process model in WebKitWebContext | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adrian Perez <aperez> | ||||||||||||||||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | andersca, ap, barraclough, bunhere, cdumez, cgarcia, commit-queue, gustavo, gyuyoung.kim, mrobinson, pnormand, rakuco, zan | ||||||||||||||||||||
Priority: | P2 | Keywords: | Gtk | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Bug Depends on: | 108832 | ||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||
Attachments: |
|
Description
Adrian Perez
2013-12-09 11:58:06 PST
Created attachment 218785 [details]
Patch
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 Unit test is missing, I will be updated the patch soon to add an unit test for the process model setting. Created attachment 218982 [details]
Patch
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.
Created attachment 218989 [details]
Patch
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. 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! (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. Created attachment 221792 [details]
Patch
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>. Created attachment 221848 [details]
Patch
(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(). 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> (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. Created attachment 222273 [details]
Patch
(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. (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. Created attachment 222275 [details]
Patch
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. 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. (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. (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 :) Created attachment 222323 [details]
Patch
Created attachment 222421 [details]
Patch
Comment on attachment 222421 [details] Patch Clearing flags on attachment: 222421 Committed r162928: <http://trac.webkit.org/changeset/162928> All reviewed patches have been landed. Closing bug. |