[webkit-reviews] review denied: [Bug 133730] [GTK] Introduce a new WebKitUserContent API : [Attachment 233651] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jun 25 03:36:59 PDT 2014 

Carlos Garcia Campos <cgarcia at igalia.com> has denied Adrian Perez
<aperez at igalia.com>'s request for review:
Bug 133730: [GTK] Introduce a new WebKitUserContent API
https://bugs.webkit.org/show_bug.cgi?id=133730

Attachment 233651: Patch
https://bugs.webkit.org/attachment.cgi?id=233651&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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>(manage
r, 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.

More information about the webkit-reviews mailing list