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

[bugzilla-daemon at webkit.org]

Adrian Perez <aperez at igalia.com> has denied  review:
Bug 149496: [GTK] Use mobile user-agent on tablet
https://bugs.webkit.org/show_bug.cgi?id=149496

Attachment 403517: Patch

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




--- Comment #22 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 403517
  --> https://bugs.webkit.org/attachment.cgi?id=403517
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.