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.
Created attachment 234713 [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
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
Created attachment 234799 [details] Patch
Created attachment 234849 [details] Patch
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.
I agree with Carlos' API suggestions so your new patch should be good to me ;)
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
Created attachment 235282 [details] Patch
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
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
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.
Created attachment 235677 [details] Patch
Comment on attachment 235677 [details] Patch Ok, let's try it out! Thank you
Comment on attachment 235677 [details] Patch Clearing flags on attachment: 235677 Committed r171742: <http://trac.webkit.org/changeset/171742>
All reviewed patches have been landed. Closing bug.