Deduplicate DeviceEvent* classes

Deduplicate DeviceEvent* classes

r175792 did a good job on refactoring this for the most parts, but it didn't do
so well by leaving behind the old classes only for NavigatorGamepad. This CL
trasitions NavigatorGamepad to the new base classes and and removes the code
duplication.
Also removeEventListener fixed not to assume that it was the last listener.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176482

[timvolodine, 2014-06-12 23:14:24]

On 2014/06/12 21:58:04, kbalazs wrote:

I think the gamepad classes should be based on DeviceEventControllerBase and
DeviceEventDispatcherBase similar to how BatteryManager is doing.
DeviceSingleWindowEventController was specifically designed for single-event
APIs firing on the window object. Ideally the refactoring should not touch
device_orientation/device_light module..

[kbalazs, 2014-06-12 23:55:49]

On 2014/06/12 23:14:24, timvolodine wrote:
> On 2014/06/12 21:58:04, kbalazs wrote:
> 
> I think the gamepad classes should be based on DeviceEventControllerBase and
> DeviceEventDispatcherBase similar to how BatteryManager is doing.
> DeviceSingleWindowEventController was specifically designed for single-event
> APIs firing on the window object. Ideally the refactoring should not touch
> device_orientation/device_light module..

This is also an option and yes, gamepad has a bit different patterns than the
other subclasses. It's events are firing on the window though so the event
listener part would be duplicated with all the active dom object and page
visibility checks. The problem with that approach is that when a new feature is
coming than it's quite likely that some of that logic will be missed and we end
up dispatching events for a hidden page (wasting battery) or doing something
incorrect with the listeners (badness). So I though
DeviceSingleWindowEventController can support some generalization, but I agree
the result is not so clear.

I will consider redoing it but I'm still very open to more comments. :)

[kbalazs, 2014-06-14 01:09:42]

On 2014/06/12 23:55:49, kbalazs wrote:
> On 2014/06/12 23:14:24, timvolodine wrote:
> > On 2014/06/12 21:58:04, kbalazs wrote:
> > 
> > I think the gamepad classes should be based on DeviceEventControllerBase and
> > DeviceEventDispatcherBase similar to how BatteryManager is doing.
> > DeviceSingleWindowEventController was specifically designed for single-event
> > APIs firing on the window object. Ideally the refactoring should not touch
> > device_orientation/device_light module..
> 
> This is also an option and yes, gamepad has a bit different patterns than the
> other subclasses. It's events are firing on the window though so the event
> listener part would be duplicated with all the active dom object and page
> visibility checks. The problem with that approach is that when a new feature
is
> coming than it's quite likely that some of that logic will be missed and we
end
> up dispatching events for a hidden page (wasting battery) or doing something
> incorrect with the listeners (badness). So I though
> DeviceSingleWindowEventController can support some generalization, but I agree
> the result is not so clear.
> 
> I will consider redoing it but I'm still very open to more comments. :)

Indeed this is much clearer. We can iterate further to remove the remaining code
dup.

[timvolodine, 2014-06-16 13:05:17]

thanks Balazs, looks much better now :).

https://codereview.chromium.org/336693004/diff/40001/LayoutTests/gamepad/mult...
File LayoutTests/gamepad/multiple-event-listeners.html (right):

https://codereview.chromium.org/336693004/diff/40001/LayoutTests/gamepad/mult...
LayoutTests/gamepad/multiple-event-listeners.html:18:
window.removeEventListener('gamepadconnected', listenerToRemove);
why is this needed? adding and removing an empty listener?

https://codereview.chromium.org/336693004/diff/40001/Source/modules/gamepad/G...
File Source/modules/gamepad/GamepadDispatcher.h (right):

https://codereview.chromium.org/336693004/diff/40001/Source/modules/gamepad/G...
Source/modules/gamepad/GamepadDispatcher.h:27: const blink::WebGamepad&
latestGamepadEventData(unsigned& index) const;
drop the index argument here?

https://codereview.chromium.org/336693004/diff/40001/Source/modules/gamepad/G...
Source/modules/gamepad/GamepadDispatcher.h:33: virtual void
didConnectGamepad(unsigned index, const blink::WebGamepad&) OVERRIDE;
nit: not really related to this patch but could you add a comment that the
methods below are inherited from WebGamepadListener?

https://codereview.chromium.org/336693004/diff/40001/Source/modules/gamepad/G...
Source/modules/gamepad/GamepadDispatcher.h:41: unsigned m_latestGamepadIndex;
is this needed?

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

