Bug 133729 – [GTK] Remove WebKitWebViewGroup from WebKit2 GTK+ API

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

In favor of adding a new user content controller/manager class for the user stylesheets and user scripts and leaving WebKitSettings as it was before we added groups to the API. We added this late to our API, and hopefully nobody except epiphany uses it, so it shouldn't be that problematic.

[Adrian Perez]

Created attachment 234713 [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

[Carlos Garcia Campos]

Comment on attachment 234713 [details]
Patch

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

[Adrian Perez]

Created attachment 234799 [details]
Patch

[Adrian Perez]

Created attachment 234849 [details]
Patch

[Adrian Perez]

The latest version of the patch has webkit_web_view_new_with_settings(),
and the WebKitWebView tests are updated to check that it properly sets
the view settings object on construction.

[Gustavo Noronha (kov)]

I agree with Carlos' API suggestions so your new patch should be good to me ;)

[Carlos Garcia Campos]

Comment on attachment 234849 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:-1220
> -WebPreferences* webkitSettingsGetPreferences(WebKitSettings* settings)
> -{
> -    return settings->priv->preferences.get();
> -}

I guess we can leave this here no?

> Source/WebKit2/UIProcess/API/gtk/WebKitSettingsPrivate.h:32
> -WebKit::WebPreferences* webkitSettingsGetPreferences(WebKitSettings*);
> +WebKit::WebPreferences* webkitSettingsGetWebPreferences(WebKitSettings*);

Why renaming this?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:377
> -    webView->priv->settings = webkit_web_view_get_settings(webView);
> +    webView->priv->settings = settings;

We should emit notify::settings here at some point.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:382
>      WebPageProxy* page = getPage(webView);
> -    page->setCanRunModal(webkit_settings_get_allow_modal_dialogs(settings));
> -    page->setCustomUserAgent(String::fromUTF8(webkit_settings_get_user_agent(settings)));
> +    page->setPreferences(*webkitSettingsGetWebPreferences(webView->priv->settings.get()));
> +    page->setCanRunModal(webkit_settings_get_allow_modal_dialogs(webView->priv->settings.get()));
> +    page->setCustomUserAgent(String::fromUTF8(webkit_settings_get_user_agent(webView->priv->settings.get())));

We can't do this here if webkit_web_view_set_settings is called during construction, because the page hasn't been created yet. We should return early here if page is NULL. Maybe we can leave this method as webkitWebViewUpdateSettings or even webkitWebViewSetupSettings (correctly handling a possible null page) and call it from both webkit_web_view_set_settings and webkitWebViewConstructed().

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:384
>      webkitWebViewBaseUpdatePreferences(WEBKIT_WEB_VIEW_BASE(webView));

We should probably change webkitWebViewBaseUpdatePreferences to use WebPageProxy::preferences() directly instead of getting the prtefs from the page group.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:497
> +    WebPageProxy* page = getPage(webView);
> +    page->setCanRunModal(webkit_settings_get_allow_modal_dialogs(webView->priv->settings.get()));
> +    page->setCustomUserAgent(String::fromUTF8(webkit_settings_get_user_agent(webView->priv->settings.get())));

This is already done by webkitWebViewSetSettings (or Update/SetupSettings if we rename it)

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:-522
> -    webkitWebViewUpdateSettings(webView);

We need this. I think we can call it SetupSettings, and remove the settings assignation from there, since it will be done by webkitWebViewConstructed or webkit_web_view_set_settings

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:529
> -    case PROP_GROUP: {
> -        gpointer group = g_value_get_object(value);
> -        webView->priv->group = group ? WEBKIT_WEB_VIEW_GROUP(group) : 0;
> +    case PROP_SETTINGS: {
> +        gpointer settings = g_value_get_object(value);
> +        webView->priv->settings = settings ? WEBKIT_SETTINGS(settings) : nullptr;

This doesn't work for the settings, because it's not a construct only parameter, we should call webkit_web_view_set_settings() here (only when != nullptr) to make sure g_object_set() has the same effects as webkit_web_view_set_settings. Here we are not disconnecting, and connecting the settings, etc.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:667
> +            static_cast<GParamFlags>(WEBKIT_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY)));

This should be CONSTRUCT, not CONSTRUCT_ONLY, since you can set new settings after construction.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2483
> +    webkitWebViewSetSettings(webView, settings);

So, here you can do webView->priv->settings = settings + webkitWebViewSetSettings() + g_object_notify

[Adrian Perez]

Created attachment 235282 [details]
Patch

[Build Bot]

Comment on attachment 235282 [details]
Patch

Attachment 235282 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5051554012856320

New failing tests:
media/W3C/video/preload/preload_property_exists.html

[Build Bot]

Created attachment 235292 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Carlos Garcia Campos]

Comment on attachment 235282 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:37
> +#include "WebPageGroup.h"

Do we really need WebPageGroup?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:377
> +    // The "settings" property can be set on construction, and in that

s/can be/is set/

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:379
> +    // WebPageProxy has been created and we should do an early return.

s/and/so/ or ,

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:403
> +    // The "settings" property can be set on construction, and in that
> +    // case webkit_web_view_set_settings() will be called *before* the
> +    // WebKitSettings have been assigned and we should do an early return.
>      WebKitSettings* settings = webView->priv->settings.get();
> +    if (!settings)
> +        return;

I think in this case, this check could be done by the caller.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:537
> +        gpointer settings = g_value_get_object(value);
> +        if (settings)

if (gpointer settings = g_value_get_object(value))
    webkit_web_view_set_settings(webView, WEBKIT_SETTINGS(settings));

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2491
> +    webkitWebViewDisconnectSettingsSignalHandlers(webView);

We could check if settings is null here, since it's the only place where this is called and settings can be null.

[Adrian Perez]

Created attachment 235677 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 235677 [details]
Patch

Ok, let's try it out! Thank you

[WebKit Commit Bot]

Comment on attachment 235677 [details]
Patch

Clearing flags on attachment: 235677

Committed r171742: <http://trac.webkit.org/changeset/171742>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.