[webkit-reviews] review denied: [Bug 95697] [WebKit2] [GTK] Add API for controlling the user agent : [Attachment 161944] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 95697: [WebKit2] [GTK] Add API for controlling the user agent
https://bugs.webkit.org/show_bug.cgi?id=95697

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

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=161944&action=review


I think this patch could be split in two, one to move the code to webcore and
update WebKit1, and another one adding wk2 api. Regarding the wk2 patch, the
logic to set user agent and application name is duplicated, because it's
already in WebPageProxy. I think we could have two different properties:
user-agent and application-name-for-user-agent. The view connects to notify
signal for both properties and simply calls WKPageSetCustomUserAgent() for
user-agent and WKPageSetApplicationNameForUserAgent() for
application-name-for-user-agent.

> Source/WebKit2/ChangeLog:11
> +	   in the lirary.

lirary -> library

> Source/WebCore/platform/gtk/UserAgentGtk.cpp:62
> +    uaOSVersion =  "Intel Mac OS X";

Extra space here after =

> Source/WebCore/platform/gtk/UserAgentGtk.cpp:64
> +    uaOSVersion =  "PPC Mac OS X";

Ditto.

> Source/WebCore/platform/gtk/UserAgentGtk.cpp:94
> +    return String::format("%s %s/%s", staticUA.utf8().data(),
> +			     applicationName.isEmpty() ? "WebKitGTK+" :
applicationName.utf8().data(), uaVersion.data());

We could use g_get_application_name() or g_get_prgname() since I guess we don't
want the localized name here, and maybe fallback to WebKitGTK+ if
g_get_prgname() returns NULL.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:51
> +    CString userAgentString;

userAgentString sounds a bit redundant, it's a CString, maybe just userAgent?

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1023
> +	* WebKitSettings:user-agent

Trailing : missing here.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1031
> +	* If this property is set to the empty string or NULL, it will revert
to the standard

NULL -> %NULL

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2597
> + * Get the #WebKitSettings::user-agent property.

:: is for signals, for properties use :

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2613
> + * @user_agent: The new custom user agent string

Add (allow-none) annotation. And you should comment here that when passing a
NULL user agent, sets the default one.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2615
> + * Set the #WebKitSettings::user-agent property.

#WebKitSettings::user-agent -> #WebKitSettings:user-agent

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2622
> +    CString newUserAgent = (!userAgent || !strlen(userAgent)) ?
WebCore::standardUserAgent("").utf8() : userAgent;

(userAgent && userAgent[0] != '\0') ? userAgent :
WebCore::standardUserAgent("").utf8()

To avoid using strlen().

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2627
> +    priv->userAgentString = newUserAgent;
> +    g_object_notify(G_OBJECT(settings), "user-agent");

This doesn't actually change the user agent, you should call
WKPageSetCustomUserAgent() with the new user agent, so the web view should
connect to notify::user-agent to update its user agent when the setting
changes.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2633
> + * @application_name: The application name used to create the user agent.

I don't think we should allow NULL for the application name here

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2635
> + * Set the #WebKitSettings::user-agent property by forming the standard user


#WebKitSettings::user-agent -> #WebKitSettings:user-agent

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2637
> + * string or null, this method sets the user agent property to the standard
user

null -> %NULL

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2638
> + * agent string with the application name "WebKitGTK+."

That's already the default user agent when this method is not called, so I
think we should enforce a non null value for the application name to actually
change the user agent based on the application name.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2641
> +void webkit_settings_set_user_agent_with_application_name(WebKitSettings*
settings, const char* applicationName)

webkit_settings_set_default_user_agent_with_application_name()?

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2651
> +    WebKitSettingsPrivate* priv = settings->priv;
> +    CString newUserAgent =
WebCore::standardUserAgent(String::fromUTF8(applicationName)).utf8();
> +    if (newUserAgent == priv->userAgentString)
> +	   return;
> +
> +    priv->userAgentString = newUserAgent;
> +    g_object_notify(G_OBJECT(settings), "user-agent");

We could have a helper function to share this code with
webkit_settings_set_user_agent(). 

set_user_agent (WebKitSettings* settings, const char* userAgent, const char*
applicationName)

webkit_settings_set_user_agent would call set_user_agent(settings, userAgent,
0)
and webkit_settings_set_default_user_agent_with_application_name would call
set_user_agent(settings, 0, applicationName);

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitSettings.cpp:264
> +    g_assert(g_strstr_len(defaultUserAgent.data(), -1, "Safari"));
> +    g_assert(g_strstr_len(defaultUserAgent.data(), -1, "Chromium"));
> +    g_assert(g_strstr_len(defaultUserAgent.data(), -1, "Chrome"));

Why not passing defaultUserAgent.length() instead of -1

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitSettings.cpp:268
> +    const char* funkyUserAgent = "Funky!";
> +    webkit_settings_set_user_agent(settings.get(), funkyUserAgent);
> +    g_assert_cmpstr(funkyUserAgent, ==,
webkit_settings_get_user_agent(settings.get()));

It would be nice to test also that setting a NULL user agent returns the
default user agent

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitSettings.cpp:277
> +    g_assert_cmpint(strlen(webCatUserAgent.data()), ==,
strlen(defaultUserAgent.data()));

could you use .length() instead of strlen?