[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 #403095|review?                     |review-
              Flags|                            |

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

-- 
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/20200629/319593a6/attachment-0001.htm>