[webkit-reviews] review denied: [Bug 149496] [GTK] Use mobile user-agent on tablet : [Attachment 403199] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at gnome.org> has denied Jan-Michael Brummer
<jan.brummer at tabos.org>'s request for review:
Bug 149496: [GTK] Use mobile user-agent on tablet
https://bugs.webkit.org/show_bug.cgi?id=149496

Attachment 403199: Patch

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




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

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

> Source/WebCore/platform/UserAgent.h:46
> +};
>  WEBCORE_EXPORT String standardUserAgent(const String& applicationName =
emptyString(), const String& applicationVersion = emptyString());

Blank line here

> Source/WebCore/platform/UserAgentQuirks.cpp:155
> +    return isGoogle(url) && chassisType == ChassisType::Desktop;

Let's use && chassisType != ChassisType::Mobile here, since we know affected
websites are likely to be broken without the user agent quirk....

> Source/WebCore/platform/glib/UserAgentGLib.cpp:50
> +static ChassisType getChassisType()

I think we should follow hostnamed as closely as possible here:
https://github.com/systemd/systemd/blob/d7f4ad203acb07e728865d5ea117f7df5e8d816
6/src/hostname/hostnamed.c#L186

First it reads /etc/machine-info to get the chassis type. That's going to fail
by default. I don't have anything set in my /etc/machine-info other than
PRETTY_HOSTNAME. So if you're relying only on this, I think you're going to be
pretty disappointed with the results. After /etc/machine-info, it next tries
/sys/class/dmi/id/chassis_type, then finally it tries
"/sys/firmware/acpi/pm_profile" if not available. I see that /etc is mounted by
BubblewrapLauncher.cpp, but not by flatpak. (Surprise! Wasn't expecting that to
be different.)	/sys/class is allowed by both BubblewrapLauncher and flatpak,
so no changes needed there. /sys/firmware is not allowed by either (and I have
no idea if that would be safe).

We are going to change something in flatpak no matter what, either to expose a
filtered version of hostnamed, or to expose /etc/machine-info. And we need to
decide how to change flatpak before we change WebKit.

> Source/WebCore/platform/glib/UserAgentGLib.cpp:56
> +	   std::ifstream infoFile("/etc/machine-info");

std::ifstream can throw exceptions, so we can't use it in WebKit. Use either
GLib or POSIX APIs instead.