Bug 63994 – [GTK] Google Calendar thinks we're mobile

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Gustavo Noronha (kov)]

The subject says it all, Calendar delivers the mobile version even though we already special-case google domains, returning the default webkitgtk user agent. It only gives us the regular site if we lie about the operating system. I'll try to get it fixed in Google Calendar, but given the bad history accumulated by google and the time it takes for us to get to anyone when it comes to User-Agent parsing problems I'll propose a patch to special-case calendar further as well.

[Gustavo Noronha (kov)]

Created attachment 99833 [details]
Patch

[Xan Lopez]

Comment on attachment 99833 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99833&action=review

I think this looks OK but I'm not really sure about whether we should make the API public or not. Seems like a real corner case. What does Martin think? :)

> Source/WebKit/gtk/tests/testwebsettings.c:62
> +    g_object_set(G_OBJECT(settings), "user-agent", "testwebsettings/0.1", NULL);

For whatever reason g_object_set takes a gpointer, so you don't really need to cast I think? That was the nerd nitpick of the day.

> Source/WebKit/gtk/tests/testwebsettings.c:63
> +    g_object_get(G_OBJECT(settings),"user-agent", &userAgent, NULL);

And same here.

[Martin Robinson]

(In reply to comment #2)
> (From update of attachment 99833 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99833&action=review
> 
> I think this looks OK but I'm not really sure about whether we should make the API public or not. Seems like a real corner case. What does Martin think? :)

I agree with Xan that this should probably not be public. Right now it's an implementation detail that we can hopefully one day dispense with (I know -- wishful thinking).

I like how this is used in the test though, so perhaps we can add a hidden API or better yet, use webkit_web_view_load_string instead. webkit_web_view_load_string allows you to override the base URI so perhaps it will trigger the User-Agent string quirks.

[Alexander Butenko]

Gustavo, you are missing google.com.do domain for Dominican Republic

[Gustavo Noronha (kov)]

Created attachment 100317 [details]
Patch

[Gustavo Noronha (kov)]

How about this? The API is no longer exposed as public, but we keep using it for the test.

[Martin Robinson]

Comment on attachment 100317 [details]
Patch

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.

[Gustavo Noronha (kov)]

(In reply to comment #7)
> > 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).

That's a good idea =)
 
> > 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
<snip>
> > + * 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.

It won't because there's only one asterisk at the beginning; gtk-doc requires that first line to look like this: /**

[Gustavo Noronha (kov)]

(In reply to comment #8)
> (In reply to comment #7)
> > > 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).
> 
> That's a good idea =)

On second thought, I'd prefer not to do it that way. What we currently have does check that it's at the end of the host string, and if we use endsWith we'll have to iterate the hashset instead of using contains().

[Martin Robinson]

(In reply to comment #8)

> It won't because there's only one asterisk at the beginning; gtk-doc requires that first line to look like this: /**

I'm not a huge fan of this approach, since it's not clear at all looking at the method that it's a private API (it even includes the @since tag -- suggesting it's public). It would be quite easy for someone to mistakenly "fix" the asterisk. For instance, I can imagine myself making such a bone-headed mistake.

Doesn't the gtkdoc documentation suggest prefixing private methods with "_"?

[Martin Robinson]

Another option would be to use WebKit-style for the method name -- which would make it more obvious.

[Gustavo Noronha (kov)]

Created attachment 100357 [details]
Patch

[Gustavo Noronha (kov)]

I used WebKit style for the function name and added a test for www.google.is.not.com.br, to make sure it's not matching mid-string.

[Xan Lopez]

Comment on attachment 100357 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100357&action=review

Looks cool in general. The mess with the style stuff in the .c files breaks my heart.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1418
> +        initializeDomainsList(googleDomains);

I'd rather have both of these initialized in their own methods, in the same way (ie, add vs append). I know, OCD.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1492
> +    return g_strdup(webkit_web_settings_get_user_agent(webSettings));

If you rewrite this to do if (quirkcs) { ....} you can get rid of the duplicated "return g_strdup(webkit_web_settings_get_user_agent(webSettings));". Just saying.

[Gustavo Noronha (kov)]

Comment on attachment 100357 [details]
Patch

r91253

[Priit Laes (IRC: plaes)]

*** Bug 45549 has been marked as a duplicate of this bug. ***