Bug 132602

Summary: [GTK] [Stable] Can't browse new Google maps with latest stable
Product: WebKit Reporter: Xabier Rodríguez Calvar <calvaris>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, cgarcia, clopez, gustavo, mcatanzaro, svillar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=142074
Attachments:
Description Flags
Patch
none
Updated patch
none
Updated path svillar: review+

Description Xabier Rodríguez Calvar 2014-05-06 01:52:20 PDT
Since I updated and recompiled my webkit stack, I can't see the new Google Maps interface properly. It opens, but after I search some place I can see nothing.
Comment 1 Carlos Alberto Lopez Perez 2014-05-06 03:19:08 PDT
Seems this is caused by the string "Version/6.0" of the user-agent that WebKitGTK+ sends.

I guess that reverting r166405 <http://trac.webkit.org/r166405> would fix this issue.


If you have epiphany, you can test it with:

# set the UA with the "Version/6.0" substring and try google maps
gsettings set 'org.gnome.Epiphany' 'user-agent' 'Mozilla/5.0 (Macintosh; Intel Mac OS X) AppleWebKit/538.15 (KHTML, like Gecko) Safari/538.15 Version/6.0 Debian/unstable (3.12.0-1) Epiphany/3.12.0'

# now set the UA without the "Version/6.0" substring and try again
gsettings set 'org.gnome.Epiphany' 'user-agent' 'Mozilla/5.0 (Macintosh; Intel Mac OS X) AppleWebKit/538.15 (KHTML, like Gecko) Safari/538.15 Debian/unstable (3.12.0-1) Epiphany/3.12.0'
Comment 2 Alberto Garcia 2014-05-06 04:25:15 PDT
Reproduced with r168307
Comment 3 Michael Catanzaro 2014-05-06 05:33:52 PDT
(In reply to comment #1)
> Seems this is caused by the string "Version/6.0" of the user-agent that WebKitGTK+ sends.
> 
> I guess that reverting r166405 <http://trac.webkit.org/r166405> would fix this issue.

As an FYI: the user agent prior to r166405 caused us to receive the fallback www.google.com (with the black header bar and the I'm feeling lucky button) for ancient browsers. This annoyed me perhaps more than it should have.
Comment 4 Carlos Garcia Campos 2014-07-04 04:46:16 PDT
*** Bug 134631 has been marked as a duplicate of this bug. ***
Comment 5 Carlos Garcia Campos 2014-07-04 04:47:46 PDT
Created attachment 234402 [details]
Patch

This is only for stable for now. The google maps quirk is not needed in trunk, but maybe the other google domains quirk is required, I don't really know.
Comment 6 Carlos Garcia Campos 2014-07-04 04:59:29 PDT
Created attachment 234403 [details]
Updated patch

Patch updated to cover the case where Google Maps is used with url http://google.com/maps instead of maps.google.com
Comment 7 Sergio Villar Senin 2014-07-07 01:58:55 PDT
Comment on attachment 234403 [details]
Updated patch

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

Overall the patch looks good but I disagree on removing only the "Version" part. See bellow.

> Source/WebCore/platform/gtk/UserAgentGtk.cpp:253
> +    uaString.appendLiteral(" Safari/");

I wouldn't do this. As I mentioned in the change that added the "Version" part, there is no point on having "Safari/" without the "Version" string as that's invalid. So either we add them both or we add nothing.

That'd mean that we should likely rename the quirk to something like ClaimToBeSafari or something.
Comment 8 Alberto Garcia 2014-07-07 02:07:55 PDT
(In reply to comment #7)
> there is no point on having "Safari/" without the "Version" string
> as that's invalid. So either we add them both or we add nothing.

Google maps works fine if we add nothing. The interesting thing is
that in that case the "Try the new Google Maps" option doesn't appear.
Comment 9 Sergio Villar Senin 2014-07-07 02:16:39 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > there is no point on having "Safari/" without the "Version" string
> > as that's invalid. So either we add them both or we add nothing.
> 
> Google maps works fine if we add nothing. The interesting thing is
> that in that case the "Try the new Google Maps" option doesn't appear.

Another option could be to downgrade the Version number we use in stable so that google and other providers could safely assume that we have a given JS API. What about using "Version 7"? Does it fix the issues?
Comment 10 Alberto Garcia 2014-07-07 02:26:30 PDT
(In reply to comment #9)
> What about using "Version 7"? Does it fix the issues?

No, it actually fails already with Version 6.

Version 5 (if such a thing exists) works fine though, sending the old
version of the maps.
Comment 11 Xabier Rodríguez Calvar 2014-07-07 02:41:38 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > there is no point on having "Safari/" without the "Version" string
> > as that's invalid. So either we add them both or we add nothing.
> 
> Google maps works fine if we add nothing. The interesting thing is
> that in that case the "Try the new Google Maps" option doesn't appear.

The problem happens when you switched already to the new maps. In that case, it's non-functional.
Comment 12 Alberto Garcia 2014-07-07 03:57:37 PDT
(In reply to comment #11)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > there is no point on having "Safari/" without the "Version" string
> > > as that's invalid. So either we add them both or we add nothing.
> >
> > Google maps works fine if we add nothing. The interesting thing is
> > that in that case the "Try the new Google Maps" option doesn't appear.
>
> The problem happens when you switched already to the new maps. In
> that case, it's non-functional.

If you have only the "Safari/" part of the user-agent it will show you
the old maps plus the button to try the new ones. The interesting
thing is that in that case the new maps do work fine.
Comment 13 Alberto Garcia 2014-07-07 06:42:19 PDT
Anyway, removing both the "Version/" and the "Safari/" part for the maps lgtm, can we do this then? I think it's the only missing patch for the 2.4.4 release, isn't it?
Comment 14 Carlos Garcia Campos 2014-07-08 00:01:41 PDT
Created attachment 234550 [details]
Updated path

Rename the quirk and remove the whole safari part of the user agent as suggested by Sergio
Comment 15 Alberto Garcia 2014-07-08 01:38:01 PDT
(In reply to comment #14)
> Created an attachment (id=234550) [details]
> Updated path
> 
> Rename the quirk and remove the whole safari part of the user agent as suggested by Sergio

Tested, it works fine.
Comment 16 Sergio Villar Senin 2014-07-08 02:32:16 PDT
Comment on attachment 234550 [details]
Updated path

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

Let's go for it

> Source/WebCore/platform/gtk/UserAgentGtk.cpp:252
> +        uaString.appendLiteral(" Safari/");

Use a single appendLiteral() here
Comment 17 Carlos Garcia Campos 2014-07-08 02:42:25 PDT
Committed <http://trac.webkit.org/changeset/170882>