Implement controller handling in vr_shell.cc

Implement controller handling in vr_shell.cc

BUG=641413
Committed: https://crrev.com/e6ccc9717dedbd3573efa6c8aa25f20db942510d
Cr-Commit-Position: refs/heads/master@{#422783}

[mthiesse, 2016-09-30 20:07:01]

mthiesse@chromium.org changed reviewers:
+ asimjour@chromium.org, bshe@chromium.org, cjgrant@chromium.org

[mthiesse, 2016-09-30 20:07:02]

PTAL

[cjgrant, 2016-09-30 20:26:39]

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:12: #include
"chrome/browser/android/vr_shell/vr_controller.h"
Duplicate include.

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:228: controller_active_ =
controller_->IsConnected();
Rebase needed.

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:298: gesture->details.move.type = 2;
Type should be an enum.

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:299: if (current_ui_rect_ == 0) {
kBrowserUiElementId

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:312: gesture->details.move.type = 1;
Enum here as well, and elsewhere.

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:313: if (closest_element_index > 0)
{
!= kBrowserUiElementId, here and elsewhere.

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.h (right):

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.h:153: int current_ui_rect_ = -1;
nit: In ToT, this would be at home alongside target_point_ and related.

[bshe, 2016-10-03 12:23:21]

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:220: // TODO (asimjour) for now,
scroll is sent to the main content.
nit: s/TODO (asimjour)/TODO(asimjour):

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:262: float closest_element =
std::numeric_limits<float>::infinity();
nit: s/closest_element/closest_element_distance ?

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:304: current_ui_rect_ = 1;
why 1?

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:330:
ui_input_manager_->ProcessUpdatedGesture(*gesture.get());
it looks like line 320 - 330 is very similar to line 332 to 342. Perhaps create
a private function to reduce the code size?

[asimjour1, 2016-10-03 15:54:39]

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:228: controller_active_ =
controller_->IsConnected();
It looks that controller_active_ is not needed. you can replace it with
controller_->IsConnected();

[mthiesse, 2016-10-03 16:30:13]

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:12: #include
"chrome/browser/android/vr_shell/vr_controller.h"
On 2016/09/30 20:26:39, cjgrant wrote:
> Duplicate include.

Done.

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:220: // TODO (asimjour) for now,
scroll is sent to the main content.
On 2016/10/03 12:23:21, bshe wrote:
> nit: s/TODO (asimjour)/TODO(asimjour): 

Done.

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:228: controller_active_ =
controller_->IsConnected();
On 2016/09/30 20:26:39, cjgrant wrote:
> Rebase needed.

Acknowledged.

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:228: controller_active_ =
controller_->IsConnected();
On 2016/10/03 15:54:38, asimjour1 wrote:
> It looks that controller_active_ is not needed. you can replace it with
> controller_->IsConnected();

Done.

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:262: float closest_element =
std::numeric_limits<float>::infinity();
On 2016/10/03 12:23:21, bshe wrote:
> nit: s/closest_element/closest_element_distance ?

Done.

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:298: gesture->details.move.type = 2;
On 2016/09/30 20:26:39, cjgrant wrote:
> Type should be an enum.

Done.

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:299: if (current_ui_rect_ == 0) {
On 2016/09/30 20:26:39, cjgrant wrote:
> kBrowserUiElementId

Done.

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:304: current_ui_rect_ = 1;
On 2016/10/03 12:23:21, bshe wrote:
> why 1?

Removed.

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:312: gesture->details.move.type = 1;
On 2016/09/30 20:26:39, cjgrant wrote:
> Enum here as well, and elsewhere.

Done.

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:313: if (closest_element_index > 0)
{
On 2016/09/30 20:26:39, cjgrant wrote:
> != kBrowserUiElementId, here and elsewhere.

Done.

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:330:
ui_input_manager_->ProcessUpdatedGesture(*gesture.get());
On 2016/10/03 12:23:21, bshe wrote:
> it looks like line 320 - 330 is very similar to line 332 to 342. Perhaps
create
> a private function to reduce the code size?

Done.

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.h (right):

https://codereview.chromium.org/2378903005/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.h:153: int current_ui_rect_ = -1;
On 2016/09/30 20:26:39, cjgrant wrote:
> nit: In ToT, this would be at home alongside target_point_ and related.

Done.

[asimjour1, 2016-10-04 13:30:07]

lgtm

https://codereview.chromium.org/2378903005/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2378903005/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:308: gesture->details.move.delta.x =
pixel_x;
On the experimental branch we have 
gesture->details.move.delta.x = ui_tex_width_ * ratio_x
if pointer is on the main content. I assume that you checked and this works.
right?

https://codereview.chromium.org/2378903005/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:314: gesture->details.buttons.pos.x
= pixel_x;
same as above

[bshe, 2016-10-04 13:50:42]

lgtm with nit

Also, please refer to a bug in the CL description.

https://codereview.chromium.org/2378903005/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_input_manager.cc (right):

https://codereview.chromium.org/2378903005/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_input_manager.cc:124: void
VrInputManager::SendMouseMoveEvent(int64_t time_ms,
Nit: perhap rename to SendMouseEvent since this is not strictly for MouseMove
events

[mthiesse, 2016-10-04 14:05:26]

https://codereview.chromium.org/2378903005/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_input_manager.cc (right):

https://codereview.chromium.org/2378903005/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_input_manager.cc:124: void
VrInputManager::SendMouseMoveEvent(int64_t time_ms,
On 2016/10/04 13:50:42, bshe wrote:
> Nit: perhap rename to SendMouseEvent since this is not strictly for MouseMove
> events

Done.

https://codereview.chromium.org/2378903005/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2378903005/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:308: gesture->details.move.delta.x =
pixel_x;
On 2016/10/04 13:30:07, asimjour1 wrote:
> On the experimental branch we have 
> gesture->details.move.delta.x = ui_tex_width_ * ratio_x
> if pointer is on the main content. I assume that you checked and this works.
> right?

It actually doesn't work yet, because the copy rect for the main content window
isn't set correctly. This will be fixed in a followup patch.

It works correctly for UI elements.

https://codereview.chromium.org/2378903005/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:314: gesture->details.buttons.pos.x
= pixel_x;
On 2016/10/04 13:30:07, asimjour1 wrote:
> same as above

Acknowledged.

[mthiesse, 2016-10-04 14:05:31]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2016-10-04 14:05:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
asimjour@chromium.org, bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/2378903005/#ps60001
(title: "Address comments")

[commit-bot: I haz the power, 2016-10-04 14:05:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mthiesse, 2016-10-04 14:08:57]

Description was changed from

==========
Implement controller handling in vr_shell.cc

BUG=
==========

to

==========
Implement controller handling in vr_shell.cc

BUG=641413
==========

[commit-bot: I haz the power, 2016-10-04 14:17:33]

Description was changed from

==========
Implement controller handling in vr_shell.cc

BUG=641413
==========

to

==========
Implement controller handling in vr_shell.cc

BUG=641413
==========

[commit-bot: I haz the power, 2016-10-04 14:17:34]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-04 14:20:05]

Description was changed from

==========
Implement controller handling in vr_shell.cc

BUG=641413
==========

to

==========
Implement controller handling in vr_shell.cc

BUG=641413

Committed: https://crrev.com/e6ccc9717dedbd3573efa6c8aa25f20db942510d
Cr-Commit-Position: refs/heads/master@{#422783}
==========

[commit-bot: I haz the power, 2016-10-04 14:20:06]

Patchset 4 (id:??) landed as
https://crrev.com/e6ccc9717dedbd3573efa6c8aa25f20db942510d
Cr-Commit-Position: refs/heads/master@{#422783}