Gamepad: add test support for page visibility behavior

Gamepad: make page visibility behavior layout testable

This CL refactors testing infrastructure for gamepad so that we can
actually write a layout test that would fail if Blink doesn't honor
visibility state. Now Platform::setGamepadListener is now hooked to
GamepadController.

BUG=344556
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278723

[kbalazs, 2014-05-29 22:31:35]

With this I will be able to add a test case for a minor issue in blink
regardning gamepad and page visibility. As I was doing it I decided that I try
to make it cover how gamepad should work regarding to page visibility more or
less completely.

[kbalazs, 2014-05-31 00:28:06]

Take this as WIP, I'm reworking it.

[kbalazs, 2014-06-02 17:26:08]

Rework done, PTAL

[kbalazs, 2014-06-03 21:45:54]

On 2014/06/02 17:26:08, kbalazs wrote:
> Rework done, PTAL

Changed my mind again. Actually there is another edge case so we need to handle
part of it in Blink. But than we shouldn't bother with the pending events in
GamepadController. So this is still not final.

[kbalazs, 2014-06-05 22:46:23]

This is now ready together with https://codereview.chromium.org/306363002/

[kbalazs, 2014-06-13 00:09:21]

On 2014/06/05 22:46:23, kbalazs wrote:
> This is now ready together with https://codereview.chromium.org/306363002/

In case you are concerned, this works fine with the current tests without any
Blink changes, so there is no dependency, we can go ahead with this. In other
words: PTAL

[bajones, 2014-06-13 00:16:44]

On 2014/06/13 at 00:09:21, b.kelemen wrote:
> On 2014/06/05 22:46:23, kbalazs wrote:
> > This is now ready together with https://codereview.chromium.org/306363002/
> 
> In case you are concerned, this works fine with the current tests without any
Blink changes, so there is no dependency, we can go ahead with this. In other
words: PTAL

Not an owner of any of the files involved, but the overall logic LGTM.

[jam, 2014-06-18 23:23:43]

https://codereview.chromium.org/304403002/diff/100001/content/public/renderer...
File content/public/renderer/renderer_gamepad_provider.h (right):

https://codereview.chromium.org/304403002/diff/100001/content/public/renderer...
content/public/renderer/renderer_gamepad_provider.h:18: virtual void
SetGamepadListener(blink::WebGamepadListener*) = 0;
nit: name the parameter. also add comment for methods and class

https://codereview.chromium.org/304403002/diff/100001/content/renderer/gamepa...
File content/renderer/gamepad_shared_memory_reader.h (right):

