[webkit-reviews] review denied: [Bug 87503] [Gtk] Add support for the Gamepad API : [Attachment 145856] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Zan Dobersek
<zandobersek at gmail.com>'s request for review:
Bug 87503: [Gtk] Add support for the Gamepad API
https://bugs.webkit.org/show_bug.cgi?id=87503

Attachment 145856: Patch
https://bugs.webkit.org/attachment.cgi?id=145856&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=145856&action=review


It looks much better. Setting r- because of the memory leaks, coding style
issues and other minor issues.

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:56
> +    GSource* m_source;

You should use GRefPtr for the source too, g_source_destroy() removed the
source from the context and marks it as destroyed, but the memory is not freed.


> Source/WebCore/platform/gtk/GamepadsGtk.cpp:62
> +    , m_inputStream(0)

This is not needd GRefPtr already initializes its internal pointer to 0

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:68
> +    m_inputStream = g_unix_input_stream_new(m_fileDescriptor, TRUE);

You should adopt the reference with adoptGRef(). It's a bit weird that this
clases closes the descriptor that was open by the parent class. Even if it's
conveneint, I guess it would be better to pass FALSE here, close the descriptor
in the parent destructor.

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:69
> +    m_source =
g_pollable_input_stream_create_source(G_POLLABLE_INPUT_STREAM(m_inputStream.get
()), 0);

Use adoptGRef here too when m_source is changed to GRefPtr

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:71
> +    g_source_attach(m_source, NULL);

Use 0 instead of NULL.

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:80
> +void GamepadDeviceGtk::readFromInputStream()

Make this return a bool and return false if an error happens, that way we can
cancel the source in case of errors

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:101
> +    gamepadDevice->readFromInputStream();
> +
> +    return TRUE;

And this would be return gamepadDevice->readFromInputStream(); Or maybe we can
read the stream here, since we have a pointer to the stream, and simply call
gamepadDevice->updateForEvent(event);

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:119
> +    Vector<OwnPtr<GamepadDeviceGtk> > m_slots;
> +    HashMap<String, GamepadDeviceGtk*> m_deviceMap;

Why do we need both the Vector and the HashMap, isn't it enough with the
HashMap

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:131
> +    for (unsigned i = 0; i < m_length; i++)
> +	   m_slots.append(GamepadDeviceGtk::create());

Why not creating the devices on demand?

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:134
> +    m_gudevClient = g_udev_client_new(subsystems);

You should use adoptGRef() here too

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:137
> +    GList* devicesList =
g_udev_client_query_by_subsystem(m_gudevClient.get(), subsystems[0]);

You can use GOwnPtr for the list, since the items are already freed in the loop


> Source/WebCore/platform/gtk/GamepadsGtk.cpp:139
> +	   GUdevDevice* device =
reinterpret_cast<GUdevDevice*>(listItem->data);

For GObjects it's better to use the GObject cast macros, that also check the
glib type. G_UDEV_DEVICE(listIem->data) in this case

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:167
> +    m_slots[index] = GamepadDeviceGtk::create(deviceFile);

m_slots[index] already contains a GamepadDeviceGtk

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:192
> +	       m_slots[i]->updateGamepad(gamepad.get());

I think it would be clearer to add getters to GamepadDevice class and fill the
Gaempad object here using those getters, similar to what chromium does.

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:205
> +    String deviceFile = String(g_udev_device_get_device_file(device));

I guess g_udev_device_get_device_file() returns an UTF8 string, so you should
use String::fromUTF8(). But I think it would be better to pass the returned
char * to the registerDevice and unregisterDevice methods, and convert to
String when accesing the HashMap.

> Source/WebCore/platform/linux/GamepadDeviceLinux.cpp:39
> +    : m_deviceFile(deviceFile)

Do we need to keep the deviceFile? Maybe we could use CString instead of String
mor simply pass the char * and use it directly in open().

> Source/WebCore/platform/linux/GamepadDeviceLinux.cpp:47
> +    if (isNull())
> +	   return;

I still don't get why we want null devices.

> Source/WebCore/platform/linux/GamepadDeviceLinux.cpp:73
> +    gamepad->id(m_deviceName);
> +    gamepad->timestamp(m_lastTimestamp);
> +    gamepad->axes(m_numberOfAxes, m_axes.data());
> +    gamepad->buttons(m_numberOfButtons, m_buttons.data());

As I said I thnk this would be clearer in the caller, adding getters for
m_deviceName, m_lastTimestamp, axes and buttons.