Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1430623004:
blimp: Add support for input handling (Closed)
|
Created:
5 years, 1 month ago by Khushal Modified:
4 years, 7 months ago CC:
chromium-reviews, James Su, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, tdresser+watch_chromium.org, shuchen+watch_chromium.org, jam, nyquist+watch-blimp_chromium.org, nona+watch_chromium.org, marcinjb+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, darin-cc_chromium.org, dtrainor+watch-blimp_chromium.org, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionblimp: Add support for basic input handling.
- Add BlimpInputManager to store all input state for the current render
widget.
- Grab and convert Android motion events to ui::MotionEvent sent to the
BlimpInputManager to generate WebGestureEvents using
ui::FilteredGestureProvider.
- Forward the WebInputEvents to the ui::InputHandlerProxy to be
handled by the cc::InputHandler on the compositor thread.
- Send the WebInputEvents not handled by the compositor to the engine.
BUG=548806
Committed: https://crrev.com/0f46911a547ea49a6461da202d0b49d10309be05
Cr-Commit-Position: refs/heads/master@{#363928}
Patch Set 1 #
Total comments: 19
Patch Set 2 : Add BlimpInputManager #Patch Set 3 : Address comments. #
Total comments: 5
Patch Set 4 : Rebase. #Patch Set 5 : Change shutdown. #Patch Set 6 : format fix. #
Total comments: 6
Patch Set 7 : Move all input handling on compositor thread to BlimpInputHandlerWrapper. #
Total comments: 14
Patch Set 8 : "Add dtor for BlimpInputHandlerWrapper" #
Total comments: 2
Patch Set 9 : Addressed nyquist@'s comments. #
Total comments: 2
Patch Set 10 : fix build error. #
Messages
Total messages: 36 (13 generated)
Description was changed from ========== blimp: Add support for input handling BUG=548806 ========== to ========== blimp: Add support for input handling BUG=548806 ==========
khushalsagar@chromium.org changed reviewers: + dtrainor@chromium.org
general review. working on build problem :). https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... File blimp/client/android/blimp_input_manager.h (right): https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... blimp/client/android/blimp_input_manager.h:6: #include "base/macros.h" ? https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... File blimp/client/android/blimp_view.cc (right): https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... blimp/client/android/blimp_view.cc:113: pos_x_0, fix indentation. Same for all other MotionEventAndroid objects below https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... blimp/client/android/blimp_view.cc:143: if (!result.succeeded) indentation https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... blimp/client/android/blimp_view.cc:153: // TODO(khushalsagar): Add support for touch events to blink. s/blink/blimp? https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... blimp/client/android/blimp_view.cc:171: // TODO(khushalsagar): Use the GestureEventQueue. TODO: Push the event to the compositor? https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... File blimp/client/android/blimp_view.h (right): https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... blimp/client/android/blimp_view.h:87: float device_scale_factor_; const? https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... File blimp/client/android/java/src/org/chromium/blimp/BlimpView.java (right): https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:88: private static boolean isValidTouchEventActionForNative(int eventAction) { Maybe just inline this or put it down right above the native method definitions. https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:107: if (mNativeBlimpViewPtr == 0) return false; Check this first before worrying about anything else IMO. https://chromiumcodereview.appspot.com/1430623004/diff/1/content/browser/rend... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://chromiumcodereview.appspot.com/1430623004/diff/1/content/browser/rend... content/browser/renderer_host/render_widget_host_view_android.cc:730: LOG(ERROR) << "Touch event consumed"; fix meh. https://chromiumcodereview.appspot.com/1430623004/diff/1/content/browser/rend... content/browser/renderer_host/render_widget_host_view_android.cc:1838: LOG(ERROR) << "OnGestureEvent"; meh too! https://chromiumcodereview.appspot.com/1430623004/diff/1/ui/events/android/ev... File ui/events/android/events_jni_registrar.h (right): https://chromiumcodereview.appspot.com/1430623004/diff/1/ui/events/android/ev... ui/events/android/events_jni_registrar.h:17: EVENTS_EXPORT bool RegisterJni(JNIEnv* env); Who calls this?
On 2015/11/02 16:55:30, David Trainor wrote: > general review. working on build problem :). > > https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... > File blimp/client/android/blimp_input_manager.h (right): > > https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... > blimp/client/android/blimp_input_manager.h:6: #include "base/macros.h" > ? > > https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... > File blimp/client/android/blimp_view.cc (right): > > https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... > blimp/client/android/blimp_view.cc:113: pos_x_0, > fix indentation. Same for all other MotionEventAndroid objects below > > https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... > blimp/client/android/blimp_view.cc:143: if (!result.succeeded) > indentation > > https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... > blimp/client/android/blimp_view.cc:153: // TODO(khushalsagar): Add support for > touch events to blink. > s/blink/blimp? > > https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... > blimp/client/android/blimp_view.cc:171: // TODO(khushalsagar): Use the > GestureEventQueue. > TODO: Push the event to the compositor? > > https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... > File blimp/client/android/blimp_view.h (right): > > https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... > blimp/client/android/blimp_view.h:87: float device_scale_factor_; > const? > > https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... > File blimp/client/android/java/src/org/chromium/blimp/BlimpView.java (right): > > https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... > blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:88: private > static boolean isValidTouchEventActionForNative(int eventAction) { > Maybe just inline this or put it down right above the native method definitions. > > https://chromiumcodereview.appspot.com/1430623004/diff/1/blimp/client/android... > blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:107: if > (mNativeBlimpViewPtr == 0) return false; > Check this first before worrying about anything else IMO. > > https://chromiumcodereview.appspot.com/1430623004/diff/1/content/browser/rend... > File content/browser/renderer_host/render_widget_host_view_android.cc (right): > > https://chromiumcodereview.appspot.com/1430623004/diff/1/content/browser/rend... > content/browser/renderer_host/render_widget_host_view_android.cc:730: LOG(ERROR) > << "Touch event consumed"; > fix meh. > > https://chromiumcodereview.appspot.com/1430623004/diff/1/content/browser/rend... > content/browser/renderer_host/render_widget_host_view_android.cc:1838: > LOG(ERROR) << "OnGestureEvent"; > meh too! > > https://chromiumcodereview.appspot.com/1430623004/diff/1/ui/events/android/ev... > File ui/events/android/events_jni_registrar.h (right): > > https://chromiumcodereview.appspot.com/1430623004/diff/1/ui/events/android/ev... > ui/events/android/events_jni_registrar.h:17: EVENTS_EXPORT bool > RegisterJni(JNIEnv* env); > Who calls this? I split the part for moving MotionEventAndroid in a separate patch. Will address the comment for the rest here. Got the build working for that too. :)
Description was changed from ========== blimp: Add support for input handling BUG=548806 ========== to ========== blimp: Add support for basic input handling. - Add BlimpInputManager to store all input state for the current render widget. - Grab and convert Android motion events to ui::MotionEvent sent to the BlimpInputManager to generate WebGestureEvents using ui::FilteredGestureProvider. - Forward the WebInputEvents to the ui::InputHandlerProxy to be handled by the cc::InputHandler on the compositor thread. - Send the WebInputEvents not handled by the compositor to the engine. BUG=548806 ==========
Updated the patch to move all input handling to the BlimpInputManager and tied it to send the input events to the engine using render_widget_message_processor. Will add tests. Could you take a look at it again? Thanks! https://codereview.chromium.org/1430623004/diff/1/blimp/client/android/blimp_... File blimp/client/android/blimp_input_manager.h (right): https://codereview.chromium.org/1430623004/diff/1/blimp/client/android/blimp_... blimp/client/android/blimp_input_manager.h:6: #include "base/macros.h" On 2015/11/02 16:55:30, David Trainor wrote: > ? Done. https://codereview.chromium.org/1430623004/diff/1/blimp/client/android/blimp_... File blimp/client/android/blimp_view.cc (right): https://codereview.chromium.org/1430623004/diff/1/blimp/client/android/blimp_... blimp/client/android/blimp_view.cc:113: pos_x_0, On 2015/11/02 16:55:30, David Trainor wrote: > fix indentation. Same for all other MotionEventAndroid objects below Done. https://codereview.chromium.org/1430623004/diff/1/blimp/client/android/blimp_... blimp/client/android/blimp_view.cc:143: if (!result.succeeded) On 2015/11/02 16:55:30, David Trainor wrote: > indentation Moved to BlimpInputManager. https://codereview.chromium.org/1430623004/diff/1/blimp/client/android/blimp_... blimp/client/android/blimp_view.cc:153: // TODO(khushalsagar): Add support for touch events to blink. On 2015/11/02 16:55:30, David Trainor wrote: > s/blink/blimp? Moved to BlimpInputManager. https://codereview.chromium.org/1430623004/diff/1/blimp/client/android/blimp_... blimp/client/android/blimp_view.cc:171: // TODO(khushalsagar): Use the GestureEventQueue. On 2015/11/02 16:55:30, David Trainor wrote: > TODO: Push the event to the compositor? Done. Moved to BlimpInputManager. https://codereview.chromium.org/1430623004/diff/1/blimp/client/android/blimp_... File blimp/client/android/blimp_view.h (right): https://codereview.chromium.org/1430623004/diff/1/blimp/client/android/blimp_... blimp/client/android/blimp_view.h:87: float device_scale_factor_; On 2015/11/02 16:55:30, David Trainor wrote: > const? Done. https://codereview.chromium.org/1430623004/diff/1/blimp/client/android/java/s... File blimp/client/android/java/src/org/chromium/blimp/BlimpView.java (right): https://codereview.chromium.org/1430623004/diff/1/blimp/client/android/java/s... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:88: private static boolean isValidTouchEventActionForNative(int eventAction) { On 2015/11/02 16:55:30, David Trainor wrote: > Maybe just inline this or put it down right above the native method definitions. Done. https://codereview.chromium.org/1430623004/diff/1/blimp/client/android/java/s... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:107: if (mNativeBlimpViewPtr == 0) return false; On 2015/11/02 16:55:30, David Trainor wrote: > Check this first before worrying about anything else IMO. Done.
https://codereview.chromium.org/1430623004/diff/40001/blimp/client/input/blim... File blimp/client/input/blimp_input_manager.cc (right): https://codereview.chromium.org/1430623004/diff/40001/blimp/client/input/blim... blimp/client/input/blimp_input_manager.cc:43: base::Unretained(this), input_handler)); Are we sure we want unretained for this? Unretained means no internal ref counting or safety IIUC (I might be wrong though). Don't we need a weak factory and weak pointers? Or is it guaranteed that we can't post a task and destroy the object after? Same for all other instances. https://codereview.chromium.org/1430623004/diff/40001/blimp/client/input/blim... blimp/client/input/blimp_input_manager.cc:81: NOTIMPLEMENTED()<< space between () and <<?
khushalsagar@chromium.org changed reviewers: + nyquist@chromium.org
https://codereview.chromium.org/1430623004/diff/40001/blimp/client/input/blim... File blimp/client/input/blimp_input_manager.cc (right): https://codereview.chromium.org/1430623004/diff/40001/blimp/client/input/blim... blimp/client/input/blimp_input_manager.cc:43: base::Unretained(this), input_handler)); On 2015/12/02 03:49:06, David Trainor wrote: > Are we sure we want unretained for this? Unretained means no internal ref > counting or safety IIUC (I might be wrong though). Don't we need a weak factory > and weak pointers? Or is it guaranteed that we can't post a task and destroy > the object after? > > Same for all other instances. You're right, its not safe to use Unretained for these calls. I have changed the shutdown sequence so we can use Unretained for tasks posted on the compositor thread, since we block on Shutdown to make sure that the input manager can not be called on the compositor thread after being destroyed. And added a weak ptr factory to post tasks to the main thread. Another approach that would avoid blocking the main thread would be to have a weak factory for the compositor thread and post a task on the main thread when we build the input handler proxy and pass the weak ptr from this factory that it can use to post tasks on the compositor thread. This way the main thread will just drop the input events till it is notified that the input handler proxy is created and it gets this weak ptr. And we can destroy this weak factory in WillShutdown() to ensure the pointers are invalidated on the compositor thread. Would that be better? https://codereview.chromium.org/1430623004/diff/40001/blimp/client/input/blim... blimp/client/input/blimp_input_manager.cc:81: NOTIMPLEMENTED()<< On 2015/12/02 03:49:06, David Trainor wrote: > space between () and <<? Done.
https://codereview.chromium.org/1430623004/diff/40001/blimp/client/input/blim... File blimp/client/input/blimp_input_manager.cc (right): https://codereview.chromium.org/1430623004/diff/40001/blimp/client/input/blim... blimp/client/input/blimp_input_manager.cc:43: base::Unretained(this), input_handler)); On 2015/12/03 11:31:38, Khushal wrote: > On 2015/12/02 03:49:06, David Trainor wrote: > > Are we sure we want unretained for this? Unretained means no internal ref > > counting or safety IIUC (I might be wrong though). Don't we need a weak > factory > > and weak pointers? Or is it guaranteed that we can't post a task and destroy > > the object after? > > > > Same for all other instances. > > You're right, its not safe to use Unretained for these calls. I have changed the > shutdown sequence so we can use Unretained for tasks posted on the compositor > thread, since we block on Shutdown to make sure that the input manager can not > be called on the compositor thread after being destroyed. And added a weak ptr > factory to post tasks to the main thread. > > Another approach that would avoid blocking the main thread would be to have a > weak factory for the compositor thread and post a task on the main thread when > we build the input handler proxy and pass the weak ptr from this factory that it Looking at this again, we'll still need to post a task to create the input handler proxy on the compositor thread. So I guess the destruction has to be blocking. :/ > can use to post tasks on the compositor thread. This way the main thread will > just drop the input events till it is notified that the input handler proxy is > created and it gets this weak ptr. And we can destroy this weak factory in > WillShutdown() to ensure the pointers are invalidated on the compositor thread. > Would that be better?
https://codereview.chromium.org/1430623004/diff/100001/blimp/client/composito... File blimp/client/compositor/blimp_compositor.cc (right): https://codereview.chromium.org/1430623004/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor.cc:166: void BlimpCompositor::DidInitializeOutputSurface() { revert? https://codereview.chromium.org/1430623004/diff/100001/blimp/client/input/bli... File blimp/client/input/blimp_input_manager.h (right): https://codereview.chromium.org/1430623004/diff/100001/blimp/client/input/bli... blimp/client/input/blimp_input_manager.h:37: // 2) BlimpInputManager blocks the main thread to ensure the compositor-thread Can you be a little more explicit that this is blocking the main thread to flush all posted tasks to the compositor thread? https://codereview.chromium.org/1430623004/diff/100001/blimp/client/input/bli... blimp/client/input/blimp_input_manager.h:119: bool main_thread_blocked_; Can you put a comment on how/where this is expected to be accessed?
Moved all code for input handling on compositor thread to BlimpInputHandlerWrapper. I anticipate we'll add more logic to the BlimpInputManager to handle overscrolls, or using a gesture event queue later. This will help keep the compositor thread boundary clear. https://codereview.chromium.org/1430623004/diff/100001/blimp/client/composito... File blimp/client/compositor/blimp_compositor.cc (right): https://codereview.chromium.org/1430623004/diff/100001/blimp/client/composito... blimp/client/compositor/blimp_compositor.cc:166: void BlimpCompositor::DidInitializeOutputSurface() { On 2015/12/03 16:14:04, David Trainor wrote: > revert? Was this meant for another line? https://codereview.chromium.org/1430623004/diff/100001/blimp/client/input/bli... File blimp/client/input/blimp_input_manager.h (right): https://codereview.chromium.org/1430623004/diff/100001/blimp/client/input/bli... blimp/client/input/blimp_input_manager.h:37: // 2) BlimpInputManager blocks the main thread to ensure the compositor-thread On 2015/12/03 16:14:04, David Trainor wrote: > Can you be a little more explicit that this is blocking the main thread to flush > all posted tasks to the compositor thread? Done. https://codereview.chromium.org/1430623004/diff/100001/blimp/client/input/bli... blimp/client/input/blimp_input_manager.h:119: bool main_thread_blocked_; On 2015/12/03 16:14:04, David Trainor wrote: > Can you put a comment on how/where this is expected to be accessed? Done. Though, I'm not sure if its necessary. We could just add a test for this.
lgtm
Description was changed from ========== blimp: Add support for basic input handling. - Add BlimpInputManager to store all input state for the current render widget. - Grab and convert Android motion events to ui::MotionEvent sent to the BlimpInputManager to generate WebGestureEvents using ui::FilteredGestureProvider. - Forward the WebInputEvents to the ui::InputHandlerProxy to be handled by the cc::InputHandler on the compositor thread. - Send the WebInputEvents not handled by the compositor to the engine. BUG=548806 ========== to ========== blimp: Add support for basic input handling. - Add BlimpInputManager to store all input state for the current render widget. - Grab and convert Android motion events to ui::MotionEvent sent to the BlimpInputManager to generate WebGestureEvents using ui::FilteredGestureProvider. - Forward the WebInputEvents to the ui::InputHandlerProxy to be handled by the cc::InputHandler on the compositor thread. - Send the WebInputEvents not handled by the compositor to the engine. BUG=548806 ==========
khushalsagar@chromium.org changed reviewers: + rbyers@chromium.org, tdresser@chromium.org
tdresser@, could you review the ui/ DEPS. rbyers@, could you review the DEPS for web input events. Thanks!
https://codereview.chromium.org/1430623004/diff/120001/blimp/client/android/j... File blimp/client/android/java/src/org/chromium/blimp/BlimpView.java (right): https://codereview.chromium.org/1430623004/diff/120001/blimp/client/android/j... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:99: final int pointerCount = event.getPointerCount(); Nit: unnecessary final here and below. https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... File blimp/client/input/blimp_input_handler_wrapper.h (right): https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... blimp/client/input/blimp_input_handler_wrapper.h:17: // This class is created and lives on the compositor thread. What is it used for? It owns a scoped_ptr to the InputHandlerProxy; so what's the difference between a proxy and a wrapper? Where does it get events from? Where does it send it back? https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... blimp/client/input/blimp_input_handler_wrapper.h:25: void HandleWebInputEvent(const blink::WebInputEvent& input_event); Who calls this? When should this be called? What is the expected output? https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... File blimp/client/input/blimp_input_manager.cc (right): https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... blimp/client/input/blimp_input_manager.cc:90: // TODO(jdduke): Remove this workaround after Android fixes UiAutomator to I'm not sure jdduke@ will be able to look at this. I don't think he focused on Chrome anymore. https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... File blimp/client/input/blimp_input_manager.h (right): https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... blimp/client/input/blimp_input_manager.h:55: // consumed is false if the event was not handled by the compositor and should |consumed| https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... blimp/client/input/blimp_input_manager.h:70: // called on the compositor thread. Nit: Called https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... blimp/client/input/blimp_input_manager.h:93: scoped_ptr<BlimpInputHandlerWrapper> input_handler_; should this be input_handler_wrapper_?
https://codereview.chromium.org/1430623004/diff/140001/blimp/client/BUILD.gn File blimp/client/BUILD.gn (right): https://codereview.chromium.org/1430623004/diff/140001/blimp/client/BUILD.gn#... blimp/client/BUILD.gn:45: "//ui/events", There was a time when Android didn't depend on ui/events, only on ui/events/events_base. (https://codereview.chromium.org/241213002) It looks like that's no longer the case, which is a shame. Unless we have a good reason to, we shouldn't be adding new cases where Android depends on ui/events. Do we have a good reason to do so here?
https://codereview.chromium.org/1430623004/diff/140001/blimp/client/BUILD.gn File blimp/client/BUILD.gn (right): https://codereview.chromium.org/1430623004/diff/140001/blimp/client/BUILD.gn#... blimp/client/BUILD.gn:45: "//ui/events", On 2015/12/07 14:32:44, tdresser wrote: > There was a time when Android didn't depend on ui/events, only on > ui/events/events_base. (https://codereview.chromium.org/241213002) > > It looks like that's no longer the case, which is a shame. Unless we have a good > reason to, we shouldn't be adding new cases where Android depends on ui/events. > Do we have a good reason to do so here? We actually need this for motion_event_android.h which we had to add to ui/events when we were isolating the input files needed from content. https://codereview.chromium.org/1417173008/ Do you think there is a different target that would be more suitable for this file?
DEPS LGTM
https://codereview.chromium.org/1430623004/diff/120001/blimp/client/android/j... File blimp/client/android/java/src/org/chromium/blimp/BlimpView.java (right): https://codereview.chromium.org/1430623004/diff/120001/blimp/client/android/j... blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:99: final int pointerCount = event.getPointerCount(); On 2015/12/05 01:54:42, nyquist wrote: > Nit: unnecessary final here and below. Done. https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... File blimp/client/input/blimp_input_handler_wrapper.h (right): https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... blimp/client/input/blimp_input_handler_wrapper.h:17: // This class is created and lives on the compositor thread. On 2015/12/05 01:54:42, nyquist wrote: > What is it used for? > It owns a scoped_ptr to the InputHandlerProxy; so what's the difference between > a proxy and a wrapper? > > Where does it get events from? Where does it send it back? Done. https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... blimp/client/input/blimp_input_handler_wrapper.h:25: void HandleWebInputEvent(const blink::WebInputEvent& input_event); On 2015/12/05 01:54:42, nyquist wrote: > Who calls this? When should this be called? What is the expected output? Done. https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... File blimp/client/input/blimp_input_manager.cc (right): https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... blimp/client/input/blimp_input_manager.cc:90: // TODO(jdduke): Remove this workaround after Android fixes UiAutomator to On 2015/12/05 01:54:42, nyquist wrote: > I'm not sure jdduke@ will be able to look at this. I don't think he focused on > Chrome anymore. Oh. I'll assign it to myself then. https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... File blimp/client/input/blimp_input_manager.h (right): https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... blimp/client/input/blimp_input_manager.h:55: // consumed is false if the event was not handled by the compositor and should On 2015/12/05 01:54:42, nyquist wrote: > |consumed| Done. https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... blimp/client/input/blimp_input_manager.h:70: // called on the compositor thread. On 2015/12/05 01:54:42, nyquist wrote: > Nit: Called Done. https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... blimp/client/input/blimp_input_manager.h:93: scoped_ptr<BlimpInputHandlerWrapper> input_handler_; On 2015/12/05 01:54:42, nyquist wrote: > should this be input_handler_wrapper_? Yeah, that will keep things clearer. Done.
On 2015/12/07 23:01:22, Khushal wrote: > https://codereview.chromium.org/1430623004/diff/120001/blimp/client/android/j... > File blimp/client/android/java/src/org/chromium/blimp/BlimpView.java (right): > > https://codereview.chromium.org/1430623004/diff/120001/blimp/client/android/j... > blimp/client/android/java/src/org/chromium/blimp/BlimpView.java:99: final int > pointerCount = event.getPointerCount(); > On 2015/12/05 01:54:42, nyquist wrote: > > Nit: unnecessary final here and below. > > Done. > > https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... > File blimp/client/input/blimp_input_handler_wrapper.h (right): > > https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... > blimp/client/input/blimp_input_handler_wrapper.h:17: // This class is created > and lives on the compositor thread. > On 2015/12/05 01:54:42, nyquist wrote: > > What is it used for? > > It owns a scoped_ptr to the InputHandlerProxy; so what's the difference > between > > a proxy and a wrapper? > > > > Where does it get events from? Where does it send it back? > > Done. > > https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... > blimp/client/input/blimp_input_handler_wrapper.h:25: void > HandleWebInputEvent(const blink::WebInputEvent& input_event); > On 2015/12/05 01:54:42, nyquist wrote: > > Who calls this? When should this be called? What is the expected output? > > Done. > > https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... > File blimp/client/input/blimp_input_manager.cc (right): > > https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... > blimp/client/input/blimp_input_manager.cc:90: // TODO(jdduke): Remove this > workaround after Android fixes UiAutomator to > On 2015/12/05 01:54:42, nyquist wrote: > > I'm not sure jdduke@ will be able to look at this. I don't think he focused on > > Chrome anymore. > > Oh. I'll assign it to myself then. > > https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... > File blimp/client/input/blimp_input_manager.h (right): > > https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... > blimp/client/input/blimp_input_manager.h:55: // consumed is false if the event > was not handled by the compositor and should > On 2015/12/05 01:54:42, nyquist wrote: > > |consumed| > > Done. > > https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... > blimp/client/input/blimp_input_manager.h:70: // called on the compositor thread. > On 2015/12/05 01:54:42, nyquist wrote: > > Nit: Called > > Done. > > https://codereview.chromium.org/1430623004/diff/120001/blimp/client/input/bli... > blimp/client/input/blimp_input_manager.h:93: > scoped_ptr<BlimpInputHandlerWrapper> input_handler_; > On 2015/12/05 01:54:42, nyquist wrote: > > should this be input_handler_wrapper_? > > Yeah, that will keep things clearer. Done. Ideally we'd be able to just depend on "//ui/events:events_base", but it looks like we've given up on keeping the events target off of Android. DEPS LGTM.
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/1430623004/#ps160001 (title: "Addressed nyquist@'s comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1430623004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1430623004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm https://codereview.chromium.org/1430623004/diff/160001/blimp/client/android/b... File blimp/client/android/blimp_view.cc (right): https://codereview.chromium.org/1430623004/diff/160001/blimp/client/android/b... blimp/client/android/blimp_view.cc:127: ui::MotionEventAndroid::Pointer pointer0(pointer_id_0, Seems like you're hitting an issue here: ../../blimp/client/android/blimp_view.cc:134: error: undefined reference to 'ui::MotionEventAndroid::Pointer::Pointer(int, float, float, float, float, float, float, int)' ../../blimp/client/android/blimp_view.cc:142: error: undefined reference to 'ui::MotionEventAndroid::Pointer::Pointer(int, float, float, float, float, float, float, int)' ../../blimp/client/android/blimp_view.cc:156: error: undefined reference to 'ui::MotionEventAndroid::MotionEventAndroid(float, _JNIEnv*, _jobject*, long long, int, int, int, int, int, int, float, float, ui::MotionEventAndroid::Pointer const&, ui::MotionEventAndroid::Pointer const&)' ../../blimp/client/android/blimp_view.cc:158: error: undefined reference to 'ui::MotionEventAndroid::~MotionEventAndroid()'
https://codereview.chromium.org/1430623004/diff/160001/blimp/client/android/b... File blimp/client/android/blimp_view.cc (right): https://codereview.chromium.org/1430623004/diff/160001/blimp/client/android/b... blimp/client/android/blimp_view.cc:127: ui::MotionEventAndroid::Pointer pointer0(pointer_id_0, On 2015/12/08 22:24:24, nyquist wrote: > Seems like you're hitting an issue here: > > ../../blimp/client/android/blimp_view.cc:134: error: undefined reference to > 'ui::MotionEventAndroid::Pointer::Pointer(int, float, float, float, float, > float, float, int)' > ../../blimp/client/android/blimp_view.cc:142: error: undefined reference to > 'ui::MotionEventAndroid::Pointer::Pointer(int, float, float, float, float, > float, float, int)' > ../../blimp/client/android/blimp_view.cc:156: error: undefined reference to > 'ui::MotionEventAndroid::MotionEventAndroid(float, _JNIEnv*, _jobject*, long > long, int, int, int, int, int, int, float, float, > ui::MotionEventAndroid::Pointer const&, ui::MotionEventAndroid::Pointer const&)' > ../../blimp/client/android/blimp_view.cc:158: error: undefined reference to > 'ui::MotionEventAndroid::~MotionEventAndroid()' Thanks. Needed a deps fix.
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, dtrainor@chromium.org, tdresser@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/1430623004/#ps180001 (title: "fix build error.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1430623004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1430623004/180001
Message was sent while issue was closed.
Description was changed from ========== blimp: Add support for basic input handling. - Add BlimpInputManager to store all input state for the current render widget. - Grab and convert Android motion events to ui::MotionEvent sent to the BlimpInputManager to generate WebGestureEvents using ui::FilteredGestureProvider. - Forward the WebInputEvents to the ui::InputHandlerProxy to be handled by the cc::InputHandler on the compositor thread. - Send the WebInputEvents not handled by the compositor to the engine. BUG=548806 ========== to ========== blimp: Add support for basic input handling. - Add BlimpInputManager to store all input state for the current render widget. - Grab and convert Android motion events to ui::MotionEvent sent to the BlimpInputManager to generate WebGestureEvents using ui::FilteredGestureProvider. - Forward the WebInputEvents to the ui::InputHandlerProxy to be handled by the cc::InputHandler on the compositor thread. - Send the WebInputEvents not handled by the compositor to the engine. BUG=548806 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== blimp: Add support for basic input handling. - Add BlimpInputManager to store all input state for the current render widget. - Grab and convert Android motion events to ui::MotionEvent sent to the BlimpInputManager to generate WebGestureEvents using ui::FilteredGestureProvider. - Forward the WebInputEvents to the ui::InputHandlerProxy to be handled by the cc::InputHandler on the compositor thread. - Send the WebInputEvents not handled by the compositor to the engine. BUG=548806 ========== to ========== blimp: Add support for basic input handling. - Add BlimpInputManager to store all input state for the current render widget. - Grab and convert Android motion events to ui::MotionEvent sent to the BlimpInputManager to generate WebGestureEvents using ui::FilteredGestureProvider. - Forward the WebInputEvents to the ui::InputHandlerProxy to be handled by the cc::InputHandler on the compositor thread. - Send the WebInputEvents not handled by the compositor to the engine. BUG=548806 Committed: https://crrev.com/0f46911a547ea49a6461da202d0b49d10309be05 Cr-Commit-Position: refs/heads/master@{#363928} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/0f46911a547ea49a6461da202d0b49d10309be05 Cr-Commit-Position: refs/heads/master@{#363928} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
