Bug 54560 – Drop the language tag part from the User Agent string

• Component: Platform
• Status: RESOLVED FIXED

[Laszlo Gombos]

As discussed in webkit-dev (https://lists.webkit.org/pipermail/webkit-dev/2011-February/015952.html) all WebKit ports should drop the language tag part from the User Agent string. Patch will follow.

I think we should consider bumping up the WebKit version with this change (e.g. MINOR_VERSION in Version.xcconfig). Feedback on this is would be appreciated.

[Alexey Proskuryakov]

I don't think that changing Version.xccconfig would be right. These versions only really reflect Apple's internal build procedures.

The minor version in Version.xccconfig changes pretty frequently anyway.

[Peter Kasting]

Is this an appropriate bug to also talk about removing the "U;" for encryption, and, on Windows, the initial "Windows;" that is redundant with the more-specific version identifier afterwards?

Removing those as well would match Firefox 4 ( http://hacks.mozilla.org/2010/09/final-user-agent-string-for-firefox-4/ ), and if we want to do them eventually, it might be better to do these all at once.  Certainly Chromium would be interested in seeing them removed.

[Alexey Proskuryakov]

Changes other than dropping language are tracked by a meta bug 54556.

[Peter Kasting]

(In reply to comment #3)
> Changes other than dropping language are tracked by a meta bug 54556.

I see.  I filed bugs on these other two items and blocked them against that bug.

[Alexey Proskuryakov]

<rdar://problem/8983585>

[Laszlo Gombos]

Created attachment 82813 [details]
first try

[Laszlo Gombos]

Chrome User Agent string seems to be constructed outside of WebKit tree (user_agent.cc).

[WebKit Review Bot]

Attachment 82813 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/ChangeLog', u'Source/WebKit/..." exit_code: 1

Source/WebKit2/UIProcess/win/WebPageProxyWin.cpp:108:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 82813 [details]
first try

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

What exactly does this change affect? Is it all of navigator.userAgent, navigator.appVersion, and HTTP User-Agent header field? Can you add tests for all of these?

> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:-124
> -    ua += "; ";
> -    ua += defaultLanguage(); // Localization information

There is likely an #include to remove in this file.

> Source/WebKit/efl/ewk/ewk_settings.cpp:326
> -    WTF::String static_ua = makeString("Mozilla/5.0 (", _ewk_settings_webkit_platform_get(), "; U; ", _ewk_settings_webkit_os_version_get(), "; ", WebCore::defaultLanguage(), ") AppleWebKit/", ua_version) + makeString(" (KHTML, like Gecko) Version/5.0 Safari/", ua_version);
> +    WTF::String static_ua = makeString("Mozilla/5.0 (", _ewk_settings_webkit_platform_get(), "; U; ", _ewk_settings_webkit_os_version_get(), ") AppleWebKit/", ua_version) + makeString(" (KHTML, like Gecko) Version/5.0 Safari/", ua_version);

Ditto.

> Source/WebKit/mac/WebView/WebView.mm:-551
> -    NSString *language = [NSUserDefaults _webkit_preferredLanguageCode];

Ditto.

> Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:-96
>      String language = defaultLanguage();
>  
>      if (applicationNameForUserAgent.isEmpty())
> -        return makeString("Mozilla/5.0 (Macintosh; U; " PROCESSOR " Mac OS X ", osVersion, "; ", language, ") AppleWebKit/", webKitVersion, " (KHTML, like Gecko)");
> -    return makeString("Mozilla/5.0 (Macintosh; U; " PROCESSOR " Mac OS X ", osVersion, "; ", language, ") AppleWebKit/", webKitVersion, " (KHTML, like Gecko) ", applicationNameForUserAgent);

This code won't even compile as is, due to unused variable warning.

[Laszlo Gombos]

Created attachment 83080 [details]
LayoutTests added

Thanks for the review Alexey. Added tests + fixed the review comments.

[Alexey Proskuryakov]

Comment on attachment 83080 [details]
LayoutTests added

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

Looks great. I have to say r- because this badly breaks Safari on Mac, but it's really almost like r+.

> Source/WebKit/ChangeLog:12
> +        * WebKit.xcodeproj/project.pbxproj: Remove WebNSUserDefaultsExtras.h
> +        and WebNSUserDefaultsExtras.mm from the project.

This cannot be done (see below).

> Source/WebKit/mac/ChangeLog:11
> +        * Misc/WebNSUserDefaultsExtras.h: Removed as it was only used to
> +        expose defaultLanguage() in the user agent string.
> +
> +        * Misc/WebNSUserDefaultsExtras.mm: Ditto.

These cannot be removed, because Safari uses +[NSUserDefaults _webkit_preferredLanguageCode].

I should have warned you about this pitfall.

> LayoutTests/http/tests/navigation/useragent.php:9
> +        layoutTestController.setPOSIXLocale("en_US.iso88591");

Why? I don't see how POSIX locale would affect the language - and I also don't see why forcing a language would be good for this test.

> LayoutTests/http/tests/navigation/useragent.php:34
> +                                   
>  \ No newline at end of file

Please remove the extra spaces that confused Subversion.

[Laszlo Gombos]

Created attachment 83218 [details]
address review comments

I should have caught the Safari break.

Setting POSIX locale was "inspired by" http://trac.webkit.org/browser/trunk/LayoutTests/fast/js/navigator-language.html. You're right that it is not needed, it was an attempt to make the test more predictable.

[Alexey Proskuryakov]

Comment on attachment 83218 [details]
address review comments

Looks great. I assume that Qt API changes will be (or have been) vetted as appropriate.

[Laszlo Gombos]

Thanks for the review. There should not be an API change for QtWebKit - other than the fact that the API now returns a different UA string.

Is there any other coordination or testing we should do before landing ? Peter suggested that Chromium could get some real-world feedback quickly.

[Alexey Proskuryakov]

> There should not be an API change for QtWebKit - other than the fact that the API now returns a different UA string.

What I meant was that %Language% was now ignored, which contradicts current header documentation.

I think that it's fine to just land this.

[Peter Kasting]

(In reply to comment #14)
> Is there any other coordination or testing we should do before landing ? Peter suggested that Chromium could get some real-world feedback quickly.

Once you land this I will land a change to make Chromium's UA string also drop this tag.

[WebKit Commit Bot]

Comment on attachment 83218 [details]
address review comments

Clearing flags on attachment: 83218

Committed r79396: <http://trac.webkit.org/changeset/79396>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.