[webkit-reviews] review denied: [Bug 63994] [GTK] Google Calendar thinks we're mobile : [Attachment 100317] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Gustavo Noronha (kov)
<gns at gnome.org>'s request for review:
Bug 63994: [GTK] Google Calendar thinks we're mobile
https://bugs.webkit.org/show_bug.cgi?id=63994

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

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=100317&action=review


Looks pretty good to me, but I have a few nits.

> Source/WebKit/gtk/tests/testwebsettings.c:27
> +char* webkit_web_settings_get_user_agent_for_uri(WebKitWebSettings *, const
char* uri);

I'm surprised you can omit the argument name in a C file. In C files, WebKit
style is to put the asterisk near the variable name.

> Source/WebKit/gtk/tests/testwebsettings.c:64
> +    gchar* userAgent;
> +
> +    // test a custom UA string
> +    userAgent = NULL;

Might as well put this on one line with the asterisk near the variable name.
Please use 0 instead of NULL.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1401
> +    int position = host.find(".google.");
> +    if (position > 0 && googleDomains.contains(host.substring(position +
sizeof(".google.") - 1)))
> +	   return true;

This doesn't seem to verify that the string isn't somewhere in the middle of
the domain. I think it would be technically more correct to check add "google."
to all the domains above and simply check host.endsWith(domain).

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1460
> +/*
> + * webkit_web_settings_get_user_agent_for_uri:
> + * @web_settings: the #WebKitWebSettings object to query
> + * @uri: the URI we want to know the User-Agent for
> + *
> + * Some web sites have been using User-Agent parsing heavily to decide
> + * the kind of content that is sent to the browser. When
> + * WebKitWebSettings:enable-site-specific-quirks is enabled WebKitGTK+
> + * will use its knowledge of sites doing bad things and lie to them by
> + * sending either the default User-Agent, i.e. not using the one
> + * specified by the browser in WebKitWebSettings:user-agent, or the
> + * Safari one (including lying about the underlying operating system).
> + *
> + * This function allows the browser to figure out what User-Agent is
> + * going to be sent to a particular URI.
> + *
> + * Please note that if WebKitWebSettings:use-site-specific-quirks is
> + * turned off calling this function is the same as calling
> + * webkit_web_settings_get_user_agent(), except you have to free the
> + * result.
> + *
> + * Returns: (transfer full): a newly allocated string containing the
> + * User-Agent that will be sent for the given URI.
> + *
> + * Since: 1.5.2
> + */

This will show up in the documentation still right? We should remove the
documentation here, since this is private. Alternative: keep the documentation,
but do not use gtkdoc.