[webkit-reviews] review granted: [Bug 134374] HIDGamepadProvider should only be active when someone is interested in Gamepads : [Attachment 233959] Patch v2 - Alpha fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 134374: HIDGamepadProvider should only be active when someone is interested
in Gamepads
https://bugs.webkit.org/show_bug.cgi?id=134374

Attachment 233959: Patch v2 - Alpha fix
https://bugs.webkit.org/attachment.cgi?id=233959&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=233959&action=review


> Source/WebCore/platform/mac/HIDGamepadProvider.cpp:83
> +    , m_connectionDelayTimer(this,
&HIDGamepadProvider::connectionDelayTimerFired)

Might be better to use the new style lambda-based timer rather than the old
pointer-to-member-function one. I think Anders added that recently.

> Source/WebCore/platform/mac/HIDGamepadProvider.cpp:114
> +void HIDGamepadProvider::openAndScheduleManager()

Should this function assert that m_gamepadVector and m_gamepadMap are both
empty?

> Source/WebCore/platform/mac/HIDGamepadProvider.cpp:127
> +void HIDGamepadProvider::closeAndUnscheduleManager()

Should this function also stop m_connectionDelayTimer?

> Source/WebCore/platform/mac/HIDGamepadProvider.cpp:143
>      m_clients.add(client);
> +
> +    if (m_clients.size() == 1)
> +	   openAndScheduleManager();

This isn’t ideal, because add will silently do nothing if we re-add an existing
client. It would be better to do this work only if the client dictionary was
empty. Checking that size is 1 afterwards is not quite the same thing if we
have some problem where we re-add the same client, so it would be nicer to just
check beforehand and store it in a boolean rather than checking for a size of
1. We don’t even have an assertion here that this client isn’t already in
m_clients.

> Source/WebCore/platform/mac/HIDGamepadProvider.cpp:150
>      m_clients.remove(client);
> +
> +    if (!m_clients.size())
> +	   closeAndUnscheduleManager();

It would be better to do this only if the remove function returned true for the
same reason I suggested the change above; if the client isn’t already in the
dictionary, it will return false. You could even do it with an early return if
you like. The clients dictionary might already be empty, and it’s harmless to
remove a client that isn’t in there except for the closeAndUnscheuleManager
work.

I think it’s better to code in a way that’s resilient to these strange cases.
Also probably good to assert contains here and assert !contains in
startMonitoringGamepads too.

> Source/WebCore/platform/mac/HIDGamepadProvider.cpp:192
> +    // Any time we get a device removed callback we know its a real event
and not an 'already connected' event.

Should be "it's" with an apostrophe.