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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jun 29 13:14:10 PDT 2020


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 403095: Patch

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




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


More information about the webkit-reviews mailing list