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

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted 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 162840: Patch
https://bugs.webkit.org/attachment.cgi?id=162840&action=review

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


Looks good to me, please fix the minor issues I mentioned before landing, and
consider adding a test case to check the user agent in the HTTP headers.

> Source/WebKit2/ChangeLog:34
> +

It seems changes in WebKitWebView are not mentioned in the ChangeLog.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1025
> +	* The user-agent string used by WebKit in the headers. Unusual
user-agent strings

in the HTTP headers?

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2613
> + * @user_agent: (allow-none) The new custom user agent string or %NULL to
use the default user agent

: is missing after the annotation

@user_agent: (allow-none): The new custom user agent string or %NULL to use the
default user agent

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2634
> + * @application_name: (allow-none) The application name used for the user
agent or %NULL to use the default user agent.
> + * @application_version: (allow-none) The application version for the user
agent or %NULL to user the default version.

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2638
> + * Set the #WebKitSettings:user-agent property by appending the application
details to the default user
> + * agent. If no application name or version is given, the default user agent
used will be used. If only
> + * the version is given, the default engine version is used with the given
application name.

Yes this is what I meant by moving the explanation to the body, thanks.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitSettings.cpp:289
> +}
> +

It would be great to include a test case to check that the expected user agent
is actually included in the HTTP headers, it could be a simple test similar to
the one used for languages.