Bug 149496 – [GTK][WPE] Use mobile user-agent on tablet

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Bastien Nocera]

From https://bugzilla.gnome.org/show_bug.cgi?id=754089

$ gdbus introspect --system --dest org.freedesktop.hostname1  --object-path /org/freedesktop/hostname1 | grep 'Chassis '
      readonly s Chassis = 'laptop';

Using that data, we should switch to using a mobile user-agent when "tablet" is detected ("handset" could also switch to that).

[Michael Catanzaro]

We stared discussing this in https://gitlab.gnome.org/GNOME/epiphany/-/merge_requests/601#note_853104

Let's have WebKit perform this chassis check. UserAgentGLib.cpp under Source/WebCore/platform/glib would be the best place. Let's also add a ChassisType parameter to UserAgentQuirks::quirksForURL in UserAgentQuirks.[cpp,h] so we can disable quirks that are not appropriate for mobile:

```
    // At least finance.yahoo.com displays a mobile version with WebKitGTK's standard user agent.
    if (baseDomain == "yahoo.com")
        return true;

    // taobao.com displays a mobile version with WebKitGTK's standard user agent.
    if (baseDomain == "taobao.com")
        return true;
```

[Jan-Michael Brummer]

Created attachment 403095 [details]
Patch

[Jan-Michael Brummer]

This is a almost working solution with debug statements. For urls without a quirk everything works fine, but as soon as a quirk is needed ChassisType is Unknown. Don't know why. Therefore please review my current attempt and obviously i did something wrong in this area... Thanks.

[Michael Catanzaro]

Comment on attachment 403095 [details]
Patch

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

Guess: since standardUserAgentForURL() is called in the web process, it is not going to have D-Bus access to hostnamed. Drat. I bet if you use WEBKIT_FORCE_SANDBOX=0 then this would work? Please check. I think if you add a call to getpid() to your debug statements, the problem would become a bit clearer... although beware, because the web process runs inside a pid namespace.

So um, this is actually a much trickier problem than I expected it to be. I'm not actually sure what to do. We could pass the information from the UI process to the web process via IPC, but that doesn't work if we're running in flatpak because the UI process won't have permission to talk to hostnamed either. There might not be any way around this without hardcoding --talk-name=org.freedesktop.hostname1 in every application's flatpak manifest, and surely we don't want to add code that relies on that.

So I know this isn't what you want to hear, but I think we need to talk to Patrick (TingPing) and the other xdg-desktop-portal developers about what a chassis portal would look like. At least, I don't see a good way to do this properly without constructing a new portal. We should try to get more opinions.

> Source/WebCore/platform/UserAgentQuirks.cpp:155
> +    bool ret = isGoogle(url) && (chassisType != UserAgentQuirks::ChassisTypeMobile);

I know this line is just here for debugging, but WebKit style avoids the unnecessary parentheses around chassisType != UserAgentQuirks::ChassisTypeMobile

> Source/WebCore/platform/UserAgentQuirks.h:47
> +    enum ChassisType {
> +        ChassisTypeUnknown,
> +        ChassisTypeDesktop,
> +        ChassisTypeMobile
> +    };

This doesn't belong in UserAgentQuirks because the ChassisType is also used in UserAgentGLib.cpp. I would declare it in Source/WebCore/platform/UserAgent.h inside the #else block for !PLATFORM(COCOA). Whereas UserAgentQuirk is a plain enum because it's used as a C-style flags variable, ChassisType should be a C++ scoped enum, like this:

enum class ChassisType {
    Unknown,
    Desktop,
    Mobile
};

Then instead of writing e.g. ChassisTypeUnknown, you would write ChassisType::Unknown.

