Gamepad: make gamepad events play well with page visibility

Gamepad: make gamepad events play well with page visibility

This CL attempts to guarantee that we inform the page about every device
change and do it without sending events while the page is hidden.
For the first claim to be true we need to compare the current state with
the state the had the page had seen the last time when it was visible.
Note that the browser side might suspend the gamepad thread so simply
queuing the events is not enough.

R=bajones@chromium.org,abarth@chromium.org
BUG=386846
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177874

[abarth-chromium, 2014-06-24 02:17:48]

This looks like the wrong place to do this work.  Do we need to do this work for
all the other device listeners that work like gamepad?

[kbalazs, 2014-06-24 19:51:59]

On 2014/06/24 02:17:48, abarth wrote:
> This looks like the wrong place to do this work.  Do we need to do this work
for
> all the other device listeners that work like gamepad?

I don't think any other device listeners has similar requirement. The concept of
multiple sources, and tracking their connected/disconnected state is specific to
gamepad. (Also the "main" api of gamepad is the polling api, it is only the
gamepadconnected/gamepaddisconnected events that need device listener like
behavior.)

I think we need this for gamepad events because the events are only useful if
the page can rely all the time on them to track which devices are connected. If
there are scenarios when it doesn't work, you need to do it in js anyways, but
than there is no use of it to be part of the api.

[abarth-chromium, 2014-06-24 19:59:26]

Ok, that makes sense.

It seems like there should be a better way to structure this API.  Why is the
code for generating these events different when we wake up from being invisible
and when we get updated data from the embedder.  In principle, it seems like
those are two instances of the same occurrence: new data is available.

[kbalazs, 2014-06-26 21:32:15]

Some discussion from irc for reference:

