[Webkit-unassigned] [Bug 133730] [GTK] Introduce a new WebKitUserContent API
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jun 13 00:29:17 PDT 2014
https://bugs.webkit.org/show_bug.cgi?id=133730
Carlos Garcia Campos <cgarcia at igalia.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #232952|review? |
Flag| |
--- Comment #5 from Carlos Garcia Campos <cgarcia at igalia.com> 2014-06-13 00:29:39 PST ---
(From update of attachment 232952)
View in context: https://bugs.webkit.org/attachment.cgi?id=232952&action=review
Looks good so far. Removing the review flags since this is a wip patch.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:25
> +
Do not leave empty lines between header includes.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:78
> + _WebKitUserContentPrivate()
> + : injectedFrames(WEBKIT_USER_CONTENT_INJECT_ALL_FRAMES) { }
You don't really need the constructor, since WEBKIT_USER_CONTENT_INJECT_ALL_FRAMES is 0 and the struct is already filled with 0 when allocated.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:83
> + CString source;
> + Vector<String> whitelist;
> + Vector<String> blacklist;
> + WebKitUserContentInjectedFrames injectedFrames;
hmm, the problem of having this common class is that we can't wrap the WebCore classes directly, or maybe we could use an union or something like that.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:86
> +WEBKIT_DEFINE_TYPE(WebKitUserContent, webkit_user_content, G_TYPE_OBJECT)
This should be an abstract class.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:91
> + // Empty
Remove this comment :-)
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:97
> +void webkit_user_content_set_source(WebKitUserContent* user_content, const gchar* source)
I'm not sure about having set/get for all members, some of them (if not all) should probably be construct only, passed to the new method and then readonly. You should not be able to modify them once the object is created, I think.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:108
> +const gchar* webkit_user_content_get_source(WebKitUserContent* user_content)
user_content -> userContent everywhere :-)
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:135
> +void webkit_user_content_set_whitelist(WebKitUserContent* user_content, GList* whitelist)
I would use const char* const* for the lists instead of GList like our current API does.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:187
> + _WebKitUserScriptPrivate()
> + : injectionTime(WEBKIT_USER_SCRIPT_INJECT_AT_DOCUMENT_START) { }
You don't need this one either
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:202
> + * @source: (transfer none): Source code of the user script.
I think you don't need to use the transfer none annotation for parameters that are const
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:206
> +WebKitUserContent* webkit_user_script_new(const gchar* source)
I think we should receive all members as contruct parameters, and if possible create the WebCore::UserScript directly here instead of duplicating everything.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:231
> +UserScript webkitWebKitUserScriptToUserScript(WebKitUserScript* script, guint64 scriptId)
Why do we need a script id?
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:237
> + StringBuilder urlStringBuilder;
> + urlStringBuilder.append("user-script:");
> + urlStringBuilder.appendNumber(scriptId);
Why? Shouldn't the baseURL be provided by the user as well? The first append should be appendLiteral instead
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:240
> + String::fromUTF8(script->parent.priv->source.data(), script->parent.priv->source.length()),
Don't use parent.priv, use a cast instead and ->priv directly.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:241
> + URL { URL { }, urlStringBuilder.toString() },
I don't understand this, this is the base URL as provided by the user.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:252
> + _WebKitUserStyleSheetPrivate()
> + : level(WEBKIT_USER_STYLE_LEVEL_USER) { }
Unneeded too
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:274
> + userContent->priv->source = g_strdup(source);
This is a leak, CString doesn't adopt the pointer, but duplicates the string already.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:311
> +UserStyleSheet webkitWebKitUserStyleSheetToUserStyleSheet(WebKitUserStyleSheet* styleSheet, guint64 styleSheetId)
> +{
> + g_assert(WEBKIT_IS_USER_STYLE_SHEET(styleSheet));
> +
> + StringBuilder urlStringBuilder;
> + urlStringBuilder.append("user-style-sheet:");
> + urlStringBuilder.appendNumber(styleSheetId);
> +
> + return UserStyleSheet {
> + String::fromUTF8(styleSheet->parent.priv->source.data(), styleSheet->parent.priv->source.length()),
> + URL { URL { }, urlStringBuilder.toString() },
> + styleSheet->parent.priv->whitelist,
> + styleSheet->parent.priv->blacklist,
> + toUserContentInjectedFrames(styleSheet->parent.priv->injectedFrames),
> + toUserStyleLevel(styleSheet->priv->level)
> + };
Same comments here than for user script
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:24
> +#include "UserScript.h"
> +#include "UserStyleSheet.h"
Use <WebCore/Foo.h> for WebCore includes
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:36
> +class ScriptMessageClientGtk : public WebScriptMessageHandler::Client {
Mark the class as final
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:39
> + ScriptMessageClientGtk(const GRefPtr<WebKitUserContentManager>& manager,
> + const String& handlerName): m_handlerName(handlerName), m_manager(manager) { }
leave the parameters in the same line and move the initializations to new lines
ScriptMessageClientGtk(const GRefPtr<WebKitUserContentManager>& manager, const String& handlerName)
: m_handlerName(handlerName)
, m_manager(manager)
{
}
Also we should receive the pointer of WebKitUserContentManager
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:60
> + guint64 numUserStyleSheets;
> + guint64 numUserScripts;
Use uint64_t for internal types and fooCount instead of numFoo.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:139
> + manager->priv->userContentController->addUserScript(
> + webkitWebKitUserScriptToUserScript(script, manager->priv->numUserScripts++));
I think we should either wrap the WebCore::UserScript in WebKitUserScript and return a const reference here or build the WebCore::UserScript here using the WebKitUserScript public interface
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:164
> + manager->priv->userContentController->addUserStyleSheet(
> + webkitWebKitUserStyleSheetToUserStyleSheet(stylesheet, manager->priv->numUserStyleSheets++));
Ditto.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:181
> + * webkit_user_content_manager_register_script_message_handler_name:
I would remove the _name webkit_user_content_manager_register_script_message_handler()
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:202
> + * Returns: Whether the message channel was registered successfully.
You should explain why this can fail, and that it's not allowed to register the same name twice.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:207
> + g_return_val_if_fail(WEBKIT_IS_USER_CONTENT_MANAGER(manager), FALSE);
> +
We should also check the name is != NULL
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:214
> + * webkit_user_content_manager_unregister_script_message_handler_name:
Same here webkit_user_content_manager_unregister_script_message_handler()
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.h:62
> + void (*_webkit_reserved0) (void);
> + void (*_webkit_reserved1) (void);
> + void (*_webkit_reserved2) (void);
> + void (*_webkit_reserved3) (void);
> + void (*_webkit_reserved4) (void);
> + void (*_webkit_reserved5) (void);
> + void (*_webkit_reserved6) (void);
> + void (*_webkit_reserved7) (void);
I don't think we need all this padding, probably 4 is enough, considering that we don't have vmethods.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list