Add GamepadService, owns 1 gamepad bg thread

Add GamepadService, owns 1 gamepad bg thread

BUG=79050
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112638

[scottmg, 2011-12-01 04:00:46]

As discussed: GamepadService owns the GamepadProvider (which is the background
polling thread), and refactor the GamepadProvider a bit to work with it.
Currently there is one browser thread per renderer-using-gamepad, which is
unnecessary. With this change, there will be a maximum of 1 browser thread.

Rather than spinning up/shutting down the thread when it's not in use, it's
created on first use, and then simply paused when there's no renderers using it.

It keeps track of the number of renderers using it by getting start messages
from the renderer, and by using NotificationService to know when a renderer has
closed. It doesn't rely on a "stop" message from the renderer so that crashing
renderers are handled.

[jam, 2011-12-01 20:07:31]

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
File content/browser/gamepad/gamepad_provider.cc (right):

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_provider.cc:12: #include
"base/synchronization/lock.h"
nit: not needed since in the header

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_provider.cc:59:
DCHECK(MessageLoop::current()->message_loop_proxy() == creator_loop_);
nit: creator_loop_ isn't needed anymore

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_provider.cc:62: if (!polling_thread_->Start()) {
can this really ever fail? most likely not (if threads aren't starting, the
system is really hosed). that way we can avoid writing the code to work around
it, which still leaves the feature broken.

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_provider.cc:91: base::AutoLock
lock(is_paused_lock_);
nit: better to use the lock for as little scope as possible, especially not when
posting tasks. can you make this ust around 98-101?

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_provider.cc:135:
devices_changed_lock_.Acquire();
nit: please use base::AutoLock and scope 137-142

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_provider.cc:152: base::AutoLock
lock(is_paused_lock_);
ditto for the post task in the lock

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
File content/browser/gamepad/gamepad_provider.h (right):

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_provider.h:36: void Pause();
nit: better to keep the comments :)

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_provider.h:57: base::Lock is_paused_lock_;
nit: can you add comments for the variables to describe what they're used for?

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
File content/browser/gamepad/gamepad_service.cc (right):

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_service.cc:27: instance_ = new GamepadService();
just use Singleton for singletons :)

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_service.cc:59: DCHECK(provider_);
nit: by convention we avoid DCHECKing variables before using them. in release
builds, this does nothing. in debug, the crash in the following line is just as
good

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_service.cc:65: DCHECK(provider_);
ditto

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_service.cc:75: switch (type) {
nit: just do line 77-80 only since you're registering for one notification

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
File content/browser/gamepad/gamepad_service.h (right):

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_service.h:25: public
base::RefCountedThreadSafe<GamepadService> {
nit: it's weird that this is both a singelton and refcounted. if you're doing
this just so you can do PostTasks, you can add

DISABLE_RUNNABLE_METHOD_REFCOUNT(GamepadService);

at the bottom of the file

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_service.h:40: // NotificationObserver overrides:
nit: can be private

http://codereview.chromium.org/8760023/diff/1/content/browser/renderer_host/g...
File content/browser/renderer_host/gamepad_browser_message_filter.h (right):

http://codereview.chromium.org/8760023/diff/1/content/browser/renderer_host/g...
content/browser/renderer_host/gamepad_browser_message_filter.h:10: #include
"base/shared_memory.h"
nit: forward declare instead

http://codereview.chromium.org/8760023/diff/1/content/browser/renderer_host/g...
content/browser/renderer_host/gamepad_browser_message_filter.h:20:
GamepadBrowserMessageFilter(RenderProcessHost* rph);
nit: explicit

http://codereview.chromium.org/8760023/diff/1/content/browser/renderer_host/g...
content/browser/renderer_host/gamepad_browser_message_filter.h:33:
GamepadService* gamepad_service_;
nit: no need to keep a pointer to a singleton. just use GetInstance every time
you need it

[scottmg, 2011-12-01 22:05:32]

thanks!

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
File content/browser/gamepad/gamepad_provider.cc (right):

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_provider.cc:12: #include
"base/synchronization/lock.h"
On 2011/12/01 20:07:31, John Abd-El-Malek wrote:
> nit: not needed since in the header

Done.

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_provider.cc:59:
DCHECK(MessageLoop::current()->message_loop_proxy() == creator_loop_);
On 2011/12/01 20:07:31, John Abd-El-Malek wrote:
> nit: creator_loop_ isn't needed anymore

Done.

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_provider.cc:62: if (!polling_thread_->Start()) {
On 2011/12/01 20:07:31, John Abd-El-Malek wrote:
> can this really ever fail? most likely not (if threads aren't starting, the
> system is really hosed). that way we can avoid writing the code to work around
> it, which still leaves the feature broken.

Done.

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_provider.cc:91: base::AutoLock
lock(is_paused_lock_);
On 2011/12/01 20:07:31, John Abd-El-Malek wrote:
> nit: better to use the lock for as little scope as possible, especially not
when
> posting tasks. can you make this ust around 98-101?

Done.

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_provider.cc:135:
devices_changed_lock_.Acquire();
On 2011/12/01 20:07:31, John Abd-El-Malek wrote:
> nit: please use base::AutoLock and scope 137-142

Done. Made a copy/reset in the scoped access to avoid calling any functions
while locked.

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_provider.cc:152: base::AutoLock
lock(is_paused_lock_);
On 2011/12/01 20:07:31, John Abd-El-Malek wrote:
> ditto for the post task in the lock

Done.

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
File content/browser/gamepad/gamepad_provider.h (right):

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_provider.h:36: void Pause();
On 2011/12/01 20:07:31, John Abd-El-Malek wrote:
> nit: better to keep the comments :)

Done.

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_provider.h:57: base::Lock is_paused_lock_;
On 2011/12/01 20:07:31, John Abd-El-Malek wrote:
> nit: can you add comments for the variables to describe what they're used for?

Done.

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
File content/browser/gamepad/gamepad_service.cc (right):

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_service.cc:27: instance_ = new GamepadService();
On 2011/12/01 20:07:31, John Abd-El-Malek wrote:
> just use Singleton for singletons :)

Done. (was because of the refcount cruft which is fixed now)

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_service.cc:59: DCHECK(provider_);
On 2011/12/01 20:07:31, John Abd-El-Malek wrote:
> nit: by convention we avoid DCHECKing variables before using them. in release
> builds, this does nothing. in debug, the crash in the following line is just
as
> good

Done.

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_service.cc:65: DCHECK(provider_);
On 2011/12/01 20:07:31, John Abd-El-Malek wrote:
> ditto

Done.

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_service.cc:75: switch (type) {
On 2011/12/01 20:07:31, John Abd-El-Malek wrote:
> nit: just do line 77-80 only since you're registering for one notification

Done.

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
File content/browser/gamepad/gamepad_service.h (right):

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_service.h:25: public
base::RefCountedThreadSafe<GamepadService> {
On 2011/12/01 20:07:31, John Abd-El-Malek wrote:
> nit: it's weird that this is both a singelton and refcounted. if you're doing
> this just so you can do PostTasks, you can add
> 
> DISABLE_RUNNABLE_METHOD_REFCOUNT(GamepadService);
> 
> at the bottom of the file

Ah, yes, that's why I did that craziness. After poking around a bit, it seems
it's changed since the new Bind() stuff was added. Rather than that macro, it
looks like the thing to do now is to wrap the object pointer in the Bind call
with Unretained().

http://codereview.chromium.org/8760023/diff/1/content/browser/gamepad/gamepad...
content/browser/gamepad/gamepad_service.h:40: // NotificationObserver overrides:
On 2011/12/01 20:07:31, John Abd-El-Malek wrote:
> nit: can be private

Done.

http://codereview.chromium.org/8760023/diff/1/content/browser/renderer_host/g...
File content/browser/renderer_host/gamepad_browser_message_filter.h (right):

http://codereview.chromium.org/8760023/diff/1/content/browser/renderer_host/g...
content/browser/renderer_host/gamepad_browser_message_filter.h:10: #include
"base/shared_memory.h"
On 2011/12/01 20:07:31, John Abd-El-Malek wrote:
> nit: forward declare instead

Unfortunately SharedMemoryHandle is a typedef.

http://codereview.chromium.org/8760023/diff/1/content/browser/renderer_host/g...
content/browser/renderer_host/gamepad_browser_message_filter.h:20:
GamepadBrowserMessageFilter(RenderProcessHost* rph);
On 2011/12/01 20:07:31, John Abd-El-Malek wrote:
> nit: explicit

Done.

http://codereview.chromium.org/8760023/diff/1/content/browser/renderer_host/g...
content/browser/renderer_host/gamepad_browser_message_filter.h:33:
GamepadService* gamepad_service_;
On 2011/12/01 20:07:31, John Abd-El-Malek wrote:
> nit: no need to keep a pointer to a singleton. just use GetInstance every time
> you need it

Done.

[jam, 2011-12-01 23:39:21]

lgtm

[commit-bot: I haz the power, 2011-12-01 23:45:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/8760023/3003

[commit-bot: I haz the power, 2011-12-01 23:45:20]

Can't apply patch for file
content/browser/renderer_host/render_process_host_impl.cc.
While running patch -p1 --forward --force;
patching file content/browser/renderer_host/render_process_host_impl.cc
Hunk #1 FAILED at 533.
1 out of 1 hunk FAILED -- saving rejects to file
content/browser/renderer_host/render_process_host_impl.cc.rej

[commit-bot: I haz the power, 2011-12-01 23:48:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/8760023/4004

[commit-bot: I haz the power, 2011-12-02 03:24:15]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is win_rel, revision is 112625, job name
was 8760023-4004 (previous was lost) (retry) (previous was lost) (retry).

[commit-bot: I haz the power, 2011-12-02 03:29:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/8760023/4004

[commit-bot: I haz the power, 2011-12-02 04:41:26]

Change committed as 112638

[Lei Zhang, 2011-12-02 07:05:01]

FYI, this happened once so far:
http://build.chromium.org/p/chromium/builders/Mac10.5%20Tests%20%282%29/build...