[Webkit-unassigned] [Bug 149496] [GTK] Use mobile user-agent on tablet

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=149496

Michael Catanzaro <mcatanzaro at gnome.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #403492|review?                     |review-
              Flags|                            |

--- 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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200703/bd4e9214/attachment-0001.htm>