https://codereview.chromium.org/304403002/diff/100001/content/renderer/gamepa...
content/renderer/gamepad_shared_memory_reader.h:22: , public
RendererGamepadProvider {
ditto

https://codereview.chromium.org/304403002/diff/100001/content/shell/renderer/...
File content/shell/renderer/DEPS (right):

https://codereview.chromium.org/304403002/diff/100001/content/shell/renderer/...
content/shell/renderer/DEPS:2: "+content/renderer",
this isn't allowed. content/shell is just an embedder of content, so it can only
include stuff from content/public. i don't actually see any new includes to
content/renderer though, why add this?

https://codereview.chromium.org/304403002/diff/100001/content/shell/renderer/...
File content/shell/renderer/test_runner/WebTestDelegate.h (right):

https://codereview.chromium.org/304403002/diff/100001/content/shell/renderer/...
content/shell/renderer/test_runner/WebTestDelegate.h:44: // XXX cm
?

https://codereview.chromium.org/304403002/diff/100001/content/shell/renderer/...
File content/shell/renderer/test_runner/gamepad_controller.h (right):

https://codereview.chromium.org/304403002/diff/100001/content/shell/renderer/...
content/shell/renderer/test_runner/gamepad_controller.h:25: , public
RendererGamepadProvider {
nit: use google style comma on previous line)

https://codereview.chromium.org/304403002/diff/100001/content/shell/renderer/...
content/shell/renderer/test_runner/gamepad_controller.h:36: virtual void
SetGamepadListener(blink::WebGamepadListener*);
nit: google style is to put the parameter name here

[kbalazs, 2014-06-19 00:18:41]

https://codereview.chromium.org/304403002/diff/100001/content/shell/renderer/...
File content/shell/renderer/DEPS (right):

https://codereview.chromium.org/304403002/diff/100001/content/shell/renderer/...
content/shell/renderer/DEPS:2: "+content/renderer",
On 2014/06/18 23:23:42, jam wrote:
> this isn't allowed. content/shell is just an embedder of content, so it can
only
> include stuff from content/public. i don't actually see any new includes to
> content/renderer though, why add this?

Yep, it was just a left behind from an early version. All comments addressed,
PTAL.

[jam, 2014-06-19 16:21:11]

lgtm with nits

https://codereview.chromium.org/304403002/diff/100001/content/renderer/gamepa...
File content/renderer/gamepad_shared_memory_reader.h (right):

https://codereview.chromium.org/304403002/diff/100001/content/renderer/gamepa...
content/renderer/gamepad_shared_memory_reader.h:22: , public
RendererGamepadProvider {
On 2014/06/18 23:23:42, jam wrote:
> ditto

this was referring to the ','

https://codereview.chromium.org/304403002/diff/120001/content/public/renderer...
File content/public/renderer/renderer_gamepad_provider.h (right):

https://codereview.chromium.org/304403002/diff/120001/content/public/renderer...
content/public/renderer/renderer_gamepad_provider.h:20: // Registers listener
for be notified of events.
nit: blank line above

https://codereview.chromium.org/304403002/diff/120001/content/public/renderer...
content/public/renderer/renderer_gamepad_provider.h:22: protected:
also above this

[kbalazs, 2014-06-19 19:04:04]

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

[commit-bot: I haz the power, 2014-06-19 19:05:43]

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

[kbalazs, 2014-06-19 22:43:17]

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

[commit-bot: I haz the power, 2014-06-19 22:43:26]

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

[commit-bot: I haz the power, 2014-06-20 06:59:08]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-20 07:13:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-20 07:13:04]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[kbalazs, 2014-06-20 13:45:27]

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

[commit-bot: I haz the power, 2014-06-20 13:46:57]

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

[commit-bot: I haz the power, 2014-06-20 16:57:09]

Change committed as 278723

[loislo, 2014-06-23 06:46:04]

On 2014/06/20 16:57:09, I haz the power (commit-bot) wrote:
> Change committed as 278723

ASAN bot is unhappy with this patch because TestInterfaces calls SetDelegate
with null pointer.

23:00:40.631 5143   ==13==ERROR: AddressSanitizer: SEGV on unknown address
0x000000000000 (pc 0x000000616467 sp 0x7fffcb97f160 bp 0x7fffcb97f160 T0)
23:00:40.631 5143       #0 0x616466 in
content::GamepadController::SetDelegate(content::WebTestDelegate*)
/mnt/data/b/build/slave/WebKit_Linux_ASAN/build/src/out/Release/../../content/shell/renderer/test_runner/gamepad_controller.cc:157:0
23:00:40.631 5143       #1 0x5d901d in
content::TestInterfaces::~TestInterfaces()
/mnt/data/b/build/slave/WebKit_Linux_ASAN/build/src/out/Release/../../content/shell/renderer/test_runner/TestInterfaces.cpp:59:0
23:00:40.631 5143       #2 0x58fb35 in
base::DefaultDeleter<content::TestInterfaces>::operator()(content::TestInterfaces*)
const
/mnt/data/b/build/slave/WebKit_Linux_ASAN/build/src/out/Release/../../base/memory/scoped_ptr.h:137:0
2

[kbalazs, 2014-06-27 21:14:56]

On 2014/06/23 06:46:04, loislo wrote:
> On 2014/06/20 16:57:09, I haz the power (commit-bot) wrote:
> > Change committed as 278723
> 
> ASAN bot is unhappy with this patch because TestInterfaces calls SetDelegate
> with null pointer.
> 
> 23:00:40.631 5143   ==13==ERROR: AddressSanitizer: SEGV on unknown address
> 0x000000000000 (pc 0x000000616467 sp 0x7fffcb97f160 bp 0x7fffcb97f160 T0)
> 23:00:40.631 5143       #0 0x616466 in
> content::GamepadController::SetDelegate(content::WebTestDelegate*)
>
/mnt/data/b/build/slave/WebKit_Linux_ASAN/build/src/out/Release/../../content/shell/renderer/test_runner/gamepad_controller.cc:157:0
> 23:00:40.631 5143       #1 0x5d901d in
> content::TestInterfaces::~TestInterfaces()
>
/mnt/data/b/build/slave/WebKit_Linux_ASAN/build/src/out/Release/../../content/shell/renderer/test_runner/TestInterfaces.cpp:59:0
> 23:00:40.631 5143       #2 0x58fb35 in
>
base::DefaultDeleter<content::TestInterfaces>::operator()(content::TestInterfaces*)
> const
>
/mnt/data/b/build/slave/WebKit_Linux_ASAN/build/src/out/Release/../../base/memory/scoped_ptr.h:137:0
> 2

This has been fixed by a blink side revert and reland with fix. Now this cannot
happen in practice, although technically it would be cleaner to not rely blink
behavior, so I should add some null checks.

[kbalazs, 2014-06-27 21:16:15]

On 2014/06/27 21:14:56, kbalazs wrote:
> On 2014/06/23 06:46:04, loislo wrote:
> > On 2014/06/20 16:57:09, I haz the power (commit-bot) wrote:
> > > Change committed as 278723
> > 
> > ASAN bot is unhappy with this patch because TestInterfaces calls SetDelegate
> > with null pointer.
> > 
> > 23:00:40.631 5143   ==13==ERROR: AddressSanitizer: SEGV on unknown address
> > 0x000000000000 (pc 0x000000616467 sp 0x7fffcb97f160 bp 0x7fffcb97f160 T0)
> > 23:00:40.631 5143       #0 0x616466 in
> > content::GamepadController::SetDelegate(content::WebTestDelegate*)
> >
>
/mnt/data/b/build/slave/WebKit_Linux_ASAN/build/src/out/Release/../../content/shell/renderer/test_runner/gamepad_controller.cc:157:0
> > 23:00:40.631 5143       #1 0x5d901d in
> > content::TestInterfaces::~TestInterfaces()
> >
>
/mnt/data/b/build/slave/WebKit_Linux_ASAN/build/src/out/Release/../../content/shell/renderer/test_runner/TestInterfaces.cpp:59:0
> > 23:00:40.631 5143       #2 0x58fb35 in
> >
>
base::DefaultDeleter<content::TestInterfaces>::operator()(content::TestInterfaces*)
> > const
> >
>
/mnt/data/b/build/slave/WebKit_Linux_ASAN/build/src/out/Release/../../base/memory/scoped_ptr.h:137:0
> > 2
> 
> This has been fixed by a blink side revert and reland with fix. Now this
cannot
> happen in practice, although technically it would be cleaner to not rely blink
> behavior, so I should add some null checks.

Err, @loislo already handled that, I forgot... Thanks again.