Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(162)

Issue 336693004: Deduplicate DeviceEvent* classes (Closed)

Created:
6 years, 6 months ago by kbalazs
Modified:
6 years, 6 months ago
CC:
blink-reviews, Inactive, mvanouwerkerk+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : derive from *Base #

Patch Set 3 : derive from *Base #

Total comments: 10

Patch Set 4 : latestConnectionChange #

Total comments: 3

Patch Set 5 : fix nits #

Patch Set 6 : oops - fix grammar in test expectation as well #

Unified diffs Side-by-side diffs Delta from patch set Stats (+84 lines, -410 lines) Patch
A LayoutTests/gamepad/multiple-event-listeners.html View 1 2 3 4 1 chunk +31 lines, -0 lines 0 comments Download
A LayoutTests/gamepad/multiple-event-listeners-expected.txt View 1 2 3 4 5 1 chunk +10 lines, -0 lines 0 comments Download
M Source/core/core.gypi View 1 2 3 1 chunk +0 lines, -4 lines 0 comments Download
D Source/core/frame/DeviceSensorEventController.h View 2 1 chunk +0 lines, -71 lines 0 comments Download
D Source/core/frame/DeviceSensorEventController.cpp View 2 1 chunk +0 lines, -113 lines 0 comments Download
D Source/core/frame/DeviceSensorEventDispatcher.h View 2 1 chunk +0 lines, -58 lines 0 comments Download
D Source/core/frame/DeviceSensorEventDispatcher.cpp View 2 1 chunk +0 lines, -95 lines 0 comments Download
M Source/modules/device_light/DeviceLightDispatcher.cpp View 2 3 4 1 chunk +0 lines, -1 line 0 comments Download
M Source/modules/gamepad/GamepadDispatcher.h View 1 2 3 2 chunks +17 lines, -6 lines 0 comments Download
M Source/modules/gamepad/GamepadDispatcher.cpp View 1 2 3 2 chunks +5 lines, -22 lines 0 comments Download
M Source/modules/gamepad/NavigatorGamepad.h View 1 2 3 3 chunks +5 lines, -6 lines 0 comments Download
M Source/modules/gamepad/NavigatorGamepad.cpp View 1 2 3 4 5 chunks +16 lines, -34 lines 0 comments Download

Messages

Total messages: 16 (0 generated)
kbalazs
6 years, 6 months ago (2014-06-12 21:58:04 UTC) #1
timvolodine
On 2014/06/12 21:58:04, kbalazs wrote: I think the gamepad classes should be based on DeviceEventControllerBase ...
6 years, 6 months ago (2014-06-12 23:14:24 UTC) #2
kbalazs
On 2014/06/12 23:14:24, timvolodine wrote: > On 2014/06/12 21:58:04, kbalazs wrote: > > I think ...
6 years, 6 months ago (2014-06-12 23:55:49 UTC) #3
kbalazs
On 2014/06/12 23:55:49, kbalazs wrote: > On 2014/06/12 23:14:24, timvolodine wrote: > > On 2014/06/12 ...
6 years, 6 months ago (2014-06-14 01:09:42 UTC) #4
timvolodine
thanks Balazs, looks much better now :). https://codereview.chromium.org/336693004/diff/40001/LayoutTests/gamepad/multiple-event-listeners.html File LayoutTests/gamepad/multiple-event-listeners.html (right): https://codereview.chromium.org/336693004/diff/40001/LayoutTests/gamepad/multiple-event-listeners.html#newcode18 LayoutTests/gamepad/multiple-event-listeners.html:18: window.removeEventListener('gamepadconnected', listenerToRemove); ...
6 years, 6 months ago (2014-06-16 13:05:17 UTC) #5
kbalazs
https://codereview.chromium.org/336693004/diff/40001/LayoutTests/gamepad/multiple-event-listeners.html File LayoutTests/gamepad/multiple-event-listeners.html (right): https://codereview.chromium.org/336693004/diff/40001/LayoutTests/gamepad/multiple-event-listeners.html#newcode18 LayoutTests/gamepad/multiple-event-listeners.html:18: window.removeEventListener('gamepadconnected', listenerToRemove); On 2014/06/16 13:05:17, timvolodine wrote: > why ...
6 years, 6 months ago (2014-06-16 17:01:47 UTC) #6
timvolodine
https://codereview.chromium.org/336693004/diff/40001/Source/modules/gamepad/NavigatorGamepad.cpp File Source/modules/gamepad/NavigatorGamepad.cpp (right): https://codereview.chromium.org/336693004/diff/40001/Source/modules/gamepad/NavigatorGamepad.cpp#newcode145 Source/modules/gamepad/NavigatorGamepad.cpp:145: const blink::WebGamepad& webGamepad = GamepadDispatcher::instance().latestGamepadEventData(index); On 2014/06/16 17:01:47, kbalazs ...
6 years, 6 months ago (2014-06-16 17:13:36 UTC) #7
kbalazs
On 2014/06/16 17:13:36, timvolodine wrote: > https://codereview.chromium.org/336693004/diff/40001/Source/modules/gamepad/NavigatorGamepad.cpp > File Source/modules/gamepad/NavigatorGamepad.cpp (right): > > https://codereview.chromium.org/336693004/diff/40001/Source/modules/gamepad/NavigatorGamepad.cpp#newcode145 > ...
6 years, 6 months ago (2014-06-16 18:08:07 UTC) #8
timvolodine
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/NavigatorGamepad.cpp ...
6 years, 6 months ago (2014-06-16 18:25:25 UTC) #9
kbalazs
On 2014/06/16 18:25:25, timvolodine wrote: > On 2014/06/16 18:08:07, kbalazs wrote: > > On 2014/06/16 ...
6 years, 6 months ago (2014-06-16 18:49:14 UTC) #10
timvolodine
lgtm for the changes related to DeviceEvent..Base classes, however I am not an owner of ...
6 years, 6 months ago (2014-06-17 14:23:51 UTC) #11
scottmg
This lgtm, but I should no longer be in owners here as this code is ...
6 years, 6 months ago (2014-06-17 17:53:58 UTC) #12
kbalazs
Adam, could you look at this? Thanks.
6 years, 6 months ago (2014-06-18 22:18:38 UTC) #13
abarth-chromium
lgtm
6 years, 6 months ago (2014-06-19 02:16:06 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/336693004/100001
6 years, 6 months ago (2014-06-19 02:16:39 UTC) #15
commit-bot: I haz the power
6 years, 6 months ago (2014-06-19 03:27:40 UTC) #16
Message was sent while issue was closed.
Change committed as 176482

Powered by Google App Engine
This is Rietveld 408576698