[webkit-reviews] review denied: [Bug 162611] [GTK] Allow distributors to brand user agent : [Attachment 392785] Patch

[bugzilla-daemon at webkit.org]

Adrian Perez <aperez at igalia.com> has denied Michael Catanzaro
<mcatanzaro at gnome.org>'s request for review:
Bug 162611: [GTK] Allow distributors to brand user agent
https://bugs.webkit.org/show_bug.cgi?id=162611

Attachment 392785: Patch

https://bugs.webkit.org/attachment.cgi?id=392785&action=review




--- Comment #22 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 392785
  --> https://bugs.webkit.org/attachment.cgi?id=392785
Patch

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

> Source/WebCore/ChangeLog:3
> +	   [GTK] Allow distributors to brand user agent

The patch (and bug title) should include “[WPE]” :)

> Source/WebCore/platform/glib/UserAgentGLib.cpp:93
> +	   uaString.appendLiteral(DISTRIBUTOR_BRANDING "; ");

Let's imagine for a moment that I don't trust what people will pass in the
build option as a valid User-Agent header value: if somebody puts something
here which does not conform to RFC 7231 (section 5.5.3, more specifically)
they will have web sites fails in mysterious ways when the server does strict
validation of the header. I have seen some CDNs (Cloudfare, for example)
returning HTTP 400 (Bad Request) if the UA string has invalid contents. And
trust me that people will knock their heads against the wall for days without
end before realizing that they have added garbage to the UA string.

Therefore, that when custom branding is added we need to unconditionally
call isValidUserAgentHeaderValue()—not just an assertion—, log an error
message very noticeably, and abort() the process... Unless you can come up
with some way of checking the passed value when running CMake.

> Source/cmake/OptionsGTK.cmake:20
> +set(DISTRIBUTOR_BRANDING "" CACHE STRING "Branding to add to user agent
string")

The name “DISTRIBUTOR_BRANDING” kind of implies that this is some general
branding, to be used only by distributors (what if I am not one and still
want to use the option?) and not just for the user agent. The description
of the option clarifies it, yet I think a better name could be something
like “USER_AGENT_BRANDING”.