Bug 134551

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.

Created attachment 234330 [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 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.

Created attachment 234337 [details] Patch

Created attachment 234338 [details] Patch

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.

Created attachment 234340 [details] Patch

Created attachment 234342 [details] Patch

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.

Created attachment 234361 [details] Patch

(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.

Comment on attachment 234361 [details] Patch WK2 parts look good to me!

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()

Created attachment 234486 [details] Patch

Comment on attachment 234486 [details] Patch LGTM, thanks! We need another GTK+ reviewer to approve the new API, though

Created attachment 234561 [details] Patch

(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().

Comment on attachment 234561 [details] Patch The new API looks good to me.

Comment on attachment 234561 [details] Patch Ok.

Comment on attachment 234561 [details] Patch Clearing flags on attachment: 234561 Committed r170891: <http://trac.webkit.org/changeset/170891>

All reviewed patches have been landed. Closing bug.