[webkit-reviews] review denied: [Bug 133850] [GTK] Implement button-press-event, button-release-event, and absolute-axis-event of GAMEPAD API. : [Attachment 400046] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied ChangSeok Oh
<changseok at webkit.org>'s request for review:
Bug 133850: [GTK] Implement button-press-event, button-release-event, and
absolute-axis-event of GAMEPAD API.
https://bugs.webkit.org/show_bug.cgi?id=133850

Attachment 400046: Patch

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




--- Comment #9 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 400046
  --> https://bugs.webkit.org/attachment.cgi?id=400046
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=400046&action=review

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:58
> +    if (!gamepad)
> +	   return;

I don't think this is possible, we will always get a non-null pointer here. To
ensure this is not called after the ManetteGamepad is destroyed, we need to
disconnect all the signals in destructor.

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:112
> +    if (!gamepad)
> +	   return;

Ditto.

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:124
> +    if (!gamepad)
> +	   return;

Ditto.

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:141
> +    m_id = String(manette_device_get_name(m_device.get()));

String::fromUTF8

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:145
> +    m_axisValues.resize(static_cast<size_t>(StandardGamepadAxis::Count));
> +    m_axisValues.fill(0);

You can pass a size to fill and it will be resized too.

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:149
> +    listenTo(device);

Do you plan to call this from more places? Because I would just connect the
signals here, making it clear they are connected on construction and
disconnected on destruction.

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:156
> +    g_signal_connect(G_OBJECT(device), "button-press-event",
(GCallback)onButtonPressEvent, this);

g_signal_connect expects a gpointer so tou don't need the GObject cast. Use
G_CALLBACK() instead of the C-style cast.

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:158
> +    g_signal_connect(G_OBJECT(device), "absolute-axis-event",
(GCallback)onAbsoluteAxisEvent, this);

And disconnect the signals in the destructor.

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:163
> +    if (button == StandardGamepadButton::Unknown || m_device.get() !=
device)

How can we get a signal for a different device?

> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.h:54
> +    void gamepadHadInput(ManetteGamepad&, bool hadButtonPresses);

Use an enum instead of a boolean parameter to improve the readability.
Something like enum class ShouldMakeGamepadsVisible { No, Yes };