Fix PepperGamepadHostTest.WaitForReply

Fix PepperGamepadHostTest.WaitForReply

Fixup after "Gamepad: don't notify about connected pads twice".
The CHECK(!g_gamepad_service) I added is failing because during
unit tests several instance can be created and destroyed.

R=bajones@chromium.org
BUG=391503
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281959

[bajones, 2014-07-08 15:59:00]

https://codereview.chromium.org/372053004/diff/1/content/browser/gamepad/game...
File content/browser/gamepad/gamepad_service.cc (right):

https://codereview.chromium.org/372053004/diff/1/content/browser/gamepad/game...
content/browser/gamepad/gamepad_service.cc:41: CHECK(!!instance !=
!!g_gamepad_service);
This reads really awkwardly, and makes me question if the logic is right. The
check currently ensures that you are going from a NULL service pointer to a
non-NULL one or vica-versa. Is that desired? Is there a reason why
CHECK(instance != g_gamepad_service) isn't appropriate to prevent repeated sets?

[kbalazs, 2014-07-08 19:23:10]

On 2014/07/08 15:59:00, bajones wrote:
>
https://codereview.chromium.org/372053004/diff/1/content/browser/gamepad/game...
> File content/browser/gamepad/gamepad_service.cc (right):
> 
>
https://codereview.chromium.org/372053004/diff/1/content/browser/gamepad/game...
> content/browser/gamepad/gamepad_service.cc:41: CHECK(!!instance !=
> !!g_gamepad_service);
> This reads really awkwardly, and makes me question if the logic is right. The
> check currently ensures that you are going from a NULL service pointer to a
> non-NULL one or vica-versa. Is that desired? Is there a reason why
> CHECK(instance != g_gamepad_service) isn't appropriate to prevent repeated
sets?

No, that is not enough. You could create another object in the unit tests before
destroying the currently set one and that check would not catch it. I can add a
comment :) Note that this is only an issue for unit test classes that are
friends of GamepadService, otherwise the constructor is private and we only
create one object via GetInstance and never destroy it.

[bajones, 2014-07-08 19:46:33]

On 2014/07/08 at 19:23:10, b.kelemen wrote:
> On 2014/07/08 15:59:00, bajones wrote:
> >
https://codereview.chromium.org/372053004/diff/1/content/browser/gamepad/game...
> > File content/browser/gamepad/gamepad_service.cc (right):
> > 
> >
https://codereview.chromium.org/372053004/diff/1/content/browser/gamepad/game...
> > content/browser/gamepad/gamepad_service.cc:41: CHECK(!!instance !=
> > !!g_gamepad_service);
> > This reads really awkwardly, and makes me question if the logic is right.
The
> > check currently ensures that you are going from a NULL service pointer to a
> > non-NULL one or vica-versa. Is that desired? Is there a reason why
> > CHECK(instance != g_gamepad_service) isn't appropriate to prevent repeated
sets?
> 
> No, that is not enough. You could create another object in the unit tests
before destroying the currently set one and that check would not catch it. I can
add a comment :) Note that this is only an issue for unit test classes that are
friends of GamepadService, otherwise the constructor is private and we only
create one object via GetInstance and never destroy it.

Ah, I see.

LGTM with the comment added.

[kbalazs, 2014-07-08 20:49:25]

The CQ bit was checked by b.kelemen@samsung.com

[kbalazs, 2014-07-08 20:49:43]

The CQ bit was unchecked by b.kelemen@samsung.com

[kbalazs, 2014-07-08 23:44:15]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-07-08 23:47:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/372053004/20001

[commit-bot: I haz the power, 2014-07-09 05:41:50]

Change committed as 281959