Fix HidService lifetime issues.

Fix HidService lifetime issues.

This reverts HidService to a singleton instance, adding two gross
and presently hard-to-avoid properties:

1. Its instance getter takes a UI message loop proxy
2. Its lifetime is bound by the message loop of its origin thread

The purpose of this is to preserve the necessary threading discipline
required by both the chrome.hid API implementation and the
permission_broker DBus client while ensuring that the HidService is
created and destroyed on the same thread.

BUG=401234
Committed: https://crrev.com/5ad6c89c9721d23519c062919d1d2c951aaea921
Cr-Commit-Position: refs/heads/master@{#292273}

[Ken Rockot(use gerrit already), 2014-08-12 17:01:06]

Renaud could you please take a look?

Also +Reilly for comments/FYI

[Reilly Grant (use Gerrit), 2014-08-12 17:12:18]

https://codereview.chromium.org/464753002/diff/20001/device/hid/hid_connectio...
File device/hid/hid_connection_unittest.cc (right):

https://codereview.chromium.org/464753002/diff/20001/device/hid/hid_connectio...
device/hid/hid_connection_unittest.cc:60: message_loop_->message_loop_proxy()));
This line points out exactly what I'm going to need to fix in these tests: The
message loop created above is a MessageLoopForIO but what HidService really
wants is a MessageLoopForUI. It's okay for now but once I fix these tests to
create two separate message loops we may want to put checks in
HidService::GetInstance to be sure that the message loop provided is of the
right type.

[rpaquay, 2014-08-26 17:35:12]

lgtm with a couple of questions.

https://codereview.chromium.org/464753002/diff/20001/device/hid/hid_service.cc
File device/hid/hid_service.cc (right):

https://codereview.chromium.org/464753002/diff/20001/device/hid/hid_service.c...
device/hid/hid_service.cc:46: delete this;
Are you sure "delete this" is ok here? I am not too familiar with
DestructionObserver behavior, but I don't see any other implementation having a
"delete this" inside.

https://codereview.chromium.org/464753002/diff/20001/device/hid/hid_service.c...
device/hid/hid_service.cc:58: if (g_service == NULL) {
Maybe it would it be desirable to add a "dcheck" somehow that "GetInstance" is
not called after "WillDestroyCurrentMessageLoop()" is called? (to avoid
accidentally re-creating after shutting down).

[Ken Rockot(use gerrit already), 2014-08-27 22:10:34]

On 2014/08/26 17:35:12, rpaquay wrote:
> lgtm with a couple of questions.
> 
> https://codereview.chromium.org/464753002/diff/20001/device/hid/hid_service.cc
> File device/hid/hid_service.cc (right):
> 
>
https://codereview.chromium.org/464753002/diff/20001/device/hid/hid_service.c...
> device/hid/hid_service.cc:46: delete this;
> Are you sure "delete this" is ok here? I am not too familiar with
> DestructionObserver behavior, but I don't see any other implementation having
a
> "delete this" inside.

Yeah, it's OK. UsbServiceImpl effectively does the same thing by setting its
global scoped_ptr to NULL which in turn deletes the calling UsbServiceImpl
(which is itself a DestructionObserver).

> 
>
https://codereview.chromium.org/464753002/diff/20001/device/hid/hid_service.c...
> device/hid/hid_service.cc:58: if (g_service == NULL) {
> Maybe it would it be desirable to add a "dcheck" somehow that "GetInstance" is
> not called after "WillDestroyCurrentMessageLoop()" is called? (to avoid
> accidentally re-creating after shutting down).

Instead I reset the pointer to NULL. So if GetInstance is called after the
previous instance's message loop is destroyed, the service can be recreated on
the current thread. This should be safe if not necessarily useful.

[Ken Rockot(use gerrit already), 2014-08-27 22:10:39]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-08-27 22:11:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rockot@chromium.org/464753002/60001

[commit-bot: I haz the power, 2014-08-27 23:22:51]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[commit-bot: I haz the power, 2014-08-27 23:52:56]

Committed patchset #3 (id:60001) as 13aca592b24cdeb9b558716db71c48961e48066f

[eroman, 2014-08-28 00:15:19]

A revert of this CL (patchset #3) has been created in
https://codereview.chromium.org/512983002/ by eroman@chromium.org.

The reason for reverting is: Caused build failure:
https://codereview.chromium.org/464753002.

[awong, 2014-08-28 00:25:14]

On 2014/08/28 00:15:19, eroman wrote:
> A revert of this CL (patchset #3) has been created in
> https://codereview.chromium.org/512983002/ by mailto:eroman@chromium.org.
> 
> The reason for reverting is: Caused build failure:
> https://codereview.chromium.org/464753002.

Having problems with the CQ. Manually reverting the patch here:
  https://codereview.chromium.org/514493004

[commit-bot: I haz the power, 2014-09-10 02:55:49]

Patchset 3 (id:??) landed as
https://crrev.com/5ad6c89c9721d23519c062919d1d2c951aaea921
Cr-Commit-Position: refs/heads/master@{#292273}