Refactorings to vr controller code.

Refactorings to vr controller code.

BUG=

Committed: https://crrev.com/8e065f3d69062bb80d4c99be75e4d46c64bcaf2e
Cr-Commit-Position: refs/heads/master@{#422480}

[mthiesse, 2016-09-30 19:55:16]

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

[mthiesse, 2016-09-30 19:55:17]

PTAL, tested on experimental branch with controller.

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

Amir is more familiar with this code than I do. I will defer to his approval and
then rubber stamp.

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

https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_controller.cc:238: if (touch_info_->touch_up
|| !(touch_info_->is_touching)) {  // gesture ends
nit: move comments to a new line

https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_controller.cc:245: } else if
(touch_position_changed) {  // User continues scrolling and there is
ditto

[cjgrant, 2016-10-03 14:27:33]

cjgrant@chromium.org changed reviewers:
+ cjgrant@chromium.org

[cjgrant, 2016-10-03 14:27:34]

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

https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_controller.cc:154: gesture_.reset(new
VrGesture());
Maybe here, or in a future change, we should cut out some use of this class
member gesture_.  It looks like we set it here, but then pluck it out to return.
 Could just pass the gesture around on a per-frame basis instead, maybe.

[asimjour, 2016-10-03 14:40:16]

asimjour@google.com changed reviewers:
+ asimjour@google.com

[asimjour, 2016-10-03 14:40:17]

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

https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_controller.h:118: std::unique_ptr<VrGesture>
gesture_;
We should be able to send multiple gestures. For example ScrollEnd + Tap

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

https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_input_manager.h:25: void
SendGesture(VrGesture gesture);
SendGesture doesn't need to be public.

[mthiesse, 2016-10-03 15:39:15]

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

https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_controller.cc:154: gesture_.reset(new
VrGesture());
On 2016/10/03 14:27:34, cjgrant wrote:
> Maybe here, or in a future change, we should cut out some use of this class
> member gesture_.  It looks like we set it here, but then pluck it out to
return.
>  Could just pass the gesture around on a per-frame basis instead, maybe.

Done.

https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_controller.cc:238: if (touch_info_->touch_up
|| !(touch_info_->is_touching)) {  // gesture ends
On 2016/10/03 12:09:23, bshe wrote:
> nit: move comments to a new line

Done.

https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_controller.cc:245: } else if
(touch_position_changed) {  // User continues scrolling and there is
On 2016/10/03 12:09:23, bshe wrote:
> ditto

Done.

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

https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_controller.h:118: std::unique_ptr<VrGesture>
gesture_;
On 2016/10/03 14:40:16, asimjour wrote:
> We should be able to send multiple gestures. For example ScrollEnd + Tap

Sure, but right now the API only supports returning one, and we don't generate
more than one. We can add complexity as needed.

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

https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_input_manager.h:25: void
SendGesture(VrGesture gesture);
On 2016/10/03 14:40:16, asimjour wrote:
> SendGesture doesn't need to be public.

Done.

[asimjour1, 2016-10-03 15:46:12]

On 2016/10/03 15:39:15, mthiesse wrote:
>
https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
> File chrome/browser/android/vr_shell/vr_controller.cc (right):
> 
>
https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
> chrome/browser/android/vr_shell/vr_controller.cc:154: gesture_.reset(new
> VrGesture());
> On 2016/10/03 14:27:34, cjgrant wrote:
> > Maybe here, or in a future change, we should cut out some use of this class
> > member gesture_.  It looks like we set it here, but then pluck it out to
> return.
> >  Could just pass the gesture around on a per-frame basis instead, maybe.
> 
> Done.
> 
>
https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
> chrome/browser/android/vr_shell/vr_controller.cc:238: if
(touch_info_->touch_up
> || !(touch_info_->is_touching)) {  // gesture ends
> On 2016/10/03 12:09:23, bshe wrote:
> > nit: move comments to a new line
> 
> Done.
> 
>
https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
> chrome/browser/android/vr_shell/vr_controller.cc:245: } else if
> (touch_position_changed) {  // User continues scrolling and there is
> On 2016/10/03 12:09:23, bshe wrote:
> > ditto
> 
> Done.
> 
>
https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
> File chrome/browser/android/vr_shell/vr_controller.h (right):
> 
>
https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
> chrome/browser/android/vr_shell/vr_controller.h:118:
std::unique_ptr<VrGesture>
> gesture_;
> On 2016/10/03 14:40:16, asimjour wrote:
> > We should be able to send multiple gestures. For example ScrollEnd + Tap
> 
> Sure, but right now the API only supports returning one, and we don't generate
> more than one. We can add complexity as needed.
> 
>
https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
> File chrome/browser/android/vr_shell/vr_input_manager.h (right):
> 
>
https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
> chrome/browser/android/vr_shell/vr_input_manager.h:25: void
> SendGesture(VrGesture gesture);
> On 2016/10/03 14:40:16, asimjour wrote:
> > SendGesture doesn't need to be public.
> 
> Done.

lgtm

[bshe, 2016-10-03 16:27:51]

On 2016/10/03 15:46:12, asimjour1 wrote:
> On 2016/10/03 15:39:15, mthiesse wrote:
> >
>
https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
> > File chrome/browser/android/vr_shell/vr_controller.cc (right):
> > 
> >
>
https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
> > chrome/browser/android/vr_shell/vr_controller.cc:154: gesture_.reset(new
> > VrGesture());
> > On 2016/10/03 14:27:34, cjgrant wrote:
> > > Maybe here, or in a future change, we should cut out some use of this
class
> > > member gesture_.  It looks like we set it here, but then pluck it out to
> > return.
> > >  Could just pass the gesture around on a per-frame basis instead, maybe.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
> > chrome/browser/android/vr_shell/vr_controller.cc:238: if
> (touch_info_->touch_up
> > || !(touch_info_->is_touching)) {  // gesture ends
> > On 2016/10/03 12:09:23, bshe wrote:
> > > nit: move comments to a new line
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
> > chrome/browser/android/vr_shell/vr_controller.cc:245: } else if
> > (touch_position_changed) {  // User continues scrolling and there is
> > On 2016/10/03 12:09:23, bshe wrote:
> > > ditto
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
> > File chrome/browser/android/vr_shell/vr_controller.h (right):
> > 
> >
>
https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
> > chrome/browser/android/vr_shell/vr_controller.h:118:
> std::unique_ptr<VrGesture>
> > gesture_;
> > On 2016/10/03 14:40:16, asimjour wrote:
> > > We should be able to send multiple gestures. For example ScrollEnd + Tap
> > 
> > Sure, but right now the API only supports returning one, and we don't
generate
> > more than one. We can add complexity as needed.
> > 
> >
>
https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
> > File chrome/browser/android/vr_shell/vr_input_manager.h (right):
> > 
> >
>
https://codereview.chromium.org/2380323003/diff/1/chrome/browser/android/vr_s...
> > chrome/browser/android/vr_shell/vr_input_manager.h:25: void
> > SendGesture(VrGesture gesture);
> > On 2016/10/03 14:40:16, asimjour wrote:
> > > SendGesture doesn't need to be public.
> > 
> > Done.
> 
> lgtm

lgtm

[mthiesse, 2016-10-03 16:28:35]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2016-10-03 16:28:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
asimjour@chromium.org
Link to the patchset: https://codereview.chromium.org/2380323003/#ps40001
(title: "Fix BUILD.gn")

[commit-bot: I haz the power, 2016-10-03 16:29:04]

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

[commit-bot: I haz the power, 2016-10-03 18:19:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 18:19:48]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[mthiesse, 2016-10-03 18:25:21]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2016-10-03 18:25:50]

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

[commit-bot: I haz the power, 2016-10-03 19:04:48]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-03 19:07:27]

Description was changed from

==========
Refactorings to vr controller code.

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

to

==========
Refactorings to vr controller code.

BUG=

Committed: https://crrev.com/8e065f3d69062bb80d4c99be75e4d46c64bcaf2e
Cr-Commit-Position: refs/heads/master@{#422480}
==========

[commit-bot: I haz the power, 2016-10-03 19:07:28]

Patchset 3 (id:??) landed as
https://crrev.com/8e065f3d69062bb80d4c99be75e4d46c64bcaf2e
Cr-Commit-Position: refs/heads/master@{#422480}