Bug 162611 - [GTK] Allow distributors to brand user agent
Summary: [GTK] Allow distributors to brand user agent
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks: 142074
  Show dependency treegraph
 
Reported: 2016-09-27 08:19 PDT by Michael Catanzaro
Modified: 2017-12-06 10:24 PST (History)
5 users (show)

See Also:


Attachments
Patch (1.38 KB, patch)
2016-09-27 09:55 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (1.42 KB, patch)
2016-10-17 04:41 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (1.34 KB, patch)
2016-10-27 07:21 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2016-09-27 08:19:43 PDT
Allow distributions to add branding to user agent, right after X11, if desired. This seems to be the safest place to put it to avoid breakage.
Comment 1 Michael Catanzaro 2016-09-27 08:22:56 PDT
Also, fix related issues in this function.
Comment 2 Michael Catanzaro 2016-09-27 08:51:41 PDT
(In reply to comment #1)
> Also, fix related issues in this function.

Just kidding
Comment 3 Michael Catanzaro 2016-09-27 09:55:42 PDT
Created attachment 289961 [details]
Patch
Comment 4 Zan Dobersek 2016-09-28 03:33:06 PDT
Comment on attachment 289961 [details]
Patch

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

> Source/WebCore/platform/gtk/UserAgentGtk.cpp:132
> +#ifdef USER_AGENT_GTK_DISTRIBUTOR_NAME

Usually we go with defined(MACRO) && MACRO -- unless an empty string is an option(?).
Comment 5 Michael Catanzaro 2016-09-28 05:23:15 PDT
(In reply to comment #4)
> Comment on attachment 289961 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289961&action=review
> 
> > Source/WebCore/platform/gtk/UserAgentGtk.cpp:132
> > +#ifdef USER_AGENT_GTK_DISTRIBUTOR_NAME
> 
> Usually we go with defined(MACRO) && MACRO -- unless an empty string is an
> option(?).

Usually our macros are defined to 0 or 1 where a boolean comparison is more natural. This one is either defined to a string if it's to be used, or not defined at all.
Comment 6 Michael Catanzaro 2016-09-29 03:16:47 PDT
(In reply to comment #5)
> Usually our macros are defined to 0 or 1 where a boolean comparison is more
> natural. This one is either defined to a string if it's to be used, or not
> defined at all.

To be clear, the reason we usually need both checks is that we don't want to enable the feature if the preprocessor variable is defined to 0. That's not a concern in this case. If you define USER_AGENT_GTK_DISTRIBUTOR_NAME to an empty string, that's pretty silly, but harmless (you'll get an extra semicolon in the UA).
Comment 7 Michael Catanzaro 2016-10-04 11:36:28 PDT
Ping reviewers
Comment 8 Michael Catanzaro 2016-10-17 04:41:01 PDT
Created attachment 291809 [details]
Patch
Comment 9 Michael Catanzaro 2016-10-27 07:21:14 PDT
Created attachment 293018 [details]
Patch
Comment 10 Michael Catanzaro 2016-12-26 15:07:54 PST
(In reply to comment #7)
> Ping reviewers
Comment 11 Michael Catanzaro 2016-12-28 15:13:15 PST
What can I do to get this approved? What if I move the distributor to the end of the UA, would that be accepted?

Distributor branding is not optional for us and I hate carrying this downstream forever.
Comment 12 Michael Catanzaro 2017-01-20 05:49:37 PST
(In reply to comment #11)
> What can I do to get this approved? What if I move the distributor to the
> end of the UA, would that be accepted?

Let's move it to the end, and also put WebKitGTK+ there. And instead of using a preprocessor conditional, let's read it from the NAME and VERSION_ID fields in /etc/os-release. Should look like e.g.:

Mozilla/5.0 (X11; Fedora; Linux x86_64) AppleWebKit/603.1 (KHTML, like Gecko) Version/10.0 Safari/603.1 Fedora/25 WebKitGTK+/603.1 Epiphany/3.22.5
Comment 13 Michael Catanzaro 2017-01-20 05:51:40 PST
(In reply to comment #12)
> Mozilla/5.0 (X11; Fedora; Linux x86_64) AppleWebKit/603.1 (KHTML, like
> Gecko) Version/10.0 Safari/603.1 Fedora/25 WebKitGTK+/603.1 Epiphany/3.22.5

I'm really bad at copy-pasting. The distributor should only be there once:

Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/603.1 (KHTML, like Gecko) Version/10.0 Safari/603.1 Fedora/25 WebKitGTK+/603.1 Epiphany/3.22.5

Also looks like we need to bump Safari version from Version/10.0 to Version/11.0.
Comment 14 Michael Catanzaro 2017-01-22 08:58:06 PST
Comment on attachment 293018 [details]
Patch

The /etc/os-release plan was a bad idea. It's not good enough because there is no field that includes a name that is guaranteed to be suitable for use in the user agent. E.g. the name could be "Debian GNU/Linux" which is not suitable.
Comment 15 Michael Catanzaro 2017-05-10 13:09:09 PDT
Comment on attachment 293018 [details]
Patch

In the end, I've come to believe Carlos Garcia was right all along and we shouldn't do this.
Comment 16 Michael Catanzaro 2017-12-06 08:52:04 PST
(In reply to Michael Catanzaro from comment #15)
> Comment on attachment 293018 [details]
> Patch
> 
> In the end, I've come to believe Carlos Garcia was right all along and we
> shouldn't do this.

Um, I forgot I ever said this. We're still using this patch in Fedora. I've removed Epiphany's distro branding feature, and advised Ubuntu to apply this patch instead.