https://codereview.chromium.org/336693004/diff/40001/Source/modules/gamepad/N...
Source/modules/gamepad/NavigatorGamepad.cpp:145: const blink::WebGamepad&
webGamepad = GamepadDispatcher::instance().latestGamepadEventData(index);
do you need to pass index here? maybe just return the lastestGamepad data.

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

https://codereview.chromium.org/336693004/diff/40001/Source/modules/gamepad/N...
Source/modules/gamepad/NavigatorGamepad.h:44: class Event;
is this necessary?

[kbalazs, 2014-06-16 17:01:47]

https://codereview.chromium.org/336693004/diff/40001/LayoutTests/gamepad/mult...
File LayoutTests/gamepad/multiple-event-listeners.html (right):

https://codereview.chromium.org/336693004/diff/40001/LayoutTests/gamepad/mult...
LayoutTests/gamepad/multiple-event-listeners.html:18:
window.removeEventListener('gamepadconnected', listenerToRemove);
On 2014/06/16 13:05:17, timvolodine wrote:
> why is this needed? adding and removing an empty listener?

This tests that we still honor the other listeners if one is removed. Look at
the change in NavigatorGamepad::didRemoveEventListener, before the patch we did
not check if there is other listeners before setting m_hasEventListener to
false.

https://codereview.chromium.org/336693004/diff/40001/Source/modules/gamepad/G...
File Source/modules/gamepad/GamepadDispatcher.h (right):

https://codereview.chromium.org/336693004/diff/40001/Source/modules/gamepad/G...
Source/modules/gamepad/GamepadDispatcher.h:33: virtual void
didConnectGamepad(unsigned index, const blink::WebGamepad&) OVERRIDE;
On 2014/06/16 13:05:17, timvolodine wrote:
> nit: not really related to this patch but could you add a comment that the
> methods below are inherited from WebGamepadListener?

Sure

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

https://codereview.chromium.org/336693004/diff/40001/Source/modules/gamepad/N...
Source/modules/gamepad/NavigatorGamepad.cpp:145: const blink::WebGamepad&
webGamepad = GamepadDispatcher::instance().latestGamepadEventData(index);
On 2014/06/16 13:05:17, timvolodine wrote:
> do you need to pass index here? maybe just return the lastestGamepad data.

Yes, the index is used below, it was an argument of the function :)

[timvolodine, 2014-06-16 17:13:36]

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

https://codereview.chromium.org/336693004/diff/40001/Source/modules/gamepad/N...
Source/modules/gamepad/NavigatorGamepad.cpp:145: const blink::WebGamepad&
webGamepad = GamepadDispatcher::instance().latestGamepadEventData(index);
On 2014/06/16 17:01:47, kbalazs wrote:
> On 2014/06/16 13:05:17, timvolodine wrote:
> > do you need to pass index here? maybe just return the lastestGamepad data.
> 
> Yes, the index is used below, it was an argument of the function :)

ah I see, so that looks confusing to me. maybe introduce another method
latestGamepadIndex? i think that'd be cleaner.

[kbalazs, 2014-06-16 18:08:07]

On 2014/06/16 17:13:36, timvolodine wrote:
>
https://codereview.chromium.org/336693004/diff/40001/Source/modules/gamepad/N...
> File Source/modules/gamepad/NavigatorGamepad.cpp (right):
> 
>
https://codereview.chromium.org/336693004/diff/40001/Source/modules/gamepad/N...
> Source/modules/gamepad/NavigatorGamepad.cpp:145: const blink::WebGamepad&
> webGamepad = GamepadDispatcher::instance().latestGamepadEventData(index);
> On 2014/06/16 17:01:47, kbalazs wrote:
> > On 2014/06/16 13:05:17, timvolodine wrote:
> > > do you need to pass index here? maybe just return the lastestGamepad data.
> > 
> > Yes, the index is used below, it was an argument of the function :)
> 
> ah I see, so that looks confusing to me. maybe introduce another method
> latestGamepadIndex? i think that'd be cleaner.

That's also confusing. "Latest gamepad" is a misleading term, it should be
latestConnectedOrDisconnectedGamepad but than having that and also
latestConnectedOrDisconnectedGamepadIndex feels too verbose.

How about a public "struct GamepadEvent" in GamepadDispatcher holding the pad
and the index together?

[timvolodine, 2014-06-16 18:25:25]

