[Webkit-unassigned] [Bug 133730] [GTK] Introduce a new WebKitUserContent API
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jun 25 03:37:03 PDT 2014
https://bugs.webkit.org/show_bug.cgi?id=133730
Carlos Garcia Campos <cgarcia at igalia.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #233651|review? |review-
Flag| |
--- Comment #8 from Carlos Garcia Campos <cgarcia at igalia.com> 2014-06-25 03:37:20 PST ---
(From update of attachment 233651)
View in context: https://bugs.webkit.org/attachment.cgi?id=233651&action=review
Looks good in general, I think we can simplify it by not exposing getters for now. We still need unit tests! :-)
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:32
> +
> +
Two empty lines here.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:46
> +
> +
We usually leave only one empty line between functions.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:140
> + UserScript* script;
> + UserStyleSheet* styleSheet;
Isn't it possible to use std::unique_ptr here?
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:158
> + * Obtains a copy of the source for an user content object. The returned
> + * string must be freed using g_free().
> + *
> + * Returns: (transfer full): An UTF-8 string.
Why returning a copy? It would be more convenient to return a const char*
Since: 2.6
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:183
> + */
Since: 2.6
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:218
> + for (auto str = *strv; str != nullptr; ++str)
Don't compare to nullptr
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:236
> + */
Since: 2.6
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:248
> + switch (userContent->priv->type) {
> + case UserContentScript:
> + return toStrV(userContent->priv->script->whitelist());
> + case UserContentStyleSheet:
> + return toStrV(userContent->priv->styleSheet->whitelist());
> + default:
> + ASSERT_NOT_REACHED();
> + return nullptr;
I'm actually wondering how useful it would be to add all those getters, I think user content are objects that the user creates to inject them, but that are not needed to be queried. In the current API they are parameters of a function. I think we could simplify this by avoinfing all the getters for now, and if they are needed eventually, we can add them later.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:283
> +struct _WebKitUserScriptPrivate {
> +};
Do not include an empty Private struct, use G_DEFINE_TYPE instead of WEBKIT_DEFINE_TYPE if we don't really need a Private struct
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:309
> + */
Since: 2.6
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:311
> +WebKitUserScript* webkit_user_script_new(const gchar* source, WebKitUserContentInjectedFrames injectedFrames,
> + WebKitUserScriptInjectionTime injectionTime, const char* const* whitelist, const char* const* blacklist)
This should be one line.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:313
> + WebKitUserContent* userContent = WEBKIT_USER_CONTENT(g_object_new(WEBKIT_TYPE_USER_SCRIPT, nullptr));
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:319
> + return WEBKIT_USER_SCRIPT(userContent);
You don't need to cast again.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:340
> +struct _WebKitUserStyleSheetPrivate {
> +};
Do not include an empty Private struct, use G_DEFINE_TYPE instead of WEBKIT_DEFINE_TYPE if we don't really need a Private struct. hmm, since the common stuff is already in the parent, I wonder whether it would be ebtter to expose these as boxed types instead
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:366
> + */
Since: 2.6
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:368
> +WebKitUserStyleSheet* webkit_user_style_sheet_new(const gchar* source, WebKitUserContentInjectedFrames injectedFrames,
> + WebKitUserStyleLevel level, const char* const* whitelist, const char* const* blacklist)
Single line
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:376
> + return WEBKIT_USER_STYLE_SHEET(userContent);
Double cast, actually same comments than before :-)
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:396
> +const UserScript& webkitWebKitUserScriptToUserScript(WebKitUserScript* userScript)
webkitUserScriptGetUserScript()
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:401
> +const UserStyleSheet& webkitWebKitUserStyleSheetToUserStyleSheet(WebKitUserStyleSheet* userStyleSheet)
Ditto.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.h:42
> + */
Since: 2.6
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.h:148
> + */
Since: 2.6
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:24
> +#include "WebKitJavascriptResult.h"
> +#include "WebKitJavascriptResultPrivate.h"
WebKitJavascriptResultPrivate.h already includes WebKitJavascriptResult.h
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:25
> +#include "WebKitPrivate.h"
This is also included by all Private.h headers
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:33
> +#include <WebCore/bindings/js/SerializedScriptValue.h>
Use #include <WebCore/SerializedScriptValue.h>
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:34
> +#include <WebCore/page/UserScript.h>
#include <WebCore/UserScript.h>
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:35
> +#include <WebCore/page/UserStyleSheet.h>
#include <WebCore/UserStyleSheet.h>
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:42
> +
> +
Double line
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:77
> +
> +
double line
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:89
> + ScriptMessageClientGtk(const GRefPtr<WebKitUserContentManager>& manager, const gchar* handlerName)
Use the pointer instead of const GRefPtr<WebKitUserContentManager>&
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:97
> + auto webView = webkitWebContextGetWebViewForPage(webkit_web_context_get_default(), &page);
Yes, it's not correct to use the default web context here, use page.viewWidget() instead.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:99
> + auto jsValue = WebSerializedScriptValue::create(&jsSerializedValue);
> + auto jsResult = adoptGRef(webkitJavascriptResultCreate(webView, jsValue.get()));
Add a new constructor for WebKitJavaScriptResult that receives a WebCore::SerializedScriptValue& directly.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:100
> + g_signal_emit(m_manager.get(), signals[SCRIPT_MESSAGE_RECEIVED], m_handlerName, jsResult.get());
Maybe we should also include the web view as parameter of the signal, since the manager can be shared with multiple view, we don't know which view is associated to this particular message, no?
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:125
> + */
Since: 2.6
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:142
> + * Returns: (transfer none): a #WebKitUserContentManager
Transfer full
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:143
> + */
Since: 2.6
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:154
> + * Adds a script to an user content manager. The same #WebKitUserScript can
Adds a #WebKitUserScript to the given #WebKitUserContentManager.
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:156
> + */
Since: 2.6
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:158
> +void
> +webkit_user_content_manager_add_script(WebKitUserContentManager* manager, WebKitUserScript* script)
Single line
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:169
> + * Removes all user scripts from the user content manager.
Removes all user scripts from @manager or from the #WebKitUserContentManager
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:170
> + */
Since: 2.6
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:172
> +void
> +webkit_user_content_manager_remove_all_scripts(WebKitUserContentManager* manager)
Single line
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:184
> + * Adds a script to an user content manager. The same #WebKitUserScript can
> + * be reused with multiple managers.
copy paste here, use style sheet instead of script
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:185
> + */
Since: 2.6
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:187
> +void
> +webkit_user_content_manager_add_style_sheet(WebKitUserContentManager* manager, WebKitUserStyleSheet* stylesheet)
Single line
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:201
> + * Removes all user style sheets from the user content manager.
> + */
> +void
> +webkit_user_content_manager_remove_all_style_sheets(WebKitUserContentManager* manager)
Same comments here
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:213
> + * Registers a new user script message handler name. After it is
s/name//
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:216
> + * to the WebKitUserContentManager::script-message-received signal. The
#WebKitUserContentManager::script-message-received
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:234
> + */
Since: 2.6
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:238
> + g_return_val_if_fail(name != nullptr, FALSE);
Do not compare to nullptr
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:240
> + const auto handler_name = String::fromUTF8(name);
handler_name -> handlerName
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:241
> + auto handler = WebScriptMessageHandler::create(std::make_unique<ScriptMessageClientGtk>(manager, name), handler_name);
You can use String::fromUTF8(name) directly here without a local variable
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:253
> + * WebKitUserContentManager::script-message-received signal:
#WebKitUserContentManager::script-message-received signal
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:258
> + * See: webkit_user_content_manager_register_script_message_handler()
s/See:/See also/
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:259
> + */
Since: 2.6
> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:269
> + g_return_val_if_fail(WEBKIT_IS_USER_CONTENT_MANAGER(manager), nullptr);
Don't use g_return macros in private methods, use assert instead if you want to be extra sure
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:687
> + */
Since: 2.6
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1971
> + *
You should add a brief explanation here
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1973
> + */
Since: 2.6
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2018
> + * 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:2021
> + * Returns: (transfer none): the #WebKitUserContentManager associated with
> + * the view
Use a single line here.
--
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