Bug 134374 – HIDGamepadProvider should only be active when someone is interested in Gamepads

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

HIDGamepadProvider should only be active when someone is interested in Gamepads

[Brady Eidson]

Created attachment 233958 [details]
Patch v1

[WebKit Commit Bot]

Attachment 233958 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mac/HIDGamepadProvider.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

Created attachment 233959 [details]
Patch v2 - Alpha fix

[Darin Adler]

Comment on attachment 233959 [details]
Patch v2 - Alpha fix

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.

[Brady Eidson]

http://trac.webkit.org/changeset/170549