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

Issue 619383002: Move PlatformEventDispatcher implementations to the Oilpan heap. (Closed)

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

Description

Move PlatformEventDispatcher implementations to the Oilpan heap. Make PlatformEventDispatcher a GC mixin, maintaining weak references to its registered controllers. R=haraken BUG=340522 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183211

Patch Set 1 #

Total comments: 16

Patch Set 2 : Fix the non-Oilpan type of PlatformEventDispatcher::m_controllers #

Patch Set 3 : Missed Persistent<> wrapping of DeviceOrientationDispatcher singleton #

Patch Set 4 : Only call stopUpdating() from controller dtors when without Oilpan #

Total comments: 5

Patch Set 5 : Fix non-Oilpan allocation of DeviceLightDispatcher singleton #

Unified diffs Side-by-side diffs Delta from patch set Stats (+129 lines, -58 lines) Patch
M Source/core/frame/PlatformEventDispatcher.h View 1 2 3 4 2 chunks +8 lines, -3 lines 0 comments Download
M Source/core/frame/PlatformEventDispatcher.cpp View 1 2 3 4 2 chunks +22 lines, -4 lines 0 comments Download
M Source/modules/battery/BatteryDispatcher.h View 2 chunks +8 lines, -5 lines 0 comments Download
M Source/modules/battery/BatteryDispatcher.cpp View 2 chunks +8 lines, -2 lines 0 comments Download
M Source/modules/battery/BatteryManager.h View 1 1 chunk +1 line, -1 line 0 comments Download
M Source/modules/battery/BatteryManager.cpp View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M Source/modules/device_light/DeviceLightController.cpp View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M Source/modules/device_light/DeviceLightDispatcher.h View 2 chunks +9 lines, -5 lines 0 comments Download
M Source/modules/device_light/DeviceLightDispatcher.cpp View 1 2 3 4 2 chunks +7 lines, -2 lines 0 comments Download
M Source/modules/device_orientation/DeviceMotionController.cpp View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M Source/modules/device_orientation/DeviceMotionDispatcher.h View 1 chunk +9 lines, -6 lines 0 comments Download
M Source/modules/device_orientation/DeviceMotionDispatcher.cpp View 2 chunks +8 lines, -2 lines 0 comments Download
M Source/modules/device_orientation/DeviceOrientationController.cpp View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M Source/modules/device_orientation/DeviceOrientationDispatcher.h View 1 chunk +9 lines, -6 lines 0 comments Download
M Source/modules/device_orientation/DeviceOrientationDispatcher.cpp View 1 2 2 chunks +8 lines, -2 lines 0 comments Download
M Source/modules/gamepad/GamepadDispatcher.h View 2 chunks +9 lines, -6 lines 0 comments Download
M Source/modules/gamepad/GamepadDispatcher.cpp View 2 chunks +7 lines, -2 lines 0 comments Download
M Source/modules/gamepad/NavigatorGamepad.cpp View 1 2 3 1 chunk +0 lines, -3 lines 0 comments Download
M Source/modules/screen_orientation/ScreenOrientationDispatcher.h View 1 2 chunks +4 lines, -6 lines 0 comments Download
M Source/modules/screen_orientation/ScreenOrientationDispatcher.cpp View 1 chunk +4 lines, -3 lines 0 comments Download

Messages

