Bug 134551 – [GTK] Move user style sheet API out of WebKitWebViewGroup

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Adrian Perez]

In order to be able to remove WebKitWebViewGroup (as per bug #133729)
we need to move the API to set user style sheets somewhere else.

[Adrian Perez]

I am working on a simplified version of the patch already attached to
bug #133730 which only includes the user style sheets parts. The rest
of the API for user content would be added later after this initial,
smaller patch.

[Adrian Perez]

Created attachment 234330 [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 234330 [details]
Patch

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

The API looks good to me, we need a WebKit2 owner for the cross-platform changes.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:25
> +#include <WebCore/page/UserScript.h>

This doesn't belong to this patch.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:26
> +#include <WebCore/page/UserStyleSheet.h>

This should be <WebCore/UserStyleSheet.h>

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:81
> +        : referenceCount(1) { }

Move the {} to their own lines:
        : referenceCount(1)
    {
    }

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:141
> + * Returns: A new #WebKitUserStyleSheet object

s/object// It's not exactly an object.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:146
> +WebKitUserStyleSheet* webkit_user_style_sheet_new(const gchar* source, WebKitUserContentInjectedFrames injectedFrames,
> +    WebKitUserStyleLevel level, const char* const* whitelist, const char* const* blacklist)

This should be one line

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:148
> +    WebKitUserStyleSheet* userStyleSheet = g_slice_new(WebKitUserStyleSheet);

You should check at least source is not NULL with a g_return macro. Are you sure we don't need the baseURI too?

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:154
> +    new (userStyleSheet) WebKitUserStyleSheet();
> +    userStyleSheet->userStyleSheet = std::make_unique<UserStyleSheet>(
> +        String::fromUTF8(source), URL { },
> +        toStringVector(whitelist), toStringVector(blacklist),
> +        toUserContentInjectedFrames(injectedFrames),
> +        toUserStyleLevel(level));

Maybe we pass the arguments to the WebKitUserStyleSheet constructor and create the WebCore::UserStyleSheet there.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:158
> +const UserStyleSheet& webkitWebKitUserStyleSheetToUserStyleSheet(WebKitUserStyleSheet* userStyleSheet)

webkitUserStyleSheetGetUserStyleSheet() or something like that

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:23
> +#include "WebKitPrivate.h"

This is already included by some of the other Private.h functions

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:27
> +#include "WebKitWebContextPrivate.h"
> +#include "WebPageProxy.h"

I don't think you need these ones.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:29
> +#include <WebCore/page/UserStyleSheet.h>

This should be <WebCore/UserStyleSheet.h>

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:77
> +WebKitUserContentManager* webkit_user_content_manager_new(void)

s/void//

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:87
> + * Adds a script to an user content manager. The same #WebKitUserScript can

s/script/style sheet/ 
s/WebKitUserScript/WebKitUserStyleSheet/

Adds a #WebKitUserStyleSheet to the given #WebKitUserContentManager.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:93
> +void
> +webkit_user_content_manager_add_style_sheet(WebKitUserContentManager* manager, WebKitUserStyleSheet* styleSheet)

This should be a single line

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:96
> +    g_assert(styleSheet);

Don't use g_assert here, since this is public API, use g_return_if_fail instead

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:104
> + * Removes all user style sheets from the user content manager.

Removes all user style sheets from @manager or from the #WebKitUserContentManager

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:109
> +void
> +webkit_user_content_manager_remove_all_style_sheets(WebKitUserContentManager* manager)

This should be a single line

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:117
> +    g_return_val_if_fail(WEBKIT_IS_USER_CONTENT_MANAGER(manager), nullptr);

Don't use g_return macros in private functions, use ASSERT instead if you want to be extra sure.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManagerPrivate.h:27
> +namespace WebKit {
> +class WebUserContentControllerProxy;
> +};

This is a private header you can simply include "WebUserContentControllerProxy.h" here and remove it from the cpp

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentPrivate.h:27
> +namespace WebCore {
> +class UserStyleSheet;
> +};

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:43
> +#include "WebKitUserContentManager.h"
> +#include "WebKitUserContentManagerPrivate.h"

ManagerPrivate.h already includes Manager.h and WebKitWebContextPrivate.h already includes WebKitUserContentManager.h

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:687
> +     */

Since: 2.6

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1988
> + *

You should add a brief explanation here

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1990
> + */

Since: 2.6

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2035
> + * Gets the user content manager associated to @web_view.

Or %NULL if the view doesn't have a user content manager.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2038
> + * Returns: (transfer none): the #WebKitUserContentManager associated with
> + *    the view

Use a single line here.

> Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt:112
> +

Remove this line

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:37
> +        m_userContentManager = webkit_web_view_get_user_content_manager(m_webView);
> +        g_object_unref(m_userContentManager);

This is confusing, I understand that we are actually leaking the ref in webkit_web_view_new_with_user_content_manager, and here we fix that leak. I think I would use a GRefPtr with adoptGRef and a comment explaining why we are adopting a reference form a method that is transfer none

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:61
> +    g_assert(webkit_web_view_get_user_content_manager(webView2.get()) != userContentManager1.get());
> +    g_assert(webkit_web_view_get_user_content_manager(webView2.get()) == nullptr);

Don't compare to nullptr. I think these asserts are redundant, g_assert(!webkit_web_view_get_user_content_manager(webView2.get())); should be enough to check that a web view created without a content manager doesn't have a conent manager and it's obviously different than the content manager created manually.

[Adrian Perez]

Created attachment 234337 [details]
Patch

[Adrian Perez]

Created attachment 234338 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 234337 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:32
> +        : userContentController(WebUserContentControllerProxy::create()) { }

Move the {} to their own line.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentPrivate.h:24
> +

Remove this empty line.

[Adrian Perez]

Created attachment 234340 [details]
Patch

[Adrian Perez]

Created attachment 234342 [details]
Patch

[Anders Carlsson]

Comment on attachment 234342 [details]
Patch

This is definitely what we want to do in WebKit2 going forward. 

You also need to send over the style sheets in WebUserContentControllerProxy::addProcess.

[Adrian Perez]

Created attachment 234361 [details]
Patch

[Adrian Perez]

(In reply to comment #10)
> (From update of attachment 234342 [details])
> This is definitely what we want to do in WebKit2 going forward. 
> 
> You also need to send over the style sheets in WebUserContentControllerProxy::addProcess.

Thanks, this is added now.

[Anders Carlsson]

Comment on attachment 234361 [details]
Patch

WK2 parts look good to me!

[Carlos Garcia Campos]

Comment on attachment 234361 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:80
> +            String::fromUTF8(source), URL { },

Are you sure we don't need the baseURL?

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:33
> +        : userContentController(WebUserContentControllerProxy::create()) { }

Move the {} the their own line, please

    _WebKitUserContentManagerPrivate()
        : userContentController(WebUserContentControllerProxy::create())
    {
    }

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentPrivate.h:24
> +

Remove this empty line

> Source/WebKit2/UIProcess/UserContent/WebUserContentControllerProxy.cpp:103
> +    m_userStyleSheets.append(std::move(userStyleSheet));

Use WTF::move() instead of std::move()

[Adrian Perez]

Created attachment 234486 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 234486 [details]
Patch

LGTM, thanks! We need another GTK+ reviewer to approve the new API, though

[Adrian Perez]

Created attachment 234561 [details]
Patch

[Adrian Perez]

(In reply to comment #16)
> (From update of attachment 234486 [details])
> LGTM, thanks! We need another GTK+ reviewer to approve the new API, though

Great! Also, I have uploaded an update with a couple of small fixes:

 * WebCore.exp.in is now sorted (using Tools/Scripts/sort-export-file)
 * A bogus reference to webkit_user_content_get_type() in the macro
   WEBKIT_USER_STYLE_SHEET_TYPE is now changed to the correct 
   webkit_user_style_sheet_get_type().

[Martin Robinson]

Comment on attachment 234561 [details]
Patch

The new API looks good to me.

[Carlos Garcia Campos]

Comment on attachment 234561 [details]
Patch

Ok.

[WebKit Commit Bot]

Comment on attachment 234561 [details]
Patch

Clearing flags on attachment: 234561

Committed r170891: <http://trac.webkit.org/changeset/170891>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.