> Source/WebCore/platform/glib/UserAgentGLib.cpp:55
> +    if (chassisType == UserAgentQuirks::ChassisTypeUnknown) {

So problem here is that if chassis type is unknown (e.g. because hostnamed is not running), then we run all the code again and again every time. Better use a static bool variable that is always set after the first run to ensure this function is really only ever executed exactly once.

> Source/WebCore/platform/glib/UserAgentGLib.cpp:59
> +        g_autoptr(GError) error = NULL;
> +        g_autoptr(GDBusConnection) connection = NULL;
> +        g_autoptr(GVariant) var = NULL;
> +        g_autoptr(GVariant) v = NULL;

We don't use autoptrs in WebKit because this is C++, so we have real smart pointers instead. Much safer:

GUniqueOutPtr<GError> error;
GRefPtr<GDBusConnection> connection;
GRefPtr<GVariant> var; // of course you'll pick better names for var and v
GRefPtr<GVariant> v;   // (perhaps variant and childVariant?)

But we also don't declare variables before first use, so all of these get declared below, not at the top of the function.

Also: NULL -> nullptr

> Source/WebCore/platform/glib/UserAgentGLib.cpp:62
> +        connection = g_bus_get_sync(G_BUS_TYPE_SYSTEM, NULL, &error);

BTW, you asked on IRC how we could avoid doing sync I/O in this awkward place, and I said I don't think it's easily avoidable. This is unfortunate, but without changing the user agent APIs to be async, there's not really anything we can do about it. And we definitely don't want to change the user agent APIs to be async. If other reviewers have any better ideas, I'd love to hear them, but this seems to me like the best we can do.

BTW, to convert to GUniqueOutPtr, you would write:

GUniqueOutPtr<GError> error;
g_bus_get_sync(G_BUS_TYPE_SYSTEM, nullptr, &error.outPtr());

(Note again NULL -> nullptr.)

> Source/WebCore/platform/glib/UserAgentGLib.cpp:65
> +            g_debug("Could not connect to system bus: %s", error->message);
> +            WTFLogAlways("%s(): EXIT 1 %s\n", __FUNCTION__, error->message);

For the final version of the patch, you would change the g_debug() to g_warning(). (And delete the WTFLogAlways, but I known that's just there for debugging.)

> Source/WebCore/platform/glib/UserAgentGLib.cpp:69
> +        var = g_dbus_connection_call_sync(connection,

To use smart pointers, you would write: var = adoptGRef(g_dbus_connection_call_sync(connection.get(),

adoptGRef() adopts the initial reference (otherwise the refcount would be increased to 2 here and we'd get a leak) and .get() returns the GDBusConnection* owned by the GRefPtr<GDBusConnection>.

> Source/WebCore/platform/glib/UserAgentGLib.cpp:70
> +        var = g_dbus_connection_call_sync(connection,
> +    "org.freedesktop.hostname1",

For indentation, we indent four spaces relative to the start of the previous line. So:

GRefPtr<GVariant> var = adoptGRef(g_dbus_connection_call_sync(connection.get(),
    "org.freedesktop.hostname1",

> Source/WebCore/platform/glib/UserAgentGLib.cpp:83
> +            g_debug("Could not access chassis property: %s", error->message);

Here I don't think we need to print anything at all, since apparently not all the world uses systemd and it's just expected that it might not be present. We don't use g_debug() in WebKit and I don't see any particular reason to log here. Returning ChassisType::Unknown will be fine.

> Source/WebCore/platform/glib/UserAgentGLib.cpp:87
> +        g_variant_get(var, "(v)", &v);

I guess you cannot use g_variant_get_variant() here, because the type is G_VARIANT_TYPE_TUPLE rather than G_VARIANT_TYPE_VARIANT?

You can do:

GRefPtr<GVariant> v;
g_variant_get(var.get(), "(v)", &v.outPtr());

I suggest renaming var to variant (no abbreviations!) and v to childVariant, or something like that.

> Source/WebCore/platform/glib/UserAgentGLib.cpp:88
> +        chassis = g_variant_get_string(v, NULL);

GUniquePtr<char> chassis(g_variant_get_string(v, nullptr));

> Source/WebCore/platform/glib/UserAgentGLib.cpp:91
> +        /* convertible is just for testing at the moment */

// Convertible is just for testing at the moment.

C++ // comment, full sentence with sentence case capitalization and a period at the end.

> Source/WebCore/platform/glib/UserAgentGLib.cpp:110
> +

Nit: it looks better without this blank line here.

> Source/WebCore/platform/glib/UserAgentGLib.cpp:123
>      struct utsname name;
>      uname(&name);
>      static NeverDestroyed<const String> uaOSVersion(makeString(name.sysname, ' ', name.machine));
> +
> +    if (getChassisType() == UserAgentQuirks::ChassisTypeMobile)
> +        return "like Android 4.4";

Check the chassis type first, before you call uname(). Avoid wasted work.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #4)
> So I know this isn't what you want to hear, but I think we need to talk to
> Patrick (TingPing) and the other xdg-desktop-portal developers about what a
> chassis portal would look like. At least, I don't see a good way to do this
> properly without constructing a new portal. We should try to get more
> opinions.

Might even want to add a chassis type function to GTK 4, that way we wouldn't need to add a portal that wouldn't be used by GTK.

[Jan-Michael Brummer]

> Guess: since standardUserAgentForURL() is called in the web process, it is
> not going to have D-Bus access to hostnamed. Drat. I bet if you use
> WEBKIT_FORCE_SANDBOX=0 then this would work? Please check. I think if you
> add a call to getpid() to your debug statements, the problem would become a
> bit clearer... although beware, because the web process runs inside a pid
> namespace.

Short update: WEBKIT_FORCE_SANDBOX=0 works fine.

[Jan-Michael Brummer]

Created attachment 403176 [details]
Patch

[Jan-Michael Brummer]

Thanks for the detailed review Michael, i really appreciate it. I've integrated those findings in my updated patch. It's a good starting point to get in contact with TingPing now.

The impact of this patch is massive for the mobile experience as for supported pages the download size is much lower and thus improves rendering speed.

[Jan-Michael Brummer]

Created attachment 403177 [details]
Patch

[Jan-Michael Brummer]

Removed some blank lines...

[Jan-Michael Brummer]

Another working option is to parse:

  /sys/devices/virtual/dmi/id/chassis_type

and

  /etc/machine-info

manually.

[Jan-Michael Brummer]

Created attachment 403199 [details]
Patch

[Jan-Michael Brummer]

I have uploaded a patch which only makes use of /etc/machine-info as DMI is not present on all systems.

Please note that the default is always ChassisType::Desktop to ensure that everything stays the same for all other systems. Only once tablet or handset is detected it is switched to ChassisType::Mobile and the user agent is modified.

In order to test it you can easily switch your chassis type.

Not sure this is an accepted solution, but it is working fine for me.

Flatpak does not mirror this file at the moment, so in both cases (file/portal) flatpak needs to be updated.

[Michael Catanzaro]

Comment on attachment 403199 [details]
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/d7f4ad203acb07e728865d5ea117f7df5e8d8166/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.

[Jan-Michael Brummer]

Created attachment 403492 [details]
Patch

[Jan-Michael Brummer]

We (Michael & Patrick) agreed on using the hostnamed logic without webkit.

Updated patch accordingly and integrated review comments.

[Michael Catanzaro]

Comment on attachment 403492 [details]
Patch

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

OK, this is looking better, but it's still written too much like C code rather than C++, so I will try to show you how to fix that.

Will you also open a flatpak pull request to expose /etc/machine-info?

> Source/WebCore/platform/glib/UserAgentGLib.cpp:49
> +static ChassisType readMachineInfo(void)

static ChassisType readMachineInfo()

The void is implicit in C++.

> Source/WebCore/platform/glib/UserAgentGLib.cpp:51
> +    ChassisType type = ChassisType::Unknown;

You don't need to declare this (see below).

> Source/WebCore/platform/glib/UserAgentGLib.cpp:52
> +    GUniqueOutPtr<GError> error;

You actually don't need this after all (see below).

> Source/WebCore/platform/glib/UserAgentGLib.cpp:53
> +    char* buffer;

GUniqueOutPtr<char> buffer;

> Source/WebCore/platform/glib/UserAgentGLib.cpp:54
> +    if (!g_file_get_contents("/etc/machine-info", &buffer, NULL, &error.outPtr())) {

(..., &buffer.outPtr(), nullptr, ...)

> Source/WebCore/platform/glib/UserAgentGLib.cpp:55
> +        g_debug("Could not open /etc/machine-info: %s\n", error->message);

We don't use g_debug() in WebKit. It's expected the file might not exist, so instead you can pass nullptr for the error and just return here without printing anything.

> Source/WebCore/platform/glib/UserAgentGLib.cpp:59
> +    GStrv split = g_strsplit(buffer, "\n", -1);

GStrv is only for use with g_autofree, where you can't write gchar**. In C++, you ought to ensure the ownership is held in a smart pointer class; this is expected in WebKit. Use GUniquePtr:

GUniquePtr<char*> split(g_strsplit(...));

That will help avoid accidental memory leaks, including the one you have here, because you forgot to free it. ;)

> Source/WebCore/platform/glib/UserAgentGLib.cpp:62
> +    for (guint i = 0; i < g_strv_length(split); i++) {

Don't use guint in WebKit (except for public APIs).

You're calling g_strv_length() once per iteration of the loop, that means you're traversing the entire length of the array to get the length every loop, even though it doesn't change. You could calculate the length outside the loop instead, but in this case you don't actually need it: for (int i = 0; split[i]; i++)

> Source/WebCore/platform/glib/UserAgentGLib.cpp:66
> +            if (!g_strcmp0(chassis, "tablet") || !g_strcmp0(chassis, "handset"))

Use normal strcmp() here, because chassis can never be nullptr.

> Source/WebCore/platform/glib/UserAgentGLib.cpp:67
> +                type = ChassisType::Mobile;

return ChassisType::Mobile;

> Source/WebCore/platform/glib/UserAgentGLib.cpp:69
> +                type = ChassisType::Desktop;

return ChassisType::Desktop;

> Source/WebCore/platform/glib/UserAgentGLib.cpp:75
> +    return type;

return ChassisType::Unknown;

> Source/WebCore/platform/glib/UserAgentGLib.cpp:78
> +static ChassisType readFallbackChassis(void)

static ChassisType readFallbackChassis()

I would split this into two functions: readDMIChassisType() and readACPIChassisType()

> Source/WebCore/platform/glib/UserAgentGLib.cpp:82
> +    GUniqueOutPtr<GError> error;
> +    char* buffer;
> +    int t;

Remember what I said about not declaring things at the top of functions, only at first use. So here you really do need to declare error and buffer, but there should be no blank line between the declarations and where they are first used, because you want the declarations as close as possible to where they are used. (Except you don't need error after all, since you're just going to ignore the error.) And t should be declared below.

Also: GUniquePtr<char> buffer;

> Source/WebCore/platform/glib/UserAgentGLib.cpp:84
> +    if (!g_file_get_contents("/sys/class/dmi/id/chassis_type", &buffer, NULL, &error.outPtr())) {

nullptr

> Source/WebCore/platform/glib/UserAgentGLib.cpp:85
> +        g_debug("Could not open /sys/class/dmi/id/chassis_type: %s\n", error->message);

Don't use g_debug(), just ignore the error.

> Source/WebCore/platform/glib/UserAgentGLib.cpp:86
> +        goto tryACPI;

You don't need goto for this, just invert the condition and move the switch inside the conditional.

We almost never use goto in C++ because it's not needed for manually freeing things in error paths.

> Source/WebCore/platform/glib/UserAgentGLib.cpp:89
> +    t = atoi(buffer);

int t = atoi(buffer.get());

> Source/WebCore/platform/glib/UserAgentGLib.cpp:114
> +    if (!g_file_get_contents("/sys/firmware/acpi/pm_profile", &buffer, NULL, &error.outPtr())) {

if (!g_file_get_contents("/sys/firmware/acpi/pm_profile", &buffer.outPtr(), nullptr, nullptr))

> Source/WebCore/platform/glib/UserAgentGLib.cpp:115
> +        g_debug("Could not open /sys/firmware/acpi/pm_profile: %s\n", error->message);

And get rid of the g_debug

> Source/WebCore/platform/glib/UserAgentGLib.cpp:119
> +    t = atoi(buffer);

t = atoi(buffer.get())

> Source/WebCore/platform/glib/UserAgentGLib.cpp:141
> +    static ChassisType chassisType = ChassisType::Desktop;

static ChassisType chassisType;

There's no point in initializing it to a value that's guaranteed to be overwritten below (and static variables are zero-initialized anyway).

> Source/WebCore/platform/glib/UserAgentGLib.cpp:171
> +        return "like Android 4.4";
>      struct utsname name;

Nit: I'd add a blank line here.

[Jan-Michael Brummer]

Created attachment 403517 [details]
Patch

[Jan-Michael Brummer]

(In reply to Michael Catanzaro from comment #17)
> Comment on attachment 403492 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403492&action=review
> 
> OK, this is looking better, but it's still written too much like C code
> rather than C++, so I will try to show you how to fix that.

Thanks, yes a decade ago i used to write C++ and now it's C all the time. :)

> Will you also open a flatpak pull request to expose /etc/machine-info?

Sure, i promised it and will do it today.

Patch is updated based on your comments and i've splitted the second function into two for better readability.

[Jan-Michael Brummer]

Flatpak pull request: https://github.com/flatpak/flatpak/pull/3727

[Michael Catanzaro]

Comment on attachment 403517 [details]
Patch

OK, this looks good. Let's wait and see if the flatpak patch is accepted before we commit. Bastien has suggested in the flatpak PR that we might need to write a portal instead.

[Adrian Perez]

Comment on attachment 403517 [details]
Patch

The logic in the patch looks good overall, but there are still a few
things that I would prefer to see ironed out before landing. At any
rate, thanks a ton for working on it!

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

> Source/WebCore/platform/glib/UserAgentGLib.cpp:52
> +    if (!g_file_get_contents("/etc/machine-info", &buffer.outPtr(), nullptr, nullptr))

Please use a GError here and emit a warning if the error is other
than G_IO_ERROR_NOT_FOUND, which will make it much easier to figure
out the cause when things don't work. In general silently skipping
over errors is a bad idea.

> Source/WebCore/platform/glib/UserAgentGLib.cpp:72
> +    if (g_file_get_contents("/sys/class/dmi/id/chassis_type", &buffer.outPtr(), nullptr, nullptr)) {

Use a GError here as well, please.

> Source/WebCore/platform/glib/UserAgentGLib.cpp:73
> +        int t = atoi(buffer.get());

Please use strtol() instead and check its result for conversion errors.
Even better, use WTF::toIntegralType() which also can report errors.

> Source/WebCore/platform/glib/UserAgentGLib.cpp:75
> +        switch (t) {

A comment here indicating which source file in the kernel defines
these constants would be useful, in case somebody need to re-check
it in the future to update the “switch” statement.

> Source/WebCore/platform/glib/UserAgentGLib.cpp:104
> +        int t = atoi(buffer.get());

Ditto: Do not silently skip over all errors, use a checked
numeric conversion function, and add a comment indicating which
Linux kernel source file defines the magic values.

[Adrian Perez]

Also, this touches UserAgentGLib.cpp, so the bug description
should have the [WPE] tag as well.

[Carlos Garcia Campos]

Comment on attachment 403517 [details]
Patch

We already have WebContentMode, Apple uses it via content policies and it changes the user agent for iOS. We could at least reuse the enum.

[Jan-Michael Brummer]

Created attachment 403599 [details]
Patch

[Jan-Michael Brummer]

Patch updated:

- I've readded the error handling code which only prints out a warning in case it is not G_IO_ERROR_NOT_FOUND.

- Used strtol.

- Added documentation links where those types are coming from.

- Added [WPE] within ChangeLog.



I did not switch to WebContentMode as i think it does not reflect a chassis type and in future this type can be expanded for other devices. Take a look at cocoa where there is something similar.

[Carlos Garcia Campos]

Comment on attachment 403599 [details]
Patch

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

> Source/WebCore/platform/glib/UserAgentGLib.cpp:55
> +        if (!g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_NOT_FOUND))
> +            g_warning("Could not open /etc/machine-info: %s", error->message);

This is not working, all API tests are failing because of this warning:

FATAL-WARNING: Could not open /etc/machine-info: Failed to open file “/etc/machine-info”: No such file or directory

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

We don't use getFoo in WebKit when the result is returned as return value.

> Source/WebCore/platform/glib/UserAgentGLib.cpp:145
> +    static bool firstRun = true;
> +
> +    if (firstRun) {

Use std::once_flag

> Source/WebCore/platform/glib/UserAgentGLib.cpp:282
> +    auto quirks = UserAgentQuirks::quirksForURL(url, getChassisType());

I wonder if we could move the chassisType() to WTF and simply use it every, instead of passing it as an argument. For now it's only used by user agent, but doesn't really belong to UserAgent, I would say.

[Jan-Michael Brummer]

(In reply to Carlos Garcia Campos from comment #27)
> Comment on attachment 403599 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403599&action=review
> 
> > Source/WebCore/platform/glib/UserAgentGLib.cpp:55
> > +        if (!g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_NOT_FOUND))
> > +            g_warning("Could not open /etc/machine-info: %s", error->message);
> 
> This is not working, all API tests are failing because of this warning:
> 
> FATAL-WARNING: Could not open /etc/machine-info: Failed to open file
> “/etc/machine-info”: No such file or directory

Shall i remove the error again?

> 
> > Source/WebCore/platform/glib/UserAgentGLib.cpp:140
> > +static ChassisType getChassisType()
> 
> We don't use getFoo in WebKit when the result is returned as return value.

So just chassisType()?

> 
> > Source/WebCore/platform/glib/UserAgentGLib.cpp:145
> > +    static bool firstRun = true;
> > +
> > +    if (firstRun) {
> 
> Use std::once_flag
> 
> > Source/WebCore/platform/glib/UserAgentGLib.cpp:282
> > +    auto quirks = UserAgentQuirks::quirksForURL(url, getChassisType());
> 
> I wonder if we could move the chassisType() to WTF and simply use it every,
> instead of passing it as an argument. For now it's only used by user agent,
> but doesn't really belong to UserAgent, I would say.

Just below WTF or in an existing file?

[Carlos Garcia Campos]

Comment on attachment 403599 [details]
Patch

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

>>> Source/WebCore/platform/glib/UserAgentGLib.cpp:55
>>> +            g_warning("Could not open /etc/machine-info: %s", error->message);
>> 
>> This is not working, all API tests are failing because of this warning:
>> 
>> FATAL-WARNING: Could not open /etc/machine-info: Failed to open file “/etc/machine-info”: No such file or directory
> 
> Shall i remove the error again?

We should handle the right error, g_file_get_contents is not a gio function, so the error is not a G_IO_ERROR but a  G_FILE_ERROR.

>>> Source/WebCore/platform/glib/UserAgentGLib.cpp:140
>>> +static ChassisType getChassisType()
>> 
>> We don't use getFoo in WebKit when the result is returned as return value.
> 
> So just chassisType()?

Right.

>>> Source/WebCore/platform/glib/UserAgentGLib.cpp:145
>>> +    if (firstRun) {
>> 
>> Use std::once_flag
> 
> Just below WTF or in an existing file?

Since it's only used by glib, under WTF/wtf/glib I would say. See RAMSize or MemoryFootprint for examples.

[Jan-Michael Brummer]

Created attachment 403775 [details]
Patch

[Jan-Michael Brummer]

Created attachment 403776 [details]
Patch

[Jan-Michael Brummer]

Ok, I've integrated Carlos suggestions. Feel free to review again.

[Carlos Garcia Campos]

Comment on attachment 403776 [details]
Patch

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

> Source/WTF/wtf/PlatformGTK.cmake:27
> +    glib/ChassisType.cpp

You have to add the files to PlatformWPE.cmake too.

> Source/WTF/wtf/glib/ChassisType.cpp:88
> +    } else {
> +        if (!g_error_matches(error.get(), G_FILE_ERROR, G_FILE_ERROR_NOENT))

else if?

> Source/WTF/wtf/glib/ChassisType.cpp:118
> +    } else {
> +        if (!g_error_matches(error.get(), G_FILE_ERROR, G_FILE_ERROR_NOENT))

Ditto.

> Source/WTF/wtf/glib/ChassisType.cpp:136
> +        if (chassisType == ChassisType::Unknown)
> +            chassisType = ChassisType::Desktop;

So, Unknown is never exposed, we always fallback to Desktop. I think it would be better to use Optional<> instead of adding the Unknown value to the enum. All read functions would return Optional<ChassisType>, returning WTF::nullopt when unknown.

[Jan-Michael Brummer]

Created attachment 403863 [details]
Patch

[Jan-Michael Brummer]

Makes sense, updated.

[Carlos Garcia Campos]

Comment on attachment 403863 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

This line should be removed, otherwise commit-queue will reject the patch.

> Source/WTF/wtf/glib/ChassisType.cpp:121
> +Optional<ChassisType> chassisType()

This shouldn't be optional since we are always falling back to Desktop, it never returns nullopt.

> Source/WTF/wtf/glib/ChassisType.cpp:123
> +    static Optional<ChassisType> chassisType;

Same here, we will always return a valid enum value.

> Source/WTF/wtf/glib/ChassisType.cpp:126
> +        chassisType = readMachineInfoChassisType();

We will need an optional here though.

> Source/WTF/wtf/glib/ChassisType.cpp:127
> +        if (chassisType == WTF::nullopt)

Don't compare to nullopt, use !chassisType

> Source/WTF/wtf/glib/ChassisType.cpp:132
> +        if (chassisType == WTF::nullopt)
> +            chassisType = ChassisType::Desktop;

And here we could do chassisType = optionalChassisType.valueOr(ChassisType::Desktop);

[Jan-Michael Brummer]

Created attachment 403865 [details]
Patch

[Jan-Michael Brummer]

Thanks Carlos, patch updated.

[Carlos Garcia Campos]

Comment on attachment 403865 [details]
Patch

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

> Source/WTF/wtf/glib/ChassisType.h:22
> +#include <wtf/Optional.h>

You don't need this include in the header, move it to the cpp, please.

[Jan-Michael Brummer]

Created attachment 403949 [details]
Patch

[Jan-Michael Brummer]

Moved include to cpp file.

[Carlos Garcia Campos]

Comment on attachment 403949 [details]
Patch

Excellent, thanks!

[EWS]

Committed r264213: <https://trac.webkit.org/changeset/264213>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403949 [details].