Bug 63994 - [GTK] Google Calendar thinks we're mobile
Summary: [GTK] Google Calendar thinks we're mobile
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gustavo Noronha (kov)
URL:
Keywords: GoogleBug, Gtk
: 45549 (view as bug list)
Depends on:
Blocks: 45549
  Show dependency treegraph
 
Reported: 2011-07-06 07:27 PDT by Gustavo Noronha (kov)
Modified: 2012-01-13 02:10 PST (History)
3 users (show)

See Also:


Attachments
Patch (19.27 KB, patch)
2011-07-06 08:04 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
Patch (18.18 KB, patch)
2011-07-11 10:06 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
Patch (19.60 KB, patch)
2011-07-11 13:20 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gustavo Noronha (kov) 2011-07-06 07:27:36 PDT
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.
Comment 1 Gustavo Noronha (kov) 2011-07-06 08:04:04 PDT
Created attachment 99833 [details]
Patch
Comment 2 Xan Lopez 2011-07-06 08:35:16 PDT
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.
Comment 3 Martin Robinson 2011-07-06 20:48:15 PDT
(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.
Comment 4 Alexander Butenko 2011-07-07 06:57:39 PDT
Gustavo, you are missing google.com.do domain for Dominican Republic
Comment 5 Gustavo Noronha (kov) 2011-07-11 10:06:28 PDT
Created attachment 100317 [details]
Patch
Comment 6 Gustavo Noronha (kov) 2011-07-11 10:07:30 PDT
How about this? The API is no longer exposed as public, but we keep using it for the test.
Comment 7 Martin Robinson 2011-07-11 10:26:27 PDT
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.
Comment 8 Gustavo Noronha (kov) 2011-07-11 12:23:22 PDT
(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: /**
Comment 9 Gustavo Noronha (kov) 2011-07-11 12:37:43 PDT
(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().
Comment 10 Martin Robinson 2011-07-11 12:43:54 PDT
(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 "_"?
Comment 11 Martin Robinson 2011-07-11 12:44:35 PDT
Another option would be to use WebKit-style for the method name -- which would make it more obvious.
Comment 12 Gustavo Noronha (kov) 2011-07-11 13:20:56 PDT
Created attachment 100357 [details]
Patch
Comment 13 Gustavo Noronha (kov) 2011-07-11 13:26:56 PDT
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.
Comment 14 Xan Lopez 2011-07-18 14:03:40 PDT
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.
Comment 15 Gustavo Noronha (kov) 2011-07-19 07:50:23 PDT
Comment on attachment 100357 [details]
Patch

r91253
Comment 16 Priit Laes (IRC: plaes) 2012-01-13 02:10:25 PST
*** Bug 45549 has been marked as a duplicate of this bug. ***