On 2014/06/16 18:08:07, kbalazs wrote:
> On 2014/06/16 17:13:36, timvolodine wrote:
> >
>
https://codereview.chromium.org/336693004/diff/40001/Source/modules/gamepad/N...
> > File Source/modules/gamepad/NavigatorGamepad.cpp (right):
> > 
> >
>
https://codereview.chromium.org/336693004/diff/40001/Source/modules/gamepad/N...
> > Source/modules/gamepad/NavigatorGamepad.cpp:145: const blink::WebGamepad&
> > webGamepad = GamepadDispatcher::instance().latestGamepadEventData(index);
> > On 2014/06/16 17:01:47, kbalazs wrote:
> > > On 2014/06/16 13:05:17, timvolodine wrote:
> > > > do you need to pass index here? maybe just return the lastestGamepad
data.
> > > 
> > > Yes, the index is used below, it was an argument of the function :)
> > 
> > ah I see, so that looks confusing to me. maybe introduce another method
> > latestGamepadIndex? i think that'd be cleaner.
> 
> That's also confusing. "Latest gamepad" is a misleading term, it should be
> latestConnectedOrDisconnectedGamepad but than having that and also
> latestConnectedOrDisconnectedGamepadIndex feels too verbose.
> 
> How about a public "struct GamepadEvent" in GamepadDispatcher holding the pad
> and the index together?

yeah but it's not really an event either.. latestGamepadConnectionChangedIndex?
or simply without the "latest": indexConnectionChangedGamepad?

[kbalazs, 2014-06-16 18:49:14]

On 2014/06/16 18:25:25, timvolodine wrote:
> On 2014/06/16 18:08:07, kbalazs wrote:
> > On 2014/06/16 17:13:36, timvolodine wrote:
> > >
> >
>
https://codereview.chromium.org/336693004/diff/40001/Source/modules/gamepad/N...
> > > File Source/modules/gamepad/NavigatorGamepad.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/336693004/diff/40001/Source/modules/gamepad/N...
> > > Source/modules/gamepad/NavigatorGamepad.cpp:145: const blink::WebGamepad&
> > > webGamepad = GamepadDispatcher::instance().latestGamepadEventData(index);
> > > On 2014/06/16 17:01:47, kbalazs wrote:
> > > > On 2014/06/16 13:05:17, timvolodine wrote:
> > > > > do you need to pass index here? maybe just return the lastestGamepad
> data.
> > > > 
> > > > Yes, the index is used below, it was an argument of the function :)
> > > 
> > > ah I see, so that looks confusing to me. maybe introduce another method
> > > latestGamepadIndex? i think that'd be cleaner.
> > 
> > That's also confusing. "Latest gamepad" is a misleading term, it should be
> > latestConnectedOrDisconnectedGamepad but than having that and also
> > latestConnectedOrDisconnectedGamepadIndex feels too verbose.
> > 
> > How about a public "struct GamepadEvent" in GamepadDispatcher holding the
pad
> > and the index together?
> 
> yeah but it's not really an event either..
latestGamepadConnectionChangedIndex?
> or simply without the "latest": indexConnectionChangedGamepad?

Made it latestConnectionChange and returning a struct now. I didn't want to omit
"latest" because it is important that this is only temporally valid.

[timvolodine, 2014-06-17 14:23:51]

lgtm for the changes related to DeviceEvent..Base classes, however I am not an
owner of this module. also, could you run the try-bots please?

https://codereview.chromium.org/336693004/diff/60001/Source/modules/device_li...
File Source/modules/device_light/DeviceLightDispatcher.cpp (right):

https://codereview.chromium.org/336693004/diff/60001/Source/modules/device_li...
Source/modules/device_light/DeviceLightDispatcher.cpp:8: #include
"core/frame/DeviceEventDispatcherBase.h"
think you can remove this line completely.

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

https://codereview.chromium.org/336693004/diff/60001/Source/modules/gamepad/N...
Source/modules/gamepad/NavigatorGamepad.cpp:144:
GamepadDispatcher::ConnectionChange change =
GamepadDispatcher::instance().latestConnectionChange();
const ..& ?

[scottmg, 2014-06-17 17:53:58]

This lgtm, but I should no longer be in owners here as this code is no longer
recognizable to me. Please get a review from someone one level higher.

https://codereview.chromium.org/336693004/diff/60001/LayoutTests/gamepad/mult...
File LayoutTests/gamepad/multiple-event-listeners.html (right):

https://codereview.chromium.org/336693004/diff/60001/LayoutTests/gamepad/mult...
LayoutTests/gamepad/multiple-event-listeners.html:6: description("Test that when
there are multiple gamepad event listeners and one is getting removed the others
still recieves events.");
nit; "receives" and "received" below

[kbalazs, 2014-06-18 22:18:38]

Adam, could you look at this? Thanks.

[abarth-chromium, 2014-06-19 02:16:06]

lgtm

[commit-bot: I haz the power, 2014-06-19 02:16:39]

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

[commit-bot: I haz the power, 2014-06-19 03:27:40]

Change committed as 176482