[Webkit-unassigned] [Bug 134551] [GTK] Move user style sheet API out of WebKitWebViewGroup

Thu Jul 3 04:14:30 PDT 2014

https://bugs.webkit.org/show_bug.cgi?id=134551

--- Comment #4 from Carlos Garcia Campos <cgarcia at igalia.com>  2014-07-03 04:14:47 PST ---
(From update of attachment 234330)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.