[webkit-reviews] review denied: [Bug 149496] [GTK] Use mobile user-agent on tablet : [Attachment 403492] 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 403492: Patch

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




--- Comment #17 from Michael Catanzaro <mcatanzaro at gnome.org> ---
Comment on attachment 403492
  --> https://bugs.webkit.org/attachment.cgi?id=403492
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.