[webkit-reviews] review denied: [Bug 209378] [GTK] Add user agent quirk for auth.mayohr.com : [Attachment 394171] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at gnome.org> has denied Ting-Wei Lan
<lantw44 at gmail.com>'s request for review:
Bug 209378: [GTK] Add user agent quirk for auth.mayohr.com
https://bugs.webkit.org/show_bug.cgi?id=209378

Attachment 394171: Patch

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




--- Comment #3 from Michael Catanzaro <mcatanzaro at gnome.org> ---
Comment on attachment 394171
  --> https://bugs.webkit.org/attachment.cgi?id=394171
Patch

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

OK this looks basically good, but let's simplify it a bit, because it reveals a
problem with our existing Chrome quirk.

> Source/WebCore/platform/UserAgentQuirks.cpp:157
> +    // This site does not recognize Chrome/X with Version/X as a valid
Chrome
> +    // user agent, so we have to hide Version/X from the user agent.

OK, this just indicates that our current Google Chrome quirk is broken, and we
just got lucky that the existing sites we use it on didn't notice. We don't
need a new quirk here; instead, we should fix the existing quirk to not include
Version/X.

> Source/WebCore/platform/glib/UserAgentGLib.cpp:113
>      // Version/X is mandatory *before* Safari/X to be a valid Safari UA. See
>      // https://bugs.webkit.org/show_bug.cgi?id=133403 for details.
> -    uaString.appendLiteral("Version/13.0 Safari/");
> +    if (!quirks.contains(UserAgentQuirks::NeedsNoSafariVersion))
> +	   uaString.appendLiteral("Version/13.0 ");

So this can become an else clause of the previous condition. Then it will get
added except when the Chrome quirk is in use. (Or when the Firefox quirk is in
use, but we already returned early up above in that case.)