Bug 134374

Summary: HIDGamepadProvider should only be active when someone is interested in Gamepads
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebCore Misc.Assignee: Brady Eidson <beidson>
Severity: Normal CC: bunhere, cdumez, commit-queue, dino, gyuyoung.kim, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: All   
Bug Depends on:    
Bug Blocks: 134076    
Description Flags
Patch v1
Patch v2 - Alpha fix darin: review+, beidson: commit-queue?

Description Brady Eidson 2014-06-26 18:28:08 PDT
HIDGamepadProvider should only be active when someone is interested in Gamepads
Comment 1 Brady Eidson 2014-06-26 21:10:04 PDT
Created attachment 233958 [details]
Patch v1
Comment 2 WebKit Commit Bot 2014-06-26 21:11:15 PDT
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.
Comment 3 Brady Eidson 2014-06-26 21:17:14 PDT
Created attachment 233959 [details]
Patch v2 - Alpha fix
Comment 4 Darin Adler 2014-06-27 09:51:27 PDT
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.
Comment 5 Brady Eidson 2014-06-27 13:36:35 PDT