Bug 151473

Summary: [GTK] Reimplement webkit_web_context_[get,set]_process_model
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKitGTKAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED WONTFIX    
Severity: Normal CC: achristensen, bugs-noreply, mcatanzaro, ossy
Priority: P2    
Version: Other   
Hardware: PC   
OS: Linux   
Bug Depends on: 151418    
Bug Blocks:    
Attachments:
Description Flags
Patch cgarcia: review-

Description Michael Catanzaro 2015-11-19 18:46:18 PST
Reimplement webkit_web_context_[get,set]_process_model() to keep the API working as before. We can implement WEBKIT_PROCESS_MODEL_SHARED_SECONDARY_PROCESS by setting a process limit of 1.
Comment 1 Michael Catanzaro 2015-11-19 20:17:06 PST
This applies on top of the patch in bug #151418.
Comment 2 Michael Catanzaro 2015-11-19 20:17:17 PST
Created attachment 265935 [details]
Patch
Comment 3 Carlos Garcia Campos 2015-11-19 23:26:13 PST
Comment on attachment 265935 [details]
Patch

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

The original patch has been rolled out, maybe we can land this as part of the network process option removal instead of a follow up patch.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1149
> +        if (context->priv->context->maximumNumberOfProcesses() != 1)
> +            context->priv->context->setMaximumNumberOfProcesses(1);

Don't need to check the value didn't change, this simply sets the value, there's no ipc traffic or any other thing to avoid here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1153
> +        if (context->priv->context->maximumNumberOfProcesses() == 1)
> +            context->priv->context->setMaximumNumberOfProcesses(0);

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1156
> +    default:
> +        ASSERT_NOT_REACHED();

Just remove the default, since there aren't more values in the enum. That way we will get a compile warning/error if we add a new value and it's not handled here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1174
> +    return context->priv->context->maximumNumberOfProcesses() == 1 ?
> +        WEBKIT_PROCESS_MODEL_SHARED_SECONDARY_PROCESS : WEBKIT_PROCESS_MODEL_MULTIPLE_SECONDARY_PROCESSES;

This is not accurate. You can do set_process_model (MULTIPLE_SECONDARY_PROCESSES); set set_web_process_count_limit(1) and then get_process_model will return SHARED_SECONDARY_PROCESS. What we set with the setter is what we should return in the getter, the fact that now it's implemented changing the limit should be an implementation detail not exposed to the users.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1186
> - * The default value is 0 and means no limit.
> + * The default value is 1. If you change the process model to
> + * %WEBKIT_PROCESS_MODEL_MULTIPLE_SECONDARY_PROCESSES, then the process count
> + * limit will be removed. Setting the process count limit to a value other than
> + * 1 automatically changes the process model to %WEBKIT_PROCESS_MODEL_MULTIPLE_SECONDARY_PROCESSES.

Don't change this either. We should just keep the current model cached in the instance and return that in the getter, and here we could just return early if the process model is SHARED_SECONDARY_PROCESS and in the getter we always return 0 when process model is SHARED_SECONDARY_PROCESS even if it's actually 1 internally.
Comment 4 Michael Catanzaro 2015-11-20 06:00:35 PST
(In reply to comment #3)
> Don't change this either. We should just keep the current model cached in
> the instance and return that in the getter, and here we could just return
> early if the process model is SHARED_SECONDARY_PROCESS and in the getter we
> always return 0 when process model is SHARED_SECONDARY_PROCESS even if it's
> actually 1 internally.

OK, if that's what you prefer. You did that in bug #151418, so we don't need this bug anymore.