Bug 134374 - HIDGamepadProvider should only be active when someone is interested in Gamepads
Summary: HIDGamepadProvider should only be active when someone is interested in Gamepads
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks: 134076
  Show dependency treegraph
 
Reported: 2014-06-26 18:28 PDT by Brady Eidson
Modified: 2014-06-27 13:36 PDT (History)
6 users (show)

See Also:


Attachments
Patch v1 (10.26 KB, patch)
2014-06-26 21:10 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch v2 - Alpha fix (10.26 KB, patch)
2014-06-26 21:17 PDT, Brady Eidson
darin: review+
beidson: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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
http://trac.webkit.org/changeset/170549