[webkit-reviews] review denied: [Bug 133729] [GTK] Remove WebKitWebViewGroup from WebKit2 GTK+ API : [Attachment 234713] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Adrian Perez
<aperez at igalia.com>'s request for review:
Bug 133729: [GTK] Remove WebKitWebViewGroup from WebKit2 GTK+ API
https://bugs.webkit.org/show_bug.cgi?id=133729

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

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=234713&action=review


We should also add ettings as a construct property nof the web view, and set
the WebPageConfiguration::preferences pointer in
webkitWebViewBaseCreateWebPage. We should also add
webkit_web_view_new_with_settings() and a test case for it. This way a program
using a global WebKitSettings object (which is a very typical use case), can
create all the web views with the global settings as construct parameter and
settings are set without additional IPC messages

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:79
> + * color, font sizes, printing mode, script support, loading of images and
various other things

and various other things on a #WebKitWebView.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1220
> +    page->pageGroup().setPreferences(settings->priv->preferences.get());

I don't think we need to do this now, we should call page->setPreferences()
instead. Otherwise these settings will be shared by all web views in the
default group

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1057
> -    webkitWebViewBaseCreateWebPage(webViewBase,
context->priv->context.get(), pageGroup, userContentControllerProxy,
relatedPage);
> +    webkitWebViewBaseCreateWebPage(webViewBase,
context->priv->context.get(), nullptr, userContentControllerProxy,
relatedPage);

You should add also a settings parameter, to be able to create the page with
settings.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:-135
> -    PROP_GROUP,

Add settings property

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:501
> +    GRefPtr<WebKitSettings> settings = adoptGRef(webkit_settings_new());
> +    webkitWebViewSetSettings(webView, settings.get());

Here you should check if the settings have been set during object construction,
and create a new settings object only when settings is NULL at this point. I
think we can get rid of SetSettings now, and call
webkitSettingsAttachSettingsToPage() here directly

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:-546
> -    case PROP_GROUP: {
> -	   gpointer group = g_value_get_object(value);
> -	   webView->priv->group = group ? WEBKIT_WEB_VIEW_GROUP(group) : 0;
> -	   break;
> -    }

We should do the same for the settings now

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:-570
> -    case PROP_GROUP:
> -	   g_value_set_object(value, webkit_web_view_get_group(webView));
> -	   break;

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:-681
> -	       static_cast<GParamFlags>(WEBKIT_PARAM_READWRITE |
G_PARAM_CONSTRUCT_ONLY)));

Settings should be readwrite and construct

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1870
> - * Creates a new #WebKitWebView with the default #WebKitWebContext and the
> - * default #WebKitWebViewGroup.
> - * See also webkit_web_view_new_with_context() and
webkit_web_view_new_with_group().
> + * Creates a new #WebKitWebView with the default #WebKitWebContext and
> + * no #WebKitUserContentManager associated with it.
> + * See also webkit_web_view_new_with_context() and
> + * webkit_web_view_new_with_user_content_manager().

You should also add webkit_web_view_new_with_settings and mention it here as
well.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1885
> - * Creates a new #WebKitWebView with the given #WebKitWebContext and the
> - * default #WebKitWebViewGroup.
> - * See also webkit_web_view_new_with_group().
> + * Creates a new #WebKitWebView with the given #WebKitWebContext and
> + * no #WebKitUserContentManager associated with it.
> + * See also webkit_web_view_new_with_user_content_manager().

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:-1985
>  /**
> - * webkit_web_view_new_with_group:
> - * @group: a #WebKitWebViewGroup
> - *
> - * Creates a new #WebKitWebView with the given #WebKitWebViewGroup.
> - * The view will be part of @group and it will be affected by the
> - * group properties like the settings.
> - *
> - * Returns: The newly created #WebKitWebView widget
> - */
> -GtkWidget* webkit_web_view_new_with_group(WebKitWebViewGroup* group)
> -{
> -    g_return_val_if_fail(WEBKIT_IS_WEB_VIEW_GROUP(group), 0);
> -
> -    return GTK_WIDGET(g_object_new(WEBKIT_TYPE_WEB_VIEW, "group", group,
NULL));
> -}

We should do this for the settings

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2442
> + * the desired preferences, and the replace the existing @web_view

and the -> and then