Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2350253004:
Controller support for VrShell (Closed)
DescriptionController support for VrShell
VrControllerManager has an instance of the VrController which detects
the scroll and click gestures.
VrControllerManager also is responsible for sending the generated events
to the UI Thread.
BUG=641461
Committed: https://crrev.com/230e7a29d98a166df6de4b9518e631dff553a739
Cr-Commit-Position: refs/heads/master@{#422179}
Patch Set 1 #
Total comments: 89
Patch Set 2 : renamed vr_content_view_core to vr_input_manager #Patch Set 3 : removed unused files #
Total comments: 33
Patch Set 4 : rebased + moved vr_input_manager #
Total comments: 66
Patch Set 5 : Controller support for VrShell #
Total comments: 4
Patch Set 6 : Controller support for VrShell #
Total comments: 4
Patch Set 7 : Controller support for VrShell #
Total comments: 3
Patch Set 8 : Removed GestureType #
Total comments: 8
Patch Set 9 : Clean up #Messages
Total messages: 52 (12 generated)
Description was changed from ========== Controller support for VrShell VrControllerManager has an instance of the VrController which detects the scroll and click gestures. VrControllerManager also is responsible for sending the generated events to the UI Thread. BUG=641461 ========== to ========== Controller support for VrShell VrControllerManager has an instance of the VrController which detects the scroll and click gestures. VrControllerManager also is responsible for sending the generated events to the UI Thread. BUG=641461 ==========
asimjour@google.com changed reviewers: + bshe@chromium.org, cjgrant@chromium.org, mthiesse@chromium.org
some format nits https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:15: inline void clamp_touchpad_position(vec2f* position) { nit: rename function names to CamelCase. And if these functions are only used by this file. Put them in anonymous namespace. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_controller.h (right): https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.h:18: namespace { nit: move this to .cc https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_controller_manager.cc (right): https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller_manager.cc:11: VrControllerManager::VrControllerManager(gvr_context_* gvr_context) {} is gvr_context neccessary? it doesn't look like it is used here. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_controller_manager.h (right): https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller_manager.h:26: std::unique_ptr<gvr::ControllerApi> controller_api_; nit: move to private. And if you want others to access it, add a public getter like gvr::ControllerApi* controller_api() https://codereview.chromium.org/2350253004/diff/1/content/browser/android/vr_... File content/browser/android/vr_content_view_core_impl_delegate.cc (right): https://codereview.chromium.org/2350253004/diff/1/content/browser/android/vr_... content/browser/android/vr_content_view_core_impl_delegate.cc:26: } // namespace nit: remove this namespace. https://codereview.chromium.org/2350253004/diff/1/content/browser/android/vr_... content/browser/android/vr_content_view_core_impl_delegate.cc:42: VrContentViewCore* VrContentViewCore::FromContentViewCore( I am not sure I understand why do you want to have two static FromContentViewCore functions? Would one enough? https://codereview.chromium.org/2350253004/diff/1/content/browser/android/vr_... File content/browser/android/vr_content_view_core_impl_delegate.h (right): https://codereview.chromium.org/2350253004/diff/1/content/browser/android/vr_... content/browser/android/vr_content_view_core_impl_delegate.h:17: class VrContentViewCoreImplDelegate : public VrContentViewCore { I am not sure I understand why do you want to add Delegate in the class name. Perhaps just VrContentViewCoreImpl? https://codereview.chromium.org/2350253004/diff/1/content/browser/android/vr_... content/browser/android/vr_content_view_core_impl_delegate.h:19: VrContentViewCoreImplDelegate(ContentViewCore* content_view_core); nit: explicit https://codereview.chromium.org/2350253004/diff/1/content/browser/android/vr_... content/browser/android/vr_content_view_core_impl_delegate.h:23: add comment "// VrcontentViewCore overrides:" https://codereview.chromium.org/2350253004/diff/1/content/public/browser/andr... File content/public/browser/android/vr_content_view_core.h (right): https://codereview.chromium.org/2350253004/diff/1/content/public/browser/andr... content/public/browser/android/vr_content_view_core.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: s/Copyright (c) 2012/Copyright 2016 https://codereview.chromium.org/2350253004/diff/1/content/public/browser/andr... content/public/browser/android/vr_content_view_core.h:19: // DEPRECATED. Do not add methods. Remove the comments here. We don't want to land DEPRECATED code :) https://codereview.chromium.org/2350253004/diff/1/content/public/browser/andr... File content/public/browser/android/vr_gesture.h (right): https://codereview.chromium.org/2350253004/diff/1/content/public/browser/andr... content/public/browser/android/vr_gesture.h:5: #ifndef CHROME_BROWSER_ANDROID_VR_SHELL_VR_GESTURE_H_ nit: s/CHROME/CONTENT
Going for lunch, will review more after. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:54: VrController::VrController(gvr_context_* vr_context) : zoom_in_progress(false) { optional nit: We've been putting default values in the .h file, but this is a purely stylistic choice. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:83: quatf orientation = {controller_state_.GetOrientation().qx, optional: return *(quatf*)&controller_state_.GetOrientation(); https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:130: if (touch_info_ == nullptr) CHECK(touch_info_ != nullptr) << "touch_info_ not initialized properly." https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:136: clamp_touchpad_position(&(touch_info_->touch_point.position)); nit: unnecessary parens https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:144: CHECK(controller_api_); unnecessary CHECK. Did you mean to CHECK gvr_context? https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:251: const VrGesture* VrController::GetGesturePtr(const int index) { const size_t index? https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:252: if (index >= (int)gesture_list_.size()) CHECK this https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:254: "list."; nit: I think this fits on previous line. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:260: if (touch_info_ == nullptr) CHECK this https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:278: // user puts finger on touch pad (or when the touch down for current gesture nit: Capital and period. Also below https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_controller.h (right): https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.h:21: // A slop represents a small rectangular region around the first touch point of nit: New line before comment, throughout this CL. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.h:31: constexpr float kRc = static_cast<float>(1.0 / (2.0 * M_PI * kCutoffHz)); nit: kRC https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.h:41: // Must be called when the Activity gets onResume(). nit: OnResume() below too. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.h:73: WAITING, // waiting for user to touch down nit: extra space https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.h:105: float last_qx; nit: trailing underscore Also below. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.h:124: void UpdateGestureFromTouchInfo(); nit: Functions before member variables. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.h:157: // before, nit: no line break
What's the purpose of vr_controller_manager? It looks to me like it's entirely unnecessary, and just forwards almost all of its calls to the vr_controller. Why not just have vr_shell.cc act as the vr_controller_manager and remove a layer of indirection? https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:213: (dqx * 2) + 1, Orientation()); Can you comment on why * 2 + 1? Also, unnecessary parenss. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:215: if (Orientation().qz < 0.25f && Orientation().qz > -0.25f && Magic numbers to constants? https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_controller_manager.cc (right): https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller_manager.cc:16: VLOG(1) << "Controller-Resume"; Is there a debug only logging path? Might make sense to use here. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller_manager.cc:22: content::ContentViewCore* content_content_view_core_ptr, People don't seem to like the content_content_ naming scheme. I'd suggest content_cvc_ptr, and ui_cvc_ptr. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller_manager.cc:49: void VrControllerManager::ProcessUpdatedGesture(VrGesture gesture) { Why not pass the CVC pointer in here, and not store it in this class? Then we only need one ProcessUpdatedGesture function. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller_manager.cc:50: content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE, You should probably either check that you're not already on the UI thread, or move the PostTask to the callsite and require this method to be called from the UI thread. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller_manager.cc:95: const gvr_quatf VrControllerManager::Orientation() { nit: gvr::Quatf https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller_manager.cc:97: vr_controller_->Orientation().qx, vr_controller_->Orientation().qy, This feels wrong. vr_controller_->Orientation() converts a gvr::Quatf to a quatf, and this function converts it back to a gvr::Quatf. Why not skip the conversion altogether? https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_controller_manager.h (right): https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller_manager.h:71: void SendGesture(VrGesture gesture, nit: Functions above member variables. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller_manager.h:74: protected: nit: Protected above private seems to be more common?
https://codereview.chromium.org/2350253004/diff/1/content/browser/android/vr_... File content/browser/android/vr_content_view_core_impl_delegate.cc (right): https://codereview.chromium.org/2350253004/diff/1/content/browser/android/vr_... content/browser/android/vr_content_view_core_impl_delegate.cc:41: // static newline https://codereview.chromium.org/2350253004/diff/1/content/browser/android/vr_... File content/browser/android/vr_content_view_core_impl_delegate.h (right): https://codereview.chromium.org/2350253004/diff/1/content/browser/android/vr_... content/browser/android/vr_content_view_core_impl_delegate.h:17: class VrContentViewCoreImplDelegate : public VrContentViewCore { On 2016/09/21 15:18:41, bshe wrote: > I am not sure I understand why do you want to add Delegate in the class name. > Perhaps just VrContentViewCoreImpl? Also, we're not implementing a ContentViewCore. Why not call this VrInputManagerImpl? https://codereview.chromium.org/2350253004/diff/1/content/public/browser/andr... File content/public/browser/android/vr_content_view_core.h (right): https://codereview.chromium.org/2350253004/diff/1/content/public/browser/andr... content/public/browser/android/vr_content_view_core.h:21: class CONTENT_EXPORT VrContentViewCore { This name feels wrong. We're not a ContentViewCore. Why not call this something like VrInputController or VrInputManager? https://codereview.chromium.org/2350253004/diff/1/content/public/browser/andr... File content/public/browser/android/vr_gesture.h (right): https://codereview.chromium.org/2350253004/diff/1/content/public/browser/andr... content/public/browser/android/vr_gesture.h:15: typedef struct quatf { I don't think you need this here at all. I don't think you're using all of the components of the quat in the content layer anyways, so you should probably just pass in the individual components you use and not create a new type here. https://codereview.chromium.org/2350253004/diff/1/content/public/browser/andr... content/public/browser/android/vr_gesture.h:20: /// qw is the linelar component. nit: linear? https://codereview.chromium.org/2350253004/diff/1/content/public/browser/andr... content/public/browser/android/vr_gesture.h:25: typedef struct vec2f { use gfx::Vector2d https://codereview.chromium.org/2350253004/diff/1/content/public/browser/andr... content/public/browser/android/vr_gesture.h:88: struct VrGesture { I don't think this should live in the content layer. Is it even used anywhere in the content layer?
asimjour@google.com changed reviewers: + asimjour@google.com
https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:15: inline void clamp_touchpad_position(vec2f* position) { On 2016/09/21 15:18:41, bshe wrote: > nit: rename function names to CamelCase. And if these functions are only used by > this file. Put them in anonymous namespace. Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:54: VrController::VrController(gvr_context_* vr_context) : zoom_in_progress(false) { On 2016/09/21 16:13:43, mthiesse wrote: > optional nit: We've been putting default values in the .h file, but this is a > purely stylistic choice. Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:83: quatf orientation = {controller_state_.GetOrientation().qx, On 2016/09/21 16:13:43, mthiesse wrote: > optional: return *(quatf*)&controller_state_.GetOrientation(); replaced with controller_state_.GetOrientation() https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:130: if (touch_info_ == nullptr) On 2016/09/21 16:13:43, mthiesse wrote: > CHECK(touch_info_ != nullptr) << "touch_info_ not initialized properly." Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:136: clamp_touchpad_position(&(touch_info_->touch_point.position)); On 2016/09/21 16:13:43, mthiesse wrote: > nit: unnecessary parens Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:144: CHECK(controller_api_); On 2016/09/21 16:13:43, mthiesse wrote: > unnecessary CHECK. Did you mean to CHECK gvr_context? Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:213: (dqx * 2) + 1, Orientation()); On 2016/09/21 17:27:46, mthiesse wrote: > Can you comment on why * 2 + 1? Also, unnecessary parenss. Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:215: if (Orientation().qz < 0.25f && Orientation().qz > -0.25f && On 2016/09/21 17:27:46, mthiesse wrote: > Magic numbers to constants? Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:251: const VrGesture* VrController::GetGesturePtr(const int index) { On 2016/09/21 16:13:43, mthiesse wrote: > const size_t index? Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:252: if (index >= (int)gesture_list_.size()) On 2016/09/21 16:13:43, mthiesse wrote: > CHECK this Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:254: "list."; On 2016/09/21 16:13:43, mthiesse wrote: > nit: I think this fits on previous line. Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:260: if (touch_info_ == nullptr) On 2016/09/21 16:13:43, mthiesse wrote: > CHECK this Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.cc:278: // user puts finger on touch pad (or when the touch down for current gesture On 2016/09/21 16:13:43, mthiesse wrote: > nit: Capital and period. > > Also below Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_controller.h (right): https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.h:18: namespace { On 2016/09/21 15:18:41, bshe wrote: > nit: move this to .cc Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.h:21: // A slop represents a small rectangular region around the first touch point of On 2016/09/21 16:13:44, mthiesse wrote: > nit: New line before comment, throughout this CL. Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.h:31: constexpr float kRc = static_cast<float>(1.0 / (2.0 * M_PI * kCutoffHz)); On 2016/09/21 16:13:44, mthiesse wrote: > nit: kRC Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.h:41: // Must be called when the Activity gets onResume(). On 2016/09/21 16:13:44, mthiesse wrote: > nit: OnResume() > > below too. Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.h:73: WAITING, // waiting for user to touch down On 2016/09/21 16:13:44, mthiesse wrote: > nit: extra space Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.h:105: float last_qx; On 2016/09/21 16:13:44, mthiesse wrote: > nit: trailing underscore > > Also below. Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.h:124: void UpdateGestureFromTouchInfo(); On 2016/09/21 16:13:44, mthiesse wrote: > nit: Functions before member variables. Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller.h:157: // before, On 2016/09/21 16:13:44, mthiesse wrote: > nit: no line break Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_controller_manager.cc (right): https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller_manager.cc:11: VrControllerManager::VrControllerManager(gvr_context_* gvr_context) {} On 2016/09/21 15:18:41, bshe wrote: > is gvr_context neccessary? it doesn't look like it is used here. Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller_manager.cc:16: VLOG(1) << "Controller-Resume"; On 2016/09/21 17:27:47, mthiesse wrote: > Is there a debug only logging path? Might make sense to use here. removed unnecessary logs https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller_manager.cc:22: content::ContentViewCore* content_content_view_core_ptr, On 2016/09/21 17:27:47, mthiesse wrote: > People don't seem to like the content_content_ naming scheme. I'd suggest > content_cvc_ptr, and ui_cvc_ptr. Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller_manager.cc:49: void VrControllerManager::ProcessUpdatedGesture(VrGesture gesture) { On 2016/09/21 17:27:47, mthiesse wrote: > Why not pass the CVC pointer in here, and not store it in this class? > > Then we only need one ProcessUpdatedGesture function. If we pass the vr_cvc pointer vr_shell need to know about the vr_cvc, which looks to be unnecessary. We can pass cvc pointer, but then the ProcessUpdatedGesture would be slightly more complex with getting the appropriate vr_cvc every time. We only have two different vr_cvc, and I don't think that in the near future there would be any additional cvc, so having two functions looks to be fine. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller_manager.cc:50: content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE, On 2016/09/21 17:27:47, mthiesse wrote: > You should probably either check that you're not already on the UI thread, or > move the PostTask to the callsite and require this method to be called from the > UI thread. Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller_manager.cc:95: const gvr_quatf VrControllerManager::Orientation() { On 2016/09/21 17:27:47, mthiesse wrote: > nit: gvr::Quatf Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller_manager.cc:97: vr_controller_->Orientation().qx, vr_controller_->Orientation().qy, On 2016/09/21 17:27:47, mthiesse wrote: > This feels wrong. vr_controller_->Orientation() converts a gvr::Quatf to a > quatf, and this function converts it back to a gvr::Quatf. Why not skip the > conversion altogether? fixed https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_controller_manager.h (right): https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller_manager.h:26: std::unique_ptr<gvr::ControllerApi> controller_api_; On 2016/09/21 15:18:41, bshe wrote: > nit: move to private. And if you want others to access it, add a public getter > like > gvr::ControllerApi* controller_api() Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller_manager.h:71: void SendGesture(VrGesture gesture, On 2016/09/21 17:27:47, mthiesse wrote: > nit: Functions above member variables. Done. https://codereview.chromium.org/2350253004/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_controller_manager.h:74: protected: On 2016/09/21 17:27:47, mthiesse wrote: > nit: Protected above private seems to be more common? Done. https://codereview.chromium.org/2350253004/diff/1/content/browser/android/vr_... File content/browser/android/vr_content_view_core_impl_delegate.cc (right): https://codereview.chromium.org/2350253004/diff/1/content/browser/android/vr_... content/browser/android/vr_content_view_core_impl_delegate.cc:26: } // namespace On 2016/09/21 15:18:41, bshe wrote: > nit: remove this namespace. Done. https://codereview.chromium.org/2350253004/diff/1/content/browser/android/vr_... content/browser/android/vr_content_view_core_impl_delegate.cc:41: // static On 2016/09/21 17:43:27, mthiesse wrote: > newline Done. https://codereview.chromium.org/2350253004/diff/1/content/browser/android/vr_... content/browser/android/vr_content_view_core_impl_delegate.cc:42: VrContentViewCore* VrContentViewCore::FromContentViewCore( On 2016/09/21 15:18:41, bshe wrote: > I am not sure I understand why do you want to have two static > FromContentViewCore functions? Would one enough? There are two static functions here, One of these functions is the implementation of the VrContentViewCore::FromContentViewCore which is originally defined in content/public and will be accessed from a chrome/browser The second one is the implementation that is used by the first static function. The second static function is hidden from chrome/browser. https://codereview.chromium.org/2350253004/diff/1/content/browser/android/vr_... File content/browser/android/vr_content_view_core_impl_delegate.h (right): https://codereview.chromium.org/2350253004/diff/1/content/browser/android/vr_... content/browser/android/vr_content_view_core_impl_delegate.h:19: VrContentViewCoreImplDelegate(ContentViewCore* content_view_core); On 2016/09/21 15:18:41, bshe wrote: > nit: explicit Done. https://codereview.chromium.org/2350253004/diff/1/content/browser/android/vr_... content/browser/android/vr_content_view_core_impl_delegate.h:23: On 2016/09/21 15:18:41, bshe wrote: > add comment > "// VrcontentViewCore overrides:" Done. https://codereview.chromium.org/2350253004/diff/1/content/public/browser/andr... File content/public/browser/android/vr_content_view_core.h (right): https://codereview.chromium.org/2350253004/diff/1/content/public/browser/andr... content/public/browser/android/vr_content_view_core.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/09/21 15:18:41, bshe wrote: > nit: s/Copyright (c) 2012/Copyright 2016 Done. https://codereview.chromium.org/2350253004/diff/1/content/public/browser/andr... content/public/browser/android/vr_content_view_core.h:19: // DEPRECATED. Do not add methods. On 2016/09/21 15:18:41, bshe wrote: > Remove the comments here. We don't want to land DEPRECATED code :) Done. https://codereview.chromium.org/2350253004/diff/1/content/public/browser/andr... File content/public/browser/android/vr_gesture.h (right): https://codereview.chromium.org/2350253004/diff/1/content/public/browser/andr... content/public/browser/android/vr_gesture.h:5: #ifndef CHROME_BROWSER_ANDROID_VR_SHELL_VR_GESTURE_H_ On 2016/09/21 15:18:41, bshe wrote: > nit: s/CHROME/CONTENT Done. https://codereview.chromium.org/2350253004/diff/1/content/public/browser/andr... content/public/browser/android/vr_gesture.h:15: typedef struct quatf { On 2016/09/21 17:43:27, mthiesse wrote: > I don't think you need this here at all. I don't think you're using all of the > components of the quat in the content layer anyways, so you should probably just > pass in the individual components you use and not create a new type here. considering that I'm moving this file to chrome/browser/android/vr_shell, I'll replace this definitions with gvr_quatf and gvr_vec2f https://codereview.chromium.org/2350253004/diff/1/content/public/browser/andr... content/public/browser/android/vr_gesture.h:20: /// qw is the linelar component. On 2016/09/21 17:43:28, mthiesse wrote: > nit: linear? Done. https://codereview.chromium.org/2350253004/diff/1/content/public/browser/andr... content/public/browser/android/vr_gesture.h:25: typedef struct vec2f { On 2016/09/21 17:43:27, mthiesse wrote: > use gfx::Vector2d Replaced with gvr_vec2f https://codereview.chromium.org/2350253004/diff/1/content/public/browser/andr... content/public/browser/android/vr_gesture.h:88: struct VrGesture { On 2016/09/21 17:43:28, mthiesse wrote: > I don't think this should live in the content layer. Is it even used anywhere in > the content layer? You are right, it used to be included in vr_content_view_core_impl_delegate, but that dependency is removed now.
asimjour@google.com changed reviewers: + tedchoc@chromium.org
asimjour@google.com changed reviewers: + boliu@chromium.org
https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:39: inline void VectSetZero(gvr_vec2f* v) { replace gvr_ with gvr:: types throughout. https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:57: VrController::VrController(gvr_context_* vr_context) { gvr_context* https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:140: void VrController::Initialize(gvr_context_* gvr_context) { gvr_context* https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.h (right): https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:21: explicit VrController(gvr_context_* gvr_context); gvr_context* https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:31: void Initialize(gvr_context_* gvr_context); gvr_context* https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:62: gvr_vec2f position; gvr::Vec2f https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:103: bool InSlop(const gvr_vec2f touch_position); gvr::Vec2f https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:107: int GetGestureListSize() { return static_cast<int>(gesture_list_.size()); } Return a size_t and let the callsite decide if they want to cast it? https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:144: gvr_vec2f overall_velocity_; gvr::Vec2f https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller_manager.cc (right): https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller_manager.cc:33: void VrControllerManager::Initialize(gvr_context_* gvr_context) { gvr_context* https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller_manager.h (right): https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller_manager.h:38: bool IsTouching(); As discussed offline, we should just expose the VrController directly from this class and remove a layer of indirection. https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_gesture.h (right): https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_gesture.h:71: gvr_quatf quat) gvr:: types throughout https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... File content/public/browser/android/vr_input_manager.h (right): https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... content/public/browser/android/vr_input_manager.h:21: // Returns the existing ContentViewCore for |web_contents|, or nullptr. update comment. https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... content/public/browser/android/vr_input_manager.h:22: static VrInputManager* FromContentViewCore( nit: no need for newline?
https://codereview.chromium.org/2350253004/diff/40001/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/2350253004/diff/40001/content/browser/BUILD.g... content/browser/BUILD.gn:1678: "android/vr_input_manager_impl.cc", is VR behind a compile-time flag? https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... File content/public/browser/android/vr_input_manager.h (right): https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... content/public/browser/android/vr_input_manager.h:19: class CONTENT_EXPORT VrInputManager { why do you need *new* entry points to send input events for vr? why can't existing entry points be re-used?
https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... File content/public/browser/android/vr_input_manager.h (right): https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... content/public/browser/android/vr_input_manager.h:19: class CONTENT_EXPORT VrInputManager { On 2016/09/22 16:33:35, boliu wrote: > why do you need *new* entry points to send input events for vr? why can't > existing entry points be re-used? As far as I know there are no existing public entry points for injecting input events from chrome/browser/. Is this not this case? Which existing entry points could we re-use?
https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... File content/public/browser/android/vr_input_manager.h (right): https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... content/public/browser/android/vr_input_manager.h:19: class CONTENT_EXPORT VrInputManager { On 2016/09/22 17:13:08, mthiesse wrote: > On 2016/09/22 16:33:35, boliu wrote: > > why do you need *new* entry points to send input events for vr? why can't > > existing entry points be re-used? > > As far as I know there are no existing public entry points for injecting input > events from chrome/browser/. Is this not this case? Which existing entry points > could we re-use? Oh, I wasn't aware of RenderWidgetHost::ForwardGestureEvent. It appears to only be used in unit tests at the moment? Is this the entry point we should use?
https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... File content/public/browser/android/vr_input_manager.h (right): https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... content/public/browser/android/vr_input_manager.h:19: class CONTENT_EXPORT VrInputManager { On 2016/09/22 17:19:49, mthiesse wrote: > On 2016/09/22 17:13:08, mthiesse wrote: > > On 2016/09/22 16:33:35, boliu wrote: > > > why do you need *new* entry points to send input events for vr? why can't > > > existing entry points be re-used? > > > > As far as I know there are no existing public entry points for injecting input > > events from chrome/browser/. Is this not this case? Which existing entry > points > > could we re-use? > > Oh, I wasn't aware of RenderWidgetHost::ForwardGestureEvent. It appears to only > be used in unit tests at the moment? Is this the entry point we should use? The normal flow goes through ContentViewCore.on*Event. There is a big refactor to remove ContentViewCore though, but the idea is input should be delivered by the "view" abstraction, so for android, the input stuff from ContentViewCore will be moved to ViewAndroid
https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... File content/public/browser/android/vr_input_manager.h (right): https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... content/public/browser/android/vr_input_manager.h:19: class CONTENT_EXPORT VrInputManager { On 2016/09/22 17:22:25, boliu wrote: > On 2016/09/22 17:19:49, mthiesse wrote: > > On 2016/09/22 17:13:08, mthiesse wrote: > > > On 2016/09/22 16:33:35, boliu wrote: > > > > why do you need *new* entry points to send input events for vr? why can't > > > > existing entry points be re-used? > > > > > > As far as I know there are no existing public entry points for injecting > input > > > events from chrome/browser/. Is this not this case? Which existing entry > > points > > > could we re-use? > > > > Oh, I wasn't aware of RenderWidgetHost::ForwardGestureEvent. It appears to > only > > be used in unit tests at the moment? Is this the entry point we should use? > > The normal flow goes through ContentViewCore.on*Event. There is a big refactor > to remove ContentViewCore though, but the idea is input should be delivered by > the "view" abstraction, so for android, the input stuff from ContentViewCore > will be moved to ViewAndroid We can't talk directly to ContentViewCoreImpl from our native code that lives in chrome/browser, and we would really prefer to avoid jni hopping into java to generate a Motion event only to hop back into native code and turn that Motion event we just generated into a gesture event.
https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... File content/public/browser/android/vr_input_manager.h (right): https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... content/public/browser/android/vr_input_manager.h:19: class CONTENT_EXPORT VrInputManager { On 2016/09/22 17:22:25, boliu wrote: > On 2016/09/22 17:19:49, mthiesse wrote: > > On 2016/09/22 17:13:08, mthiesse wrote: > > > On 2016/09/22 16:33:35, boliu wrote: > > > > why do you need *new* entry points to send input events for vr? why can't > > > > existing entry points be re-used? > > > > > > As far as I know there are no existing public entry points for injecting > input > > > events from chrome/browser/. Is this not this case? Which existing entry > > points > > > could we re-use? > > > > Oh, I wasn't aware of RenderWidgetHost::ForwardGestureEvent. It appears to > only > > be used in unit tests at the moment? Is this the entry point we should use? > > The normal flow goes through ContentViewCore.on*Event. There is a big refactor > to remove ContentViewCore though, but the idea is input should be delivered by > the "view" abstraction, so for android, the input stuff from ContentViewCore > will be moved to ViewAndroid we can add the functionality to content_view_core_impl, but the interface is not designed to be used from native. So, we would have to send events from vr_shell native to Java and from ContentViewCore to native again + add new interface to content_view_core_impl. Or we can add the functionality that we need to content_view_core to avoid JNI calls. The first option adds latency, and complexity to ChromeTabbedActivity. The second option is against the big refactoring that you mentioned.
On 2016/09/22 17:34:57, asimjour wrote: > https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... > File content/public/browser/android/vr_input_manager.h (right): > > https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... > content/public/browser/android/vr_input_manager.h:19: class CONTENT_EXPORT > VrInputManager { > On 2016/09/22 17:22:25, boliu wrote: > > On 2016/09/22 17:19:49, mthiesse wrote: > > > On 2016/09/22 17:13:08, mthiesse wrote: > > > > On 2016/09/22 16:33:35, boliu wrote: > > > > > why do you need *new* entry points to send input events for vr? why > can't > > > > > existing entry points be re-used? > > > > > > > > As far as I know there are no existing public entry points for injecting > > input > > > > events from chrome/browser/. Is this not this case? Which existing entry > > > points > > > > could we re-use? > > > > > > Oh, I wasn't aware of RenderWidgetHost::ForwardGestureEvent. It appears to > > only > > > be used in unit tests at the moment? Is this the entry point we should use? > > > > The normal flow goes through ContentViewCore.on*Event. There is a big refactor > > to remove ContentViewCore though, but the idea is input should be delivered by > > the "view" abstraction, so for android, the input stuff from ContentViewCore > > will be moved to ViewAndroid > > we can add the functionality to content_view_core_impl, but the interface is not > designed to be used from native. What do you mean? There is a native ContentViewCore in content/public > So, we would have to send events from vr_shell native to Java and from > ContentViewCore to native again + add new interface to content_view_core_impl. > Or we can add the functionality that we need to content_view_core to avoid JNI > calls. > The first option adds latency, and complexity to ChromeTabbedActivity. The > second option is against the big refactoring that you mentioned. You are welcome to help with the refactor effort: crbug.com/622847. I'm semi-ok with adding methods to native ContentViewCore in the short term, but like you said, that's somewhat going against the refactor. I expect the new entry points to be in native as well, but you'll have to redo object hook ups after input is moved off of ContentViewCore.
On 2016/09/22 17:51:11, boliu wrote: > On 2016/09/22 17:34:57, asimjour wrote: > > > https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... > > File content/public/browser/android/vr_input_manager.h (right): > > > > > https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... > > content/public/browser/android/vr_input_manager.h:19: class CONTENT_EXPORT > > VrInputManager { > > On 2016/09/22 17:22:25, boliu wrote: > > > On 2016/09/22 17:19:49, mthiesse wrote: > > > > On 2016/09/22 17:13:08, mthiesse wrote: > > > > > On 2016/09/22 16:33:35, boliu wrote: > > > > > > why do you need *new* entry points to send input events for vr? why > > can't > > > > > > existing entry points be re-used? > > > > > > > > > > As far as I know there are no existing public entry points for injecting > > > input > > > > > events from chrome/browser/. Is this not this case? Which existing entry > > > > points > > > > > could we re-use? > > > > > > > > Oh, I wasn't aware of RenderWidgetHost::ForwardGestureEvent. It appears to > > > only > > > > be used in unit tests at the moment? Is this the entry point we should > use? > > > > > > The normal flow goes through ContentViewCore.on*Event. There is a big > refactor > > > to remove ContentViewCore though, but the idea is input should be delivered > by > > > the "view" abstraction, so for android, the input stuff from ContentViewCore > > > will be moved to ViewAndroid > > > > we can add the functionality to content_view_core_impl, but the interface is > not > > designed to be used from native. > > What do you mean? There is a native ContentViewCore in content/public > > > So, we would have to send events from vr_shell native to Java and from > > ContentViewCore to native again + add new interface to content_view_core_impl. > > Or we can add the functionality that we need to content_view_core to avoid JNI > > calls. > > The first option adds latency, and complexity to ChromeTabbedActivity. The > > second option is against the big refactoring that you mentioned. > > You are welcome to help with the refactor effort: crbug.com/622847. > > I'm semi-ok with adding methods to native ContentViewCore in the short term, but > like you said, that's somewhat going against the refactor. I expect the new > entry points to be in native as well, but you'll have to redo object hook ups > after input is moved off of ContentViewCore. Is there a good reason not to use RenderWidgetHost::ForwardGestureEvent directly? We're already bypassing the Android View hierarchy, our input events are created natively, not in Java, and I see no reason to make the refactor more difficult by adding native methods to ContentViewCore (this is equivalent to exposing new entry points into the content layer).
On 2016/09/22 17:51:11, boliu wrote: > On 2016/09/22 17:34:57, asimjour wrote: > > > https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... > > File content/public/browser/android/vr_input_manager.h (right): > > > > > https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... > > content/public/browser/android/vr_input_manager.h:19: class CONTENT_EXPORT > > VrInputManager { > > On 2016/09/22 17:22:25, boliu wrote: > > > On 2016/09/22 17:19:49, mthiesse wrote: > > > > On 2016/09/22 17:13:08, mthiesse wrote: > > > > > On 2016/09/22 16:33:35, boliu wrote: > > > > > > why do you need *new* entry points to send input events for vr? why > > can't > > > > > > existing entry points be re-used? > > > > > > > > > > As far as I know there are no existing public entry points for injecting > > > input > > > > > events from chrome/browser/. Is this not this case? Which existing entry > > > > points > > > > > could we re-use? > > > > > > > > Oh, I wasn't aware of RenderWidgetHost::ForwardGestureEvent. It appears to > > > only > > > > be used in unit tests at the moment? Is this the entry point we should > use? > > > > > > The normal flow goes through ContentViewCore.on*Event. There is a big > refactor > > > to remove ContentViewCore though, but the idea is input should be delivered > by > > > the "view" abstraction, so for android, the input stuff from ContentViewCore > > > will be moved to ViewAndroid > > > > we can add the functionality to content_view_core_impl, but the interface is > not > > designed to be used from native. > > What do you mean? There is a native ContentViewCore in content/public > > > So, we would have to send events from vr_shell native to Java and from > > ContentViewCore to native again + add new interface to content_view_core_impl. > > Or we can add the functionality that we need to content_view_core to avoid JNI > > calls. > > The first option adds latency, and complexity to ChromeTabbedActivity. The > > second option is against the big refactoring that you mentioned. > > You are welcome to help with the refactor effort: crbug.com/622847. > > I'm semi-ok with adding methods to native ContentViewCore in the short term, but > like you said, that's somewhat going against the refactor. I expect the new > entry points to be in native as well, but you'll have to redo object hook ups > after input is moved off of ContentViewCore. Is there a good reason not to use RenderWidgetHost::ForwardGestureEvent directly? We're already bypassing the Android View hierarchy, our input events are created natively, not in Java, and I see no reason to make the refactor more difficult by adding native methods to ContentViewCore (this is equivalent to exposing new entry points into the content layer).
On 2016/09/22 18:05:12, mthiesse wrote: > On 2016/09/22 17:51:11, boliu wrote: > > On 2016/09/22 17:34:57, asimjour wrote: > > > > > > https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... > > > File content/public/browser/android/vr_input_manager.h (right): > > > > > > > > > https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... > > > content/public/browser/android/vr_input_manager.h:19: class CONTENT_EXPORT > > > VrInputManager { > > > On 2016/09/22 17:22:25, boliu wrote: > > > > On 2016/09/22 17:19:49, mthiesse wrote: > > > > > On 2016/09/22 17:13:08, mthiesse wrote: > > > > > > On 2016/09/22 16:33:35, boliu wrote: > > > > > > > why do you need *new* entry points to send input events for vr? why > > > can't > > > > > > > existing entry points be re-used? > > > > > > > > > > > > As far as I know there are no existing public entry points for > injecting > > > > input > > > > > > events from chrome/browser/. Is this not this case? Which existing > entry > > > > > points > > > > > > could we re-use? > > > > > > > > > > Oh, I wasn't aware of RenderWidgetHost::ForwardGestureEvent. It appears > to > > > > only > > > > > be used in unit tests at the moment? Is this the entry point we should > > use? > > > > > > > > The normal flow goes through ContentViewCore.on*Event. There is a big > > refactor > > > > to remove ContentViewCore though, but the idea is input should be > delivered > > by > > > > the "view" abstraction, so for android, the input stuff from > ContentViewCore > > > > will be moved to ViewAndroid > > > > > > we can add the functionality to content_view_core_impl, but the interface is > > not > > > designed to be used from native. > > > > What do you mean? There is a native ContentViewCore in content/public > > > > > So, we would have to send events from vr_shell native to Java and from > > > ContentViewCore to native again + add new interface to > content_view_core_impl. > > > Or we can add the functionality that we need to content_view_core to avoid > JNI > > > calls. > > > The first option adds latency, and complexity to ChromeTabbedActivity. The > > > second option is against the big refactoring that you mentioned. > > > > You are welcome to help with the refactor effort: crbug.com/622847. > > > > I'm semi-ok with adding methods to native ContentViewCore in the short term, > but > > like you said, that's somewhat going against the refactor. I expect the new > > entry points to be in native as well, but you'll have to redo object hook ups > > after input is moved off of ContentViewCore. > > Is there a good reason not to use RenderWidgetHost::ForwardGestureEvent > directly? We're already bypassing the Android View hierarchy, our input events > are created natively, not in Java, and I see no reason to make the refactor more > difficult by adding native methods to ContentViewCore (this is equivalent to > exposing new entry points into the content layer). The current call is java CVC -> native CVC -> RenderWidgetHostViewAndroid -> RenderWidgetHost There is a whole bunch of android-specific stuff in RenderWidgetHostViewAndroid that's dealing with input and other things that depend on input, which you'll be skipping if you go directly in RWH.
On 2016/09/22 18:33:44, boliu wrote: > On 2016/09/22 18:05:12, mthiesse wrote: > > On 2016/09/22 17:51:11, boliu wrote: > > > On 2016/09/22 17:34:57, asimjour wrote: > > > > > > > > > > https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... > > > > File content/public/browser/android/vr_input_manager.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2350253004/diff/40001/content/public/browser/... > > > > content/public/browser/android/vr_input_manager.h:19: class CONTENT_EXPORT > > > > VrInputManager { > > > > On 2016/09/22 17:22:25, boliu wrote: > > > > > On 2016/09/22 17:19:49, mthiesse wrote: > > > > > > On 2016/09/22 17:13:08, mthiesse wrote: > > > > > > > On 2016/09/22 16:33:35, boliu wrote: > > > > > > > > why do you need *new* entry points to send input events for vr? > why > > > > can't > > > > > > > > existing entry points be re-used? > > > > > > > > > > > > > > As far as I know there are no existing public entry points for > > injecting > > > > > input > > > > > > > events from chrome/browser/. Is this not this case? Which existing > > entry > > > > > > points > > > > > > > could we re-use? > > > > > > > > > > > > Oh, I wasn't aware of RenderWidgetHost::ForwardGestureEvent. It > appears > > to > > > > > only > > > > > > be used in unit tests at the moment? Is this the entry point we should > > > use? > > > > > > > > > > The normal flow goes through ContentViewCore.on*Event. There is a big > > > refactor > > > > > to remove ContentViewCore though, but the idea is input should be > > delivered > > > by > > > > > the "view" abstraction, so for android, the input stuff from > > ContentViewCore > > > > > will be moved to ViewAndroid > > > > > > > > we can add the functionality to content_view_core_impl, but the interface > is > > > not > > > > designed to be used from native. > > > > > > What do you mean? There is a native ContentViewCore in content/public > > > > > > > So, we would have to send events from vr_shell native to Java and from > > > > ContentViewCore to native again + add new interface to > > content_view_core_impl. > > > > Or we can add the functionality that we need to content_view_core to avoid > > JNI > > > > calls. > > > > The first option adds latency, and complexity to ChromeTabbedActivity. The > > > > second option is against the big refactoring that you mentioned. > > > > > > You are welcome to help with the refactor effort: crbug.com/622847. > > > > > > I'm semi-ok with adding methods to native ContentViewCore in the short term, > > but > > > like you said, that's somewhat going against the refactor. I expect the new > > > entry points to be in native as well, but you'll have to redo object hook > ups > > > after input is moved off of ContentViewCore. > > > > Is there a good reason not to use RenderWidgetHost::ForwardGestureEvent > > directly? We're already bypassing the Android View hierarchy, our input events > > are created natively, not in Java, and I see no reason to make the refactor > more > > difficult by adding native methods to ContentViewCore (this is equivalent to > > exposing new entry points into the content layer). > > The current call is java CVC -> native CVC -> RenderWidgetHostViewAndroid -> > RenderWidgetHost > > There is a whole bunch of android-specific stuff in RenderWidgetHostViewAndroid > that's dealing with input and other things that depend on input, which you'll be > skipping if you go directly in RWH. This seems to me like a feature and not a bug. The experience in VR is much more similar to a desktop or laptop with touchpad/mouse experience; we've been doing lots of work all over the place to disable Android-specific features that don't play well in VR. As far as I could tell the only thing that cares about input events passing through RWHVA is the overscroll manager, which we don't want active in VR anyways as, like I just said, we're much more similar to a touch pad experience. I think if we decide we want the Android specific behaviour we can probably revisit this in the future.
<snip> On 2016/09/22 18:46:24, mthiesse wrote: > This seems to me like a feature and not a bug. I'd like to see some discussion with the input team on that point though. I'm ok if they agree it's fine.
https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:39: inline void VectSetZero(gvr_vec2f* v) { On 2016/09/22 16:20:10, mthiesse wrote: > replace gvr_ with gvr:: types throughout. Done. https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:57: VrController::VrController(gvr_context_* vr_context) { On 2016/09/22 16:20:11, mthiesse wrote: > gvr_context* Done. https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:140: void VrController::Initialize(gvr_context_* gvr_context) { On 2016/09/22 16:20:11, mthiesse wrote: > gvr_context* Done. https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.h (right): https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:21: explicit VrController(gvr_context_* gvr_context); On 2016/09/22 16:20:11, mthiesse wrote: > gvr_context* Done. https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:31: void Initialize(gvr_context_* gvr_context); On 2016/09/22 16:20:11, mthiesse wrote: > gvr_context* Done. https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:62: gvr_vec2f position; On 2016/09/22 16:20:11, mthiesse wrote: > gvr::Vec2f Done. https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:103: bool InSlop(const gvr_vec2f touch_position); On 2016/09/22 16:20:11, mthiesse wrote: > gvr::Vec2f Replaced gvr_vec2f with gvr::Vec2f in this file and .cc file https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:107: int GetGestureListSize() { return static_cast<int>(gesture_list_.size()); } On 2016/09/22 16:20:11, mthiesse wrote: > Return a size_t and let the callsite decide if they want to cast it? Done. https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:144: gvr_vec2f overall_velocity_; On 2016/09/22 16:20:11, mthiesse wrote: > gvr::Vec2f Done. https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller_manager.cc (right): https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller_manager.cc:33: void VrControllerManager::Initialize(gvr_context_* gvr_context) { On 2016/09/22 16:20:11, mthiesse wrote: > gvr_context* Done. https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller_manager.h (right): https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller_manager.h:38: bool IsTouching(); On 2016/09/22 16:20:11, mthiesse wrote: > As discussed offline, we should just expose the VrController directly from this > class and remove a layer of indirection. Done. https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_gesture.h (right): https://codereview.chromium.org/2350253004/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_gesture.h:71: gvr_quatf quat) On 2016/09/22 16:20:11, mthiesse wrote: > gvr:: types throughout Done.
a few more nits. it is good that we can keep everything in vr_shell https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:98: bool VrController::ButtonDown(const int32_t button) { nit: IsButtonDown? https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:102: bool VrController::ButtonUp(const int32_t button) { nit: IsButtonUp https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:114: controller_state_.Update(*controller_api_); If controller is not connected, what would Update do? https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:122: return; nit: unnecessary return https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.h (right): https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:55: enum gesture_detector_state { nit: use CamelCase https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:74: gvr_controller_button button; prefer gvr::ControllerButton https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:100: // Returns true if the touch position is with in the slop of the initial touch nit: within https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:144: gvr::Vec2f overall_velocity_; nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller_manager.cc (right): https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller_manager.cc:28: if (!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) nit: use {} if code is more than 1 line. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller_manager.cc:38: if (!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) ditto https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller_manager.cc:65: return; nit: remove return https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller_manager.h (right): https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller_manager.h:33: void ProcessUpdatedGesture(VrGesture gesture); nit: empty line before comment https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller_manager.h:47: VrInputManager* vr_content_ptr; nit: s/vr_content_ptr/vr_content_ptr_ https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller_manager.h:48: VrInputManager* vr_ui_ptr; ditto https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller_manager.h:49: std::unique_ptr<gvr::ControllerApi> controller_api_; nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_gesture.h (right): https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_gesture.h:9: nit: put them into vr_shell namespace? https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_input_manager.h (right): https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_input_manager.h:54: float dpi_scale() const { return dpi_scale_; } nit: no need for private accessor. you can directly use dpi_scale_
did input folks say skipping RWHVA is ok? RWHVA has code that sends a speculative begin frame on input to reduce latency of first frame after input. That's probably important for vr..? https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_input_manager.cc (right): https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_input_manager.cc:14: VrInputManager::VrInputManager(content::ContentViewCore* content_view_core) suggestion: build on to of WebContents, since CVC is intended to go away soon
I'm probably a pretty picky reviewer. If you don't want to have all this discussion here and what wants to forge ahead and get things working first, then just remove me from this code review. I don't own anything from chrome :p
On 2016/09/22 23:25:37, boliu wrote: > RWHVA has code that sends a speculative begin frame on input to reduce latency > of first frame after input. That's probably important for vr..? That's actually not important for VR, for two reasons. Firstly, we care much less about scroll latency than on a touch device because there's no feeling of the content not 'sticking to your finger', the controllers are handheld (and we want to minimize as much as possible the work done for the content window so we can keep the UI updating at 60fps). Secondly, we're actually in the process of decoupling our compositing from VSync in order to coordinate better with the new single-buffered rendering path on VR android devices. It's also very likely that we don't want the FilteredGestureProvider processing our input events, as we're performing our own gesture detection more suitable for the input device we have. Also also, I'm not sure we want to pollute the touch latency histogram data with VR inputs...
boliu@chromium.org changed reviewers: - boliu@chromium.org
fair -me then
tdresser@chromium.org changed reviewers: + tdresser@chromium.org
mthiesse's arguments SGTM. A few high level concerns: Why are we introducing a new set of gesture types, instead of using the existing ones? Why does VR have a different 2 vector type than the rest of Chrome? https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:19: constexpr float kDisplacementScaleFactor = 800.0f; Add comment explaining what this is. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:29: constexpr float kSlopHorizontal = 0.125f; Why are vertical and horizontal not equal? Why use a rectangle instead of a circle? https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:30: constexpr float kDelta = 1.0e-7f; What is this? https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:32: constexpr float kMinZoomAngle = 0.25f; What is this? https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:53: } All this code would go away if we used Vector2dF, wouldn't it? https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:84: } Why doesn't this return a Vector2dF? https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:163: return VrGesture( If I understand correctly, we perform gesture detection to come up with some WebInputEvents, and then we immediately turn them into VrGestures? Why don't we use WebInputEvents throughout? Barring that, why not just construct VrGestures immediately? https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.h (right): https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:63: int64_t timestamp; Can we store time in a base::TimeTicks (throughout the patch)? We've had lots of issues with mixing timebases elsewhere. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:70: bool is_touching; Should this be an enum instead of a bunch of bools? It looks like this could hold an invalid state. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:101: // point, nit: remove newline. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:127: float last_qz_; Can we use a Vector2dF? https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:144: gvr::Vec2f overall_velocity_; On 2016/09/22 23:16:49, bshe wrote: > nit: DISALLOW_COPY_AND_ASSIGN I'm out of the loop on VR things. Why don't we use Vector2dF here? https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_input_manager.cc (right): https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_input_manager.cc:182: int64_t time_ms, We're using long in some places, and int64_t in some places. Why?
https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:19: constexpr float kDisplacementScaleFactor = 800.0f; On 2016/09/23 13:48:18, tdresser (OOO Sept 26-28) wrote: > Add comment explaining what this is. This is a temporary scaling factor that should be replaced later on when we have access to the details of the touchpad. This scaling is usually is done by Operating system, but in this case OS only reports raw data. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:29: constexpr float kSlopHorizontal = 0.125f; On 2016/09/23 13:48:18, tdresser (OOO Sept 26-28) wrote: > Why are vertical and horizontal not equal? Why use a rectangle instead of a > circle? The constants are picked by UX team based on user study. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:30: constexpr float kDelta = 1.0e-7f; On 2016/09/23 13:48:19, tdresser (OOO Sept 26-28) wrote: > What is this? Two vectors with this distance are considered to be equal. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:32: constexpr float kMinZoomAngle = 0.25f; On 2016/09/23 13:48:18, tdresser (OOO Sept 26-28) wrote: > What is this? Minimum angle that required for zoom gesture. It is about the orientation of the controller, and it's not related to touch events. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:98: bool VrController::ButtonDown(const int32_t button) { On 2016/09/22 23:16:48, bshe wrote: > nit: IsButtonDown? Done. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:102: bool VrController::ButtonUp(const int32_t button) { On 2016/09/22 23:16:49, bshe wrote: > nit: IsButtonUp Done. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:114: controller_state_.Update(*controller_api_); On 2016/09/22 23:16:48, bshe wrote: > If controller is not connected, what would Update do? UpdateState only updates the state of the controller regarding to the connectivity. If the controller is not connected, the controller_state_.GetConnectionState() will indicate that, so the controller won't generate any gestures. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:122: return; On 2016/09/22 23:16:49, bshe wrote: > nit: unnecessary return Done. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:163: return VrGesture( On 2016/09/23 13:48:18, tdresser (OOO Sept 26-28) wrote: > If I understand correctly, we perform gesture detection to come up with some > WebInputEvents, and then we immediately turn them into VrGestures? > > Why don't we use WebInputEvents throughout? > > Barring that, why not just construct VrGestures immediately? Our gestures are not limited to the gestures that are supported by WebInputEvents. We have to use VrGesture to be able to expand the gesture set. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.h (right): https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:55: enum gesture_detector_state { On 2016/09/22 23:16:49, bshe wrote: > nit: use CamelCase Done. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:63: int64_t timestamp; On 2016/09/23 13:48:19, tdresser (OOO Sept 26-28) wrote: > Can we store time in a base::TimeTicks (throughout the patch)? We've had lots of > issues with mixing timebases elsewhere. Would it be fine if we switch to TimeTicks in another CL? There are some concerns about the controller events that we need to talk + I think vr_shell also uses the same timestamp and we can fix controller/vr_shell in the same patch. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:70: bool is_touching; On 2016/09/23 13:48:19, tdresser (OOO Sept 26-28) wrote: > Should this be an enum instead of a bunch of bools? It looks like this could > hold an invalid state. I rather to keep it as is to be consistence with gesture detection. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:74: gvr_controller_button button; On 2016/09/22 23:16:49, bshe wrote: > prefer gvr::ControllerButton Done. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:100: // Returns true if the touch position is with in the slop of the initial touch On 2016/09/22 23:16:49, bshe wrote: > nit: within Done. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:101: // point, On 2016/09/23 13:48:19, tdresser (OOO Sept 26-28) wrote: > nit: remove newline. Done. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:127: float last_qz_; On 2016/09/23 13:48:19, tdresser (OOO Sept 26-28) wrote: > Can we use a Vector2dF? removed last_qz_ https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:144: gvr::Vec2f overall_velocity_; On 2016/09/22 23:16:49, bshe wrote: > nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:144: gvr::Vec2f overall_velocity_; On 2016/09/22 23:16:49, bshe wrote: > nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller_manager.cc (right): https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller_manager.cc:28: if (!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) On 2016/09/22 23:16:49, bshe wrote: > nit: use {} if code is more than 1 line. Done. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller_manager.cc:38: if (!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) On 2016/09/22 23:16:49, bshe wrote: > ditto Done. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller_manager.cc:65: return; On 2016/09/22 23:16:49, bshe wrote: > nit: remove return Done. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller_manager.h (right): https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller_manager.h:33: void ProcessUpdatedGesture(VrGesture gesture); On 2016/09/22 23:16:49, bshe wrote: > nit: empty line before comment Done. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller_manager.h:47: VrInputManager* vr_content_ptr; On 2016/09/22 23:16:49, bshe wrote: > nit: s/vr_content_ptr/vr_content_ptr_ Done. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller_manager.h:48: VrInputManager* vr_ui_ptr; On 2016/09/22 23:16:49, bshe wrote: > ditto Done. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller_manager.h:49: std::unique_ptr<gvr::ControllerApi> controller_api_; On 2016/09/22 23:16:49, bshe wrote: > nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_gesture.h (right): https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_gesture.h:9: On 2016/09/22 23:16:49, bshe wrote: > nit: put them into vr_shell namespace? Done. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_input_manager.cc (right): https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_input_manager.cc:14: VrInputManager::VrInputManager(content::ContentViewCore* content_view_core) On 2016/09/22 23:25:36, boliu wrote: > suggestion: build on to of WebContents, since CVC is intended to go away soon Done. https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_input_manager.cc:182: int64_t time_ms, On 2016/09/23 13:48:19, tdresser (OOO Sept 26-28) wrote: > We're using long in some places, and int64_t in some places. Why? replaced long with int64_t https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_input_manager.h (right): https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_input_manager.h:54: float dpi_scale() const { return dpi_scale_; } On 2016/09/22 23:16:49, bshe wrote: > nit: no need for private accessor. you can directly use dpi_scale_ Done.
https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:163: return VrGesture( On 2016/09/23 16:58:20, asimjour wrote: > On 2016/09/23 13:48:18, tdresser (OOO Sept 26-28) wrote: > > If I understand correctly, we perform gesture detection to come up with some > > WebInputEvents, and then we immediately turn them into VrGestures? > > > > Why don't we use WebInputEvents throughout? > > > > Barring that, why not just construct VrGestures immediately? > > Our gestures are not limited to the gestures that are supported by > WebInputEvents. We have to use VrGesture to be able to expand the gesture set. So why not construct VREvents immediately instead of temporarily using WebInputEvents? https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.h (right): https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:63: int64_t timestamp; On 2016/09/23 16:58:21, asimjour wrote: > On 2016/09/23 13:48:19, tdresser (OOO Sept 26-28) wrote: > > Can we store time in a base::TimeTicks (throughout the patch)? We've had lots > of > > issues with mixing timebases elsewhere. > > Would it be fine if we switch to TimeTicks in another CL? There are some > concerns about the controller events that we need to talk + I think vr_shell > also uses the same timestamp and we can fix controller/vr_shell in the same > patch. SGTM - maybe add a TODO? https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.h:70: bool is_touching; On 2016/09/23 16:58:20, asimjour wrote: > On 2016/09/23 13:48:19, tdresser (OOO Sept 26-28) wrote: > > Should this be an enum instead of a bunch of bools? It looks like this could > > hold an invalid state. > > I rather to keep it as is to be consistence with gesture detection. Ideally I think we'd change it in both places, but this is fine for now. https://codereview.chromium.org/2350253004/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2350253004/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:57: } Can these be methods on the vector class? https://codereview.chromium.org/2350253004/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_gesture.h (right): https://codereview.chromium.org/2350253004/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_gesture.h:24: } GestureAngularMove; Throughout this file, it looks like we should be using 2 vectors and 3 vectors, instead of a pile of floats.
mthiesse@chromium.org changed reviewers: - tedchoc@chromium.org
https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:163: return VrGesture( On 2016/09/23 17:45:17, tdresser (OOO Sept 26-28) wrote: > On 2016/09/23 16:58:20, asimjour wrote: > > On 2016/09/23 13:48:18, tdresser (OOO Sept 26-28) wrote: > > > If I understand correctly, we perform gesture detection to come up with some > > > WebInputEvents, and then we immediately turn them into VrGestures? > > > > > > Why don't we use WebInputEvents throughout? > > > > > > Barring that, why not just construct VrGestures immediately? > > > > Our gestures are not limited to the gestures that are supported by > > WebInputEvents. We have to use VrGesture to be able to expand the gesture set. > > So why not construct VREvents immediately instead of temporarily using > WebInputEvents? Only constants regarding the state of the gesture are borrowed from WebInputEvent to avoid introducing similar constant twice. For VrGestures that are VR specific these constant are not needed. So we define a new set of constant, it will be identical to WebInputEvents. However, it means if we remove the first argument and don't use VrGesture, we have to add our additional gestures to WebInputEvent, which is not possible. https://codereview.chromium.org/2350253004/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2350253004/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:57: } On 2016/09/23 17:45:17, tdresser (OOO Sept 26-28) wrote: > Can these be methods on the vector class? Done. https://codereview.chromium.org/2350253004/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_gesture.h (right): https://codereview.chromium.org/2350253004/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_gesture.h:24: } GestureAngularMove; On 2016/09/23 17:45:17, tdresser (OOO Sept 26-28) wrote: > Throughout this file, it looks like we should be using 2 vectors and 3 vectors, > instead of a pile of floats. Done.
https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:163: return VrGesture( On 2016/09/23 19:56:31, asimjour wrote: > On 2016/09/23 17:45:17, tdresser (OOO Sept 26-28) wrote: > > On 2016/09/23 16:58:20, asimjour wrote: > > > On 2016/09/23 13:48:18, tdresser (OOO Sept 26-28) wrote: > > > > If I understand correctly, we perform gesture detection to come up with > some > > > > WebInputEvents, and then we immediately turn them into VrGestures? > > > > > > > > Why don't we use WebInputEvents throughout? > > > > > > > > Barring that, why not just construct VrGestures immediately? > > > > > > Our gestures are not limited to the gestures that are supported by > > > WebInputEvents. We have to use VrGesture to be able to expand the gesture > set. > > > > So why not construct VREvents immediately instead of temporarily using > > WebInputEvents? > > Only constants regarding the state of the gesture are borrowed from > WebInputEvent to avoid introducing similar constant twice. > For VrGestures that are VR specific these constant are not needed. So we define > a new set of constant, it will be identical to WebInputEvents. > However, it means if we remove the first argument and don't use VrGesture, we > have to add our additional gestures to WebInputEvent, which is not possible. Which constants? Just the types? Don't we already have copies of the types? https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:242: // user has not put finger on touch pad. Capitalize comments. https://codereview.chromium.org/2350253004/diff/100001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2350253004/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_controller.cc:41: static inline void ClampTouchpadPosition(gvr::Vec2f* position) { We can't modify Vec2f? Where is it defined? https://codereview.chromium.org/2350253004/diff/100001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_controller_manager.cc (right): https://codereview.chromium.org/2350253004/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_controller_manager.cc:51: int64_t event_time_milliseconds = (int64_t)(event_time / 1000000); Use static_cast<int64_t>()
On 2016/09/23 20:36:44, tdresser (OOO Sept 26-28) wrote: > https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... > File chrome/browser/android/vr_shell/vr_controller.cc (right): > > https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... > chrome/browser/android/vr_shell/vr_controller.cc:163: return VrGesture( > On 2016/09/23 19:56:31, asimjour wrote: > > On 2016/09/23 17:45:17, tdresser (OOO Sept 26-28) wrote: > > > On 2016/09/23 16:58:20, asimjour wrote: > > > > On 2016/09/23 13:48:18, tdresser (OOO Sept 26-28) wrote: > > > > > If I understand correctly, we perform gesture detection to come up with > > some > > > > > WebInputEvents, and then we immediately turn them into VrGestures? > > > > > > > > > > Why don't we use WebInputEvents throughout? > > > > > > > > > > Barring that, why not just construct VrGestures immediately? > > > > > > > > Our gestures are not limited to the gestures that are supported by > > > > WebInputEvents. We have to use VrGesture to be able to expand the gesture > > set. > > > > > > So why not construct VREvents immediately instead of temporarily using > > > WebInputEvents? > > > > Only constants regarding the state of the gesture are borrowed from > > WebInputEvent to avoid introducing similar constant twice. > > For VrGestures that are VR specific these constant are not needed. So we > define > > a new set of constant, it will be identical to WebInputEvents. > > However, it means if we remove the first argument and don't use VrGesture, we > > have to add our additional gestures to WebInputEvent, which is not possible. > > Which constants? Just the types? Don't we already have copies of the types? > I'm not sure if I understand what you mean. Do you want me to use a new enum instead of WebInputEvents in VrGestures? We don't convert the WebInputEvents to VrGestures. VrGestureScroll has WebInputEvents for the state of the scrolling (start, update, and end) the same thing happens for Zoom with three states. but other gestures (like moving the window) won't have anything to do with WebInputEvents. Scrolling and Zoom use WebInputEvents, and they never convert it to anything else. > https://codereview.chromium.org/2350253004/diff/60001/chrome/browser/android/... > chrome/browser/android/vr_shell/vr_controller.cc:242: // user has not put finger > on touch pad. > Capitalize comments. > Done > https://codereview.chromium.org/2350253004/diff/100001/chrome/browser/android... > File chrome/browser/android/vr_shell/vr_controller.cc (right): > > https://codereview.chromium.org/2350253004/diff/100001/chrome/browser/android... > chrome/browser/android/vr_shell/vr_controller.cc:41: static inline void > ClampTouchpadPosition(gvr::Vec2f* position) { > We can't modify Vec2f? Where is it defined? > We can't. It's in gvr library, (third_party/gvr-android-sdk/) > https://codereview.chromium.org/2350253004/diff/100001/chrome/browser/android... > File chrome/browser/android/vr_shell/vr_controller_manager.cc (right): > > https://codereview.chromium.org/2350253004/diff/100001/chrome/browser/android... > chrome/browser/android/vr_shell/vr_controller_manager.cc:51: int64_t > event_time_milliseconds = (int64_t)(event_time / 1000000); > Use static_cast<int64_t>() Done
lgtm
https://codereview.chromium.org/2350253004/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_controller_manager.h (right): https://codereview.chromium.org/2350253004/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_controller_manager.h:32: void ProcessUpdatedGesture(VrGesture gesture); ProcessUpdatedContentGesture? https://codereview.chromium.org/2350253004/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_gesture.h (right): https://codereview.chromium.org/2350253004/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_gesture.h:49: kGestureTypeNull, I discussed this with Tim at the offsite, and we think we should just be using the WebInputEvent gesture types throughout. AngularMove maps to MouseMove, we can use mousedown/up with modifiers for the buttons, and the rest map easily.
https://codereview.chromium.org/2350253004/diff/100001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2350253004/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_controller.cc:41: static inline void ClampTouchpadPosition(gvr::Vec2f* position) { On 2016/09/23 20:36:44, tdresser (OOO Sept 26-28) wrote: > We can't modify Vec2f? Where is it defined? We can't. It's in gvr library (third_party/gvr-android-sdk/) https://codereview.chromium.org/2350253004/diff/100001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_controller_manager.cc (right): https://codereview.chromium.org/2350253004/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_controller_manager.cc:51: int64_t event_time_milliseconds = (int64_t)(event_time / 1000000); On 2016/09/23 20:36:44, tdresser (OOO Sept 26-28) wrote: > Use static_cast<int64_t>() Done. https://codereview.chromium.org/2350253004/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_controller_manager.h (right): https://codereview.chromium.org/2350253004/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_controller_manager.h:32: void ProcessUpdatedGesture(VrGesture gesture); On 2016/09/29 14:46:59, mthiesse wrote: > ProcessUpdatedContentGesture? Done.
lgtm with nits https://codereview.chromium.org/2350253004/diff/140001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2350253004/diff/140001/chrome/browser/android... chrome/browser/android/vr_shell/vr_controller.cc:7: #include <android/log.h> remove this https://codereview.chromium.org/2350253004/diff/140001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_controller.h (right): https://codereview.chromium.org/2350253004/diff/140001/chrome/browser/android... chrome/browser/android/vr_shell/vr_controller.h:11: #include "content/public/browser/android/content_view_core.h" I think you can forward declare #include "base/task_runner_util.h" #include "chrome/browser/android/vr_shell/vr_gesture.h" #include "content/public/browser/android/content_view_core.h" https://codereview.chromium.org/2350253004/diff/140001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_gesture.h (right): https://codereview.chromium.org/2350253004/diff/140001/chrome/browser/android... chrome/browser/android/vr_shell/vr_gesture.h:96: const GestureScroll kGestureScroll = {{0, 0}, 0, {0, 0}}; Move these to the .cc file where they're used? Also a name like kDefaultGestureScroll would be less confusing?
https://codereview.chromium.org/2350253004/diff/140001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_gesture.h (right): https://codereview.chromium.org/2350253004/diff/140001/chrome/browser/android... chrome/browser/android/vr_shell/vr_gesture.h:75: type(WebInputEvent::GestureTap), One more thing: Do MouseDown for the touch pad and KeyDown for the app button make more sense?
https://codereview.chromium.org/2350253004/diff/140001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2350253004/diff/140001/chrome/browser/android... chrome/browser/android/vr_shell/vr_controller.cc:7: #include <android/log.h> On 2016/09/30 14:09:36, mthiesse wrote: > remove this Done. https://codereview.chromium.org/2350253004/diff/140001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_controller.h (right): https://codereview.chromium.org/2350253004/diff/140001/chrome/browser/android... chrome/browser/android/vr_shell/vr_controller.h:11: #include "content/public/browser/android/content_view_core.h" On 2016/09/30 14:09:36, mthiesse wrote: > I think you can forward declare > #include "base/task_runner_util.h" > #include "chrome/browser/android/vr_shell/vr_gesture.h" > #include "content/public/browser/android/content_view_core.h" Removed extra #include. Only vr_gesture.h is remained which looks to be ok to be included. https://codereview.chromium.org/2350253004/diff/140001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_gesture.h (right): https://codereview.chromium.org/2350253004/diff/140001/chrome/browser/android... chrome/browser/android/vr_shell/vr_gesture.h:75: type(WebInputEvent::GestureTap), On 2016/09/30 14:14:31, mthiesse wrote: > One more thing: Do MouseDown for the touch pad and KeyDown for the app button > make more sense? App button is not a part of the touchpad, so we generate any Gesture for that here. https://codereview.chromium.org/2350253004/diff/140001/chrome/browser/android... chrome/browser/android/vr_shell/vr_gesture.h:96: const GestureScroll kGestureScroll = {{0, 0}, 0, {0, 0}}; On 2016/09/30 14:09:36, mthiesse wrote: > Move these to the .cc file where they're used? > > Also a name like kDefaultGestureScroll would be less confusing? Done
I'm okay with landing this to get it into M55, but let's look into getting rid of the VrGesture type. Can you file a bug for this? LGTM, assuming we clean this up later.
The CQ bit was checked by asimjour@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bshe@chromium.org, mthiesse@chromium.org Link to the patchset: https://codereview.chromium.org/2350253004/#ps160001 (title: "Clean up")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Controller support for VrShell VrControllerManager has an instance of the VrController which detects the scroll and click gestures. VrControllerManager also is responsible for sending the generated events to the UI Thread. BUG=641461 ========== to ========== Controller support for VrShell VrControllerManager has an instance of the VrController which detects the scroll and click gestures. VrControllerManager also is responsible for sending the generated events to the UI Thread. BUG=641461 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Controller support for VrShell VrControllerManager has an instance of the VrController which detects the scroll and click gestures. VrControllerManager also is responsible for sending the generated events to the UI Thread. BUG=641461 ========== to ========== Controller support for VrShell VrControllerManager has an instance of the VrController which detects the scroll and click gestures. VrControllerManager also is responsible for sending the generated events to the UI Thread. BUG=641461 Committed: https://crrev.com/230e7a29d98a166df6de4b9518e631dff553a739 Cr-Commit-Position: refs/heads/master@{#422179} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/230e7a29d98a166df6de4b9518e631dff553a739 Cr-Commit-Position: refs/heads/master@{#422179} |