<kbalazs> hi abarth
<abarth> kbalazs: hi
<kbalazs> abarth: gamepad... so normally chrome just sends us the events and we
just dispatch them to the page
<abarth> ok
<abarth> but this adds a batching notion
<abarth> do you think the page can observe the difference between batched and
non-batched events?
<kbalazs> abarth: but than we don't dispatch when the page is not visible, as we
normally shouldn't call js in hidden state
<abarth> that makes senes
<abarth> sense
<kbalazs> abarth: no, it shouldn't be able to observe this,
<abarth> I bet I can write a web page that distinguishes them
<kbalazs> abarth: yes, sure, it's not hard of course
<kbalazs> I meant we don't give a clear notion about it but sure it can be
guessed
<abarth> ok, then it is possible to observe the difference
<abarth> we should design the system so that the web page can't tell how we've
implemented it
<abarth> that way we're free to change the implementation in the future
<abarth> and we won't break web pages
<abarth> e.g., whether we've batched in Blink or in Chromium
<kbalazs> abarth: hm, that's sounds quite strict to me. if something is not part
of the api than why we are not free to change it any time?
<abarth> because we'll break web pages
<kbalazs> abarth: yes, web pages that rely on an implementation detail that is
not documented and not guaranteed. in this particular case, why would anybody do
this? :)
<abarth> welcome to the web
<abarth> lots of web sites rely upon undocumented and non-guaranteed behavior
<abarth> (it's actually not specific to the web---it's true of many computing
systems)
<kbalazs> abarth: sure, I got your general idea, I'm just not sure how does it
cause a problem here. as for the example, if we change it to do the batching in
chromium that change shouldn't be observable
<abarth> i bet I can write a web page that distinguishes the two implementations
<abarth> specifically, line 254 of NavigatorGamepad.cpp in your CL
<abarth> is observable
<abarth> you're returning a novel JS object during the batch update
<abarth> which I can observe via ===
<abarth> an implementation on the Chromium side of the API wouldn't be able to
do that
<kbalazs> abarth: hmmm, right, I didn't think about that. I can change it to
store a copy of the data in visibilityStateChanged
<abarth> I'd recommend changing the design so that visibility changing isn't a
special case
<abarth> that code path isn't going to be well tested or happen very often in
the real world
<abarth> which means it will have bugs
<abarth> worse, if its existence is observable to web pages, they'll have bugs
<abarth> that only manifest when users switch tabs at critical times
<kbalazs> abarth: I see, I just don't think there is a solution. we either
dispatch while hidden, or special case, or let this api keep being unuseful
<abarth> why isn't there a solution?
<abarth> what happens if the OS blocks on NFS for 10 minutes
<abarth> and then notices that a bunch of gamepad state has changed?
<abarth> presumably some sequence of things will happen that ought to be the
same as the behavior we want in this case
<kbalazs> abarth: I'm not sure I can follow you :) yes, this scenario can lead
to similar observable behavior as my patch, but what's the point?
<abarth> My point is that it's not an impossible problem to solve.  You just
need to use a different design for the implementation.
<kbalazs> abarth: I think this is simply logically not possible to 1 not special
case visibility + 2 not dispatch events while hidden + 3 make everything work
fine. because this is logically equavalent to !specialCase && specialCase
<abarth> I think there must be some misunderstanding
<abarth> consider a sequence of observable events A, B, C, and D
<abarth> why can we not simply delay the observable events while the page is
visible but have the events themselves be in all other respects the same?
<abarth> s/visible/invisible/
<kbalazs> abarth: I think this is what I'm doing, but still you can check the
time difference and the visibility state from js and "guess" that the
implementation special cases for this scenario
<abarth> kbalazs: that's not what your CL does.  specifically,
navigator.getGamepads() returns a magical different object during the batch
update
<abarth> kbalazs: that is never seen otherwise
<kbalazs> abarth: agree, that is wrong and needs to be fixed
<abarth> I can keep a reference to the old one and use === to see that the new
object is magical
<abarth> that's not the only problem
<abarth> I'd recommend changing the design so that this code path isn't a
special case
<abarth> otherwise, it's always going to be buggy and wrong
<kbalazs> abarth: understood, still think this is impossible. we are not
dispatching events while hidden, so visibility is in fact already a special
case, and you ask me to redisign this stuff as if it would not be a special
case. but it is, that's a source of the problem
<abarth> kbalazs: well, I'm not sure what more to say.  we have many other
systems that do similar things and don't have this problem
<abarth> kbalazs: see the clients of AsyncMethodRunner, for example

[kbalazs, 2014-06-26 21:50:55]

I am still having trouble understanding how can I reach the goal of the CL
without handling the visibility change as a special case. We don't dispatch
events in hidden state, so imho it's already a special case.

Maybe unifying the mechanisms of dispatching these events would solve the
problem about leaking information of the implementation? Like avoiding
dispatching multiple events without returning to the event loop. This is indeed
not something we does anywhere else.

With that in mind it would be better to handle that in chromium, but for that to
work in every cases we need to teach chromium that one renderer can have
multiple page interested in gamepad, so GamepadDispatcher should be per page and
not a singleton. Chromium can store a snapshot for every client and send events
when the client becomes visible - which we already tell to it - based on that
snapshot.

In any case we should be aware that it's getting to be a lot of craft for a
"potential" bug, maybe we should just wait until someone actually files a bug
for our behavior and then reconsidering how to fix it.

[abarth-chromium, 2014-06-27 03:07:50]

The way I'd probably approach this problem is by changing the interface between
Blink and Chromium.  Today, Blink expects to receive each connect and disconnect
event separately from Chromium.  Instead, if the interface with Chromium worked
in bulk, then NavigatorGamepad::didUpdateData() would know how to deal with
multiple game pads connecting and disconnecting.  When we changed visibility
state, that would just be another instance of a bulk change in gamepad data.

[kbalazs, 2014-06-27 18:51:42]

On 2014/06/27 03:07:50, abarth wrote:
> The way I'd probably approach this problem is by changing the interface
between
> Blink and Chromium.  Today, Blink expects to receive each connect and
disconnect
> event separately from Chromium.  Instead, if the interface with Chromium
worked
> in bulk, then NavigatorGamepad::didUpdateData() would know how to deal with
> multiple game pads connecting and disconnecting.  When we changed visibility
> state, that would just be another instance of a bulk change in gamepad data.