Total messages: 18 (3 generated)
sof
Please take a look. PlatformEventDispatcher::m_controllers is kept as a vector (a HeapVector of bare pointers ...
6 years, 2 months ago (2014-10-02 21:41:37 UTC) #2
haraken
https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEventDispatcher.cpp File Source/core/frame/PlatformEventDispatcher.cpp (right): https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEventDispatcher.cpp#newcode28 Source/core/frame/PlatformEventDispatcher.cpp:28: void PlatformEventDispatcher::removeController(PlatformEventController* controller) We need to make sure that ...
6 years, 2 months ago (2014-10-03 01:55:33 UTC) #4
sof
https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEventDispatcher.cpp File Source/core/frame/PlatformEventDispatcher.cpp (right): https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEventDispatcher.cpp#newcode85 Source/core/frame/PlatformEventDispatcher.cpp:85: visitor->trace(m_controllers); On 2014/10/03 01:55:33, haraken wrote: > > Why ...
6 years, 2 months ago (2014-10-03 05:05:29 UTC) #5
haraken
https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEventDispatcher.cpp File Source/core/frame/PlatformEventDispatcher.cpp (right): https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEventDispatcher.cpp#newcode85 Source/core/frame/PlatformEventDispatcher.cpp:85: visitor->trace(m_controllers); On 2014/10/03 05:05:29, sof wrote: > On 2014/10/03 ...
6 years, 2 months ago (2014-10-03 05:36:02 UTC) #6
sof
https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEventDispatcher.cpp File Source/core/frame/PlatformEventDispatcher.cpp (right): https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEventDispatcher.cpp#newcode28 Source/core/frame/PlatformEventDispatcher.cpp:28: void PlatformEventDispatcher::removeController(PlatformEventController* controller) On 2014/10/03 01:55:33, haraken wrote: > ...
6 years, 2 months ago (2014-10-03 06:34:04 UTC) #7
haraken
https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEventDispatcher.cpp File Source/core/frame/PlatformEventDispatcher.cpp (right): https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEventDispatcher.cpp#newcode28 Source/core/frame/PlatformEventDispatcher.cpp:28: void PlatformEventDispatcher::removeController(PlatformEventController* controller) On 2014/10/03 06:34:04, sof wrote: > ...
6 years, 2 months ago (2014-10-03 10:10:56 UTC) #8
sof
https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEventDispatcher.cpp File Source/core/frame/PlatformEventDispatcher.cpp (right): https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEventDispatcher.cpp#newcode28 Source/core/frame/PlatformEventDispatcher.cpp:28: void PlatformEventDispatcher::removeController(PlatformEventController* controller) On 2014/10/03 10:10:56, haraken wrote: > ...
6 years, 2 months ago (2014-10-03 10:27:12 UTC) #9
sof
https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEventDispatcher.cpp File Source/core/frame/PlatformEventDispatcher.cpp (right): https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEventDispatcher.cpp#newcode28 Source/core/frame/PlatformEventDispatcher.cpp:28: void PlatformEventDispatcher::removeController(PlatformEventController* controller) On 2014/10/03 10:27:11, sof wrote: > ...
6 years, 2 months ago (2014-10-03 12:21:17 UTC) #10
haraken
LGTM https://codereview.chromium.org/619383002/diff/60001/Source/core/frame/PlatformEventDispatcher.h File Source/core/frame/PlatformEventDispatcher.h (right): https://codereview.chromium.org/619383002/diff/60001/Source/core/frame/PlatformEventDispatcher.h#newcode32 Source/core/frame/PlatformEventDispatcher.h:32: void clearWeakMembers(Visitor*); You can add #if ENABLE(OILPAN). https://codereview.chromium.org/619383002/diff/60001/Source/modules/device_light/DeviceLightDispatcher.cpp ...
6 years, 2 months ago (2014-10-03 13:18:17 UTC) #11
sof
Thanks for the review. https://codereview.chromium.org/619383002/diff/60001/Source/core/frame/PlatformEventDispatcher.h File Source/core/frame/PlatformEventDispatcher.h (right): https://codereview.chromium.org/619383002/diff/60001/Source/core/frame/PlatformEventDispatcher.h#newcode32 Source/core/frame/PlatformEventDispatcher.h:32: void clearWeakMembers(Visitor*); On 2014/10/03 13:18:17, ...
6 years, 2 months ago (2014-10-03 13:33:58 UTC) #12
haraken
https://codereview.chromium.org/619383002/diff/60001/Source/modules/device_light/DeviceLightDispatcher.cpp File Source/modules/device_light/DeviceLightDispatcher.cpp (right): https://codereview.chromium.org/619383002/diff/60001/Source/modules/device_light/DeviceLightDispatcher.cpp#newcode15 Source/modules/device_light/DeviceLightDispatcher.cpp:15: DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<DeviceLightDispatcher>, deviceLightDispatcher, (adoptPtrWillBeNoop(new DeviceLightDispatcher()))); On 2014/10/03 13:33:58, sof wrote: ...
6 years, 2 months ago (2014-10-03 14:15:07 UTC) #13
sof
On 2014/10/03 14:15:07, haraken wrote: > https://codereview.chromium.org/619383002/diff/60001/Source/modules/device_light/DeviceLightDispatcher.cpp > File Source/modules/device_light/DeviceLightDispatcher.cpp (right): > > https://codereview.chromium.org/619383002/diff/60001/Source/modules/device_light/DeviceLightDispatcher.cpp#newcode15 > ...
6 years, 2 months ago (2014-10-03 14:31:55 UTC) #14
haraken
On 2014/10/03 14:31:55, sof wrote: > On 2014/10/03 14:15:07, haraken wrote: > > > https://codereview.chromium.org/619383002/diff/60001/Source/modules/device_light/DeviceLightDispatcher.cpp ...
6 years, 2 months ago (2014-10-03 14:36:33 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/619383002/80001
6 years, 2 months ago (2014-10-03 18:38:14 UTC) #17
commit-bot: I haz the power
6 years, 2 months ago (2014-10-03 20:09:56 UTC) #18
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as 183211

Powered by Google App Engine
This is Rietveld 408576698