Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1828203003:
Flash players weren't receiving mouse wheel events. (Closed)
|
Created:
4 years, 9 months ago by dtapuska Modified:
4 years, 8 months ago Reviewers:
aelias_OOO_until_Jul13 CC:
blink-reviews, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFlash players weren't receiving mouse wheel events.
With the move to gesture based scrolling flash players stopped receiving
mouse wheel events. Flash players were receiving wheel events by fluke
because they are no fast scrollable regions. The events wouldn't get
handled in the compositor and would get sent to main thread. But since
with wheel gestures we don't ever send the events to main thread if
there is no listener this stopped working.
Correctly articulate to the EventHandlerRegistry when plugin objects
wish to receive wheel events.
BUG=597262
Committed: https://crrev.com/a6293982c7f5c0ad0e2ae56238fd6062ca866fcf
Cr-Commit-Position: refs/heads/master@{#383663}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Add unit test #
Messages
Total messages: 15 (6 generated)
Description was changed from ========== Flash players weren't receiving mouse wheel events. With the move to gesture based scrolling flash players stopped receiving mouse wheel events. Flash players were receiving wheel events by fluke because they are no fast scrollable regions the events wouldn't get handled in the compositor and would get sent to main thread. But since with wheel gestures we don't ever send the events to main thread if there is no listener this stopped working. Correctly articulate to the EventHandlerRegistry when plugin objects wish to receive wheel events. BUG=597262 ========== to ========== Flash players weren't receiving mouse wheel events. With the move to gesture based scrolling flash players stopped receiving mouse wheel events. Flash players were receiving wheel events by fluke because they are no fast scrollable regions the events wouldn't get handled in the compositor and would get sent to main thread. But since with wheel gestures we don't ever send the events to main thread if there is no listener this stopped working. Correctly articulate to the EventHandlerRegistry when plugin objects wish to receive wheel events. BUG=597262 ==========
dtapuska@chromium.org changed reviewers: + aelias@chromium.org
Description was changed from ========== Flash players weren't receiving mouse wheel events. With the move to gesture based scrolling flash players stopped receiving mouse wheel events. Flash players were receiving wheel events by fluke because they are no fast scrollable regions the events wouldn't get handled in the compositor and would get sent to main thread. But since with wheel gestures we don't ever send the events to main thread if there is no listener this stopped working. Correctly articulate to the EventHandlerRegistry when plugin objects wish to receive wheel events. BUG=597262 ========== to ========== Flash players weren't receiving mouse wheel events. With the move to gesture based scrolling flash players stopped receiving mouse wheel events. Flash players were receiving wheel events by fluke because they are no fast scrollable regions. The events wouldn't get handled in the compositor and would get sent to main thread. But since with wheel gestures we don't ever send the events to main thread if there is no listener this stopped working. Correctly articulate to the EventHandlerRegistry when plugin objects wish to receive wheel events. BUG=597262 ==========
Can you write a unit test for this? Plugin input keeps having serious regressions because the codepath is so weird and there's little test coverage; we should at least add tests as we run into problems. https://codereview.chromium.org/1828203003/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right): https://codereview.chromium.org/1828203003/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:733: frameHost->eventHandlerRegistry().didRemoveEventHandler(*m_element, EventHandlerRegistry::WheelEventBlocking); Can we call "setWantsWheelEvents(false);" to avoid the code duplication?
https://codereview.chromium.org/1828203003/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right): https://codereview.chromium.org/1828203003/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:733: frameHost->eventHandlerRegistry().didRemoveEventHandler(*m_element, EventHandlerRegistry::WheelEventBlocking); On 2016/03/24 19:15:05, aelias wrote: > Can we call "setWantsWheelEvents(false);" to avoid the code duplication? I wanted to avoid the updateGeometry call inside the setWantsWheelEvents method. But I could break apart that method into a helper function I could call from dispose. Also for some reason the build bots aren't liking this change for the tests. Will have to look at it Tuesday.
https://codereview.chromium.org/1828203003/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right): https://codereview.chromium.org/1828203003/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:733: frameHost->eventHandlerRegistry().didRemoveEventHandler(*m_element, EventHandlerRegistry::WheelEventBlocking); On 2016/03/24 at 21:14:29, dtapuska wrote: > On 2016/03/24 19:15:05, aelias wrote: > > Can we call "setWantsWheelEvents(false);" to avoid the code duplication? > > I wanted to avoid the updateGeometry call inside the setWantsWheelEvents method. Why did you want that, is there a reason to think calling updateGeometry() will cause a problem? If it's just for performance, sounds like premature optimization, since I'd be surprised if WebPluginContainerImpl::dispose() was on any kind of critical path.
On 2016/03/25 00:05:40, aelias wrote: > https://codereview.chromium.org/1828203003/diff/1/third_party/WebKit/Source/w... > File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right): > > https://codereview.chromium.org/1828203003/diff/1/third_party/WebKit/Source/w... > third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:733: > frameHost->eventHandlerRegistry().didRemoveEventHandler(*m_element, > EventHandlerRegistry::WheelEventBlocking); > On 2016/03/24 at 21:14:29, dtapuska wrote: > > On 2016/03/24 19:15:05, aelias wrote: > > > Can we call "setWantsWheelEvents(false);" to avoid the code duplication? > > > > I wanted to avoid the updateGeometry call inside the setWantsWheelEvents > method. > > Why did you want that, is there a reason to think calling updateGeometry() will > cause a problem? If it's just for performance, sounds like premature > optimization, since I'd be surprised if WebPluginContainerImpl::dispose() was on > any kind of critical path. Done; and I added a test.
lgtm
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1828203003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1828203003/20001
Message was sent while issue was closed.
Description was changed from ========== Flash players weren't receiving mouse wheel events. With the move to gesture based scrolling flash players stopped receiving mouse wheel events. Flash players were receiving wheel events by fluke because they are no fast scrollable regions. The events wouldn't get handled in the compositor and would get sent to main thread. But since with wheel gestures we don't ever send the events to main thread if there is no listener this stopped working. Correctly articulate to the EventHandlerRegistry when plugin objects wish to receive wheel events. BUG=597262 ========== to ========== Flash players weren't receiving mouse wheel events. With the move to gesture based scrolling flash players stopped receiving mouse wheel events. Flash players were receiving wheel events by fluke because they are no fast scrollable regions. The events wouldn't get handled in the compositor and would get sent to main thread. But since with wheel gestures we don't ever send the events to main thread if there is no listener this stopped working. Correctly articulate to the EventHandlerRegistry when plugin objects wish to receive wheel events. BUG=597262 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Flash players weren't receiving mouse wheel events. With the move to gesture based scrolling flash players stopped receiving mouse wheel events. Flash players were receiving wheel events by fluke because they are no fast scrollable regions. The events wouldn't get handled in the compositor and would get sent to main thread. But since with wheel gestures we don't ever send the events to main thread if there is no listener this stopped working. Correctly articulate to the EventHandlerRegistry when plugin objects wish to receive wheel events. BUG=597262 ========== to ========== Flash players weren't receiving mouse wheel events. With the move to gesture based scrolling flash players stopped receiving mouse wheel events. Flash players were receiving wheel events by fluke because they are no fast scrollable regions. The events wouldn't get handled in the compositor and would get sent to main thread. But since with wheel gestures we don't ever send the events to main thread if there is no listener this stopped working. Correctly articulate to the EventHandlerRegistry when plugin objects wish to receive wheel events. BUG=597262 Committed: https://crrev.com/a6293982c7f5c0ad0e2ae56238fd6062ca866fcf Cr-Commit-Position: refs/heads/master@{#383663} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/a6293982c7f5c0ad0e2ae56238fd6062ca866fcf Cr-Commit-Position: refs/heads/master@{#383663} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