Technically it won't be a bulk, i.e. in blink we will process the messages
one-by-one. Also the snapshot thing (which is hacky) is not needed, jut a queue
of pending events can be stored in chromium. I think on-by-one dispatch is
desired because than we don't need to dispatch multiple events at the same
place, so there is no need for the hacks I did to deal with the detached during
dispatch state.

[kbalazs, 2014-07-07 21:03:36]

On 2014/06/27 18:51:42, kbalazs wrote:
> On 2014/06/27 03:07:50, abarth wrote:
> > The way I'd probably approach this problem is by changing the interface
> between
> > Blink and Chromium.  Today, Blink expects to receive each connect and
> disconnect
> > event separately from Chromium.  Instead, if the interface with Chromium
> worked
> > in bulk, then NavigatorGamepad::didUpdateData() would know how to deal with
> > multiple game pads connecting and disconnecting.  When we changed visibility
> > state, that would just be another instance of a bulk change in gamepad data.
> 
> Technically it won't be a bulk, i.e. in blink we will process the messages
> one-by-one. Also the snapshot thing (which is hacky) is not needed, jut a
queue
> of pending events can be stored in chromium. I think on-by-one dispatch is
> desired because than we don't need to dispatch multiple events at the same
> place, so there is no need for the hacks I did to deal with the detached
during
> dispatch state.

Abandoning this idea, it would need a whole bunch of refactor at chrome side
like creating a message filter for every frame.

I think I can make it better in Blink by using a queue and AsyncMethodRunner for
the events.

[kbalazs, 2014-07-07 21:13:31]

PTAL

[abarth-chromium, 2014-07-07 22:11:04]

This CL looks much better than the previous CL.  One comment below about memory
safety..

https://codereview.chromium.org/346273008/diff/20001/Source/modules/gamepad/N...
File Source/modules/gamepad/NavigatorGamepad.cpp (right):

https://codereview.chromium.org/346273008/diff/20001/Source/modules/gamepad/N...
Source/modules/gamepad/NavigatorGamepad.cpp:211: }
I'd probably move one of these to be after the call into the dispatcher for
consistency.

https://codereview.chromium.org/346273008/diff/20001/Source/modules/gamepad/N...
File Source/modules/gamepad/NavigatorGamepad.h (right):

https://codereview.chromium.org/346273008/diff/20001/Source/modules/gamepad/N...
Source/modules/gamepad/NavigatorGamepad.h:95: Deque<Gamepad*> m_pendingEvents;
Don't we need to trace these Gamepad* so that they don't get GCed?

[kbalazs, 2014-07-08 17:23:16]

On 2014/07/07 22:11:04, abarth wrote:
> This CL looks much better than the previous CL.  One comment below about
memory
> safety..
> 
>
https://codereview.chromium.org/346273008/diff/20001/Source/modules/gamepad/N...
> File Source/modules/gamepad/NavigatorGamepad.cpp (right):
> 
>
https://codereview.chromium.org/346273008/diff/20001/Source/modules/gamepad/N...
> Source/modules/gamepad/NavigatorGamepad.cpp:211: }
> I'd probably move one of these to be after the call into the dispatcher for
> consistency.
> 
>
https://codereview.chromium.org/346273008/diff/20001/Source/modules/gamepad/N...
> File Source/modules/gamepad/NavigatorGamepad.h (right):
> 
>
https://codereview.chromium.org/346273008/diff/20001/Source/modules/gamepad/N...
> Source/modules/gamepad/NavigatorGamepad.h:95: Deque<Gamepad*> m_pendingEvents;
> Don't we need to trace these Gamepad* so that they don't get GCed?

Yes, definitely. I'm still trying to figure out how should I do this.

[kbalazs, 2014-07-08 21:01:20]

https://codereview.chromium.org/346273008/diff/40001/Source/platform/AsyncMet...
File Source/platform/AsyncMethodRunner.h (right):

https://codereview.chromium.org/346273008/diff/40001/Source/platform/AsyncMet...
Source/platform/AsyncMethodRunner.h:78: return;
FYI I have added this to avoid special casing the first pageBecomeVisible that
comes when the view is set up initially.

[tkent, 2014-07-08 23:17:53]

https://codereview.chromium.org/346273008/diff/40001/Source/modules/gamepad/N...
File Source/modules/gamepad/NavigatorGamepad.h (right):

https://codereview.chromium.org/346273008/diff/40001/Source/modules/gamepad/N...
Source/modules/gamepad/NavigatorGamepad.h:93: WillBeHeapDeque<Member<Gamepad> >
m_pendingEvents;
NavigatorGamepad is not garbage-collected on trunk, and Gamepad is
garbage-collected on trunk.  So, This should be
    PersistentHeapDeque<Member<Gamepad> >
on trunk.

Also, NavigatorGamepad will be garbage-collected with ENABLE(OILPAN). So, This
should be
    HeapDeque<Member<Gamepad> >
with ENABLE(OILPAN).

Conclusion:
Add
#define PersistentHeapDequeWillBeHeapDeque WebCore::HeapDeque
#define PersistentHeapDequeWillBeHeapDeque WebCOre::PersistentHeapDeque
to platform/heap/Handle.h like PersistentHeapHashSetWillBeHeapHashSet, and use
PersistentHeapDequeWillBeHeapDeque<Member<Gamepad> > in NavigatorGamepad.

[abarth-chromium, 2014-07-10 15:51:15]

LGTM

I'm slightly skeptical about the changes to async method runner, but maybe it
ok.

[kbalazs, 2014-07-10 15:55:36]

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

[commit-bot: I haz the power, 2014-07-10 15:55:50]

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

[haraken, 2014-07-10 15:59:22]

LGTM from the oilpan perspective.

[abarth-chromium, 2014-07-10 16:02:40]

The CQ bit was unchecked by abarth@chromium.org

[abarth-chromium, 2014-07-10 16:03:11]

On second thought, can you add a counter to AsyncMethodRunner instead?  That way
we'll resume only if we've gotten enough resumes to balance the number of
suspend calls?

[kbalazs, 2014-07-10 16:07:01]

On 2014/07/10 16:03:11, abarth wrote:
> On second thought, can you add a counter to AsyncMethodRunner instead?  That
way
> we'll resume only if we've gotten enough resumes to balance the number of
> suspend calls?

I don't think we really need this extra logic. I just needed the change in
AsyncMethodRunner because at the very beginning we have a visibilityChange with
visible=true and in this case the resume should be avoided or be a noop.

[kbalazs, 2014-07-10 16:46:46]

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

[commit-bot: I haz the power, 2014-07-10 16:47:09]

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

[commit-bot: I haz the power, 2014-07-10 18:59:12]

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

[commit-bot: I haz the power, 2014-07-10 20:03:48]

Change committed as 177874

[falken, 2014-07-11 01:59:29]

On 2014/07/10 20:03:48, I haz the power (commit-bot) wrote:
> Change committed as 177874

The added page-visibility.html layout test looks flaky (but mostly just on
Oilpan?):
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

It fails with:
FAIL More gamepad events should have been received

[kbalazs, 2014-07-11 14:47:04]

On 2014/07/11 01:59:29, falken wrote:
> On 2014/07/10 20:03:48, I haz the power (commit-bot) wrote:
> > Change committed as 177874
> 
> The added page-visibility.html layout test looks flaky (but mostly just on
> Oilpan?):
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...
> 
> It fails with:
> FAIL More gamepad events should have been received

Thanks. Do you think I should mark it as [ Failure Pass ] for the time being?
It's not clear to me what is the procedure when a test is flaky but never
actually red.