Refactor VR Shell Input. Locks input to click/scroll targets.

Refactor VR Shell Input. Locks input to click/scroll targets.

This CL refactors input paths to more closely align the content input and skia input paths.
We also now lock input to the scroll/click target.
This CL also adds a button with hover/down states.

TODO: Actually look at the passed in coordinates to see if the hover/down states should be applied.
Review-Url: https://codereview.chromium.org/2878543002
Cr-Commit-Position: refs/heads/master@{#471019}
Committed: https://chromium.googlesource.com/chromium/src/+/fb2492ce3f4bf8d9e151f3759b42be6aca6155c4

[mthiesse, 2017-05-10 19:24:30]

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

[mthiesse, 2017-05-10 19:24:31]

PTAL

[cjgrant, 2017-05-10 20:16:26]

A little bit of faith in this review, as it's really hard to trace all the
possibilities.  I'm trusting that input still works.  :)

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

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/ui_texture.h:30: // Returns true if the
state changed.
Are you adding this here as a general purpose means of checking if some
arbitrary property changed?  I find it really confusing for the bit of help it's
offering.  It could just be a pair of get/set flags methods, and subclasses
could check and update as needed.  At a minimum, it should be renamed
UpdateDrawFlags() or something, as the texture is not actually being updated.

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (left):

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:141:
canvas->restore();
Why are you removing this?  It removes the scale on line 87.

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

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:512: HandleWebVrCompatClick();
s/Compat/Compatibility/

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:533: return;
Blank line after return?

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:722: if
(!controller_->ButtonDownHappened(gvr::kControllerButtonClick))
Isn't it more intuitive to not call SendButtonDown if a controller button-down
did not happen?

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:809: if
(!GetTargetLocalPoint(eye_to_target, *element.get(),
So to be totally clear, the idea of having an element determine whether it's
being hit is a follow-on change...

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1152: // and secondarily based on
their distance (larger z-axis first).
"...based on their Z-axis distance..." maybe?

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

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.h:219: UiElement*
cursor_render_target_ = nullptr;
Now might be a good time to TODO the fact that we should handle the deletion of
elements, eventually.

[mthiesse, 2017-05-10 21:09:08]

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

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/ui_texture.h:30: // Returns true if the
state changed.
On 2017/05/10 20:16:25, cjgrant wrote:
> Are you adding this here as a general purpose means of checking if some
> arbitrary property changed?  I find it really confusing for the bit of help
it's
> offering.  It could just be a pair of get/set flags methods, and subclasses
> could check and update as needed.  At a minimum, it should be renamed
> UpdateDrawFlags() or something, as the texture is not actually being updated.

Changed to SetDrawFlags, missing getter was an oversight.

The purpose here is to make it easier to avoid re-drawing when not necessary.

We should flesh this out more in future CLs, as it's possible the state changes,
but the element still doesn't need to redraw.

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/textures/url_bar_texture.cc (left):

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/textures/url_bar_texture.cc:141:
canvas->restore();
On 2017/05/10 20:16:25, cjgrant wrote:
> Why are you removing this?  It removes the scale on line 87.

Rebasing error, good catch.

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

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:512: HandleWebVrCompatClick();
On 2017/05/10 20:16:25, cjgrant wrote:
> s/Compat/Compatibility/

Done.

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:533: return;
On 2017/05/10 20:16:26, cjgrant wrote:
> Blank line after return?

Done.

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:722: if
(!controller_->ButtonDownHappened(gvr::kControllerButtonClick))
On 2017/05/10 20:16:26, cjgrant wrote:
> Isn't it more intuitive to not call SendButtonDown if a controller button-down
> did not happen?

So this is a stylistic choice. I decided to avoid cluttering up
HandleControllerInput with logic about which events to send, and pushed all of
the logic down into the functions.

I could do something like call them all "MaybeSendButtonDown", but all of these
functions are consistent and hopefully clear in what they're doing, so I don't
know that that's necessary.

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:809: if
(!GetTargetLocalPoint(eye_to_target, *element.get(),
On 2017/05/10 20:16:26, cjgrant wrote:
> So to be totally clear, the idea of having an element determine whether it's
> being hit is a follow-on change...

yes.

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1152: // and secondarily based on
their distance (larger z-axis first).
Didn't mean to include these changes in this CL. Will separate out.

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1152: // and secondarily based on
their distance (larger z-axis first).
On 2017/05/10 20:16:26, cjgrant wrote:
> "...based on their Z-axis distance..." maybe?

Done.

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

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.h:219: UiElement*
cursor_render_target_ = nullptr;
On 2017/05/10 20:16:26, cjgrant wrote:
> Now might be a good time to TODO the fact that we should handle the deletion
of
> elements, eventually.

Done.

[mthiesse, 2017-05-10 21:40:36]

Patchset #3 (id:40001) has been deleted

[mthiesse, 2017-05-10 21:43:09]

Patchset #4 (id:80001) has been deleted

[cjgrant, 2017-05-10 21:52:09]

lgtm

[mthiesse, 2017-05-10 22:34:56]

Patchset #4 (id:100001) has been deleted

[asimjour1, 2017-05-11 12:31:12]

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

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_controller.cc:340:
!ButtonState(gvr::kControllerButtonClick)) {
Do we need both !ButtonDownHappened and !ButtonState at the same time. I think
only !ButtonState should be sufficient.

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

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.h:221: // TODO(mthiesse): Whether a
target is flinging should be a property on the
I don't get this comment. If you mean "whether a target is flinging at this
moment", I think you are wrong. At least for the content we shouldn't need that
flag.

[mthiesse, 2017-05-11 14:12:00]

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

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_controller.cc:340:
!ButtonState(gvr::kControllerButtonClick)) {
On 2017/05/11 12:31:12, asimjour1 wrote:
> Do we need both !ButtonDownHappened and !ButtonState at the same time. I think
> only !ButtonState should be sufficient. 

Done.

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

https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.h:221: // TODO(mthiesse): Whether a
target is flinging should be a property on the
On 2017/05/11 12:31:12, asimjour1 wrote:
> I don't get this comment. If you mean "whether a target is flinging at this
> moment", I think you are wrong. At least for the content we shouldn't need
that
> flag.

You're right, changed the TODO.

[asimjour1, 2017-05-11 14:12:34]

On 2017/05/11 14:12:00, mthiesse wrote:
>
https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
> File chrome/browser/android/vr_shell/vr_controller.cc (right):
> 
>
https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
> chrome/browser/android/vr_shell/vr_controller.cc:340:
> !ButtonState(gvr::kControllerButtonClick)) {
> On 2017/05/11 12:31:12, asimjour1 wrote:
> > Do we need both !ButtonDownHappened and !ButtonState at the same time. I
think
> > only !ButtonState should be sufficient. 
> 
> Done.
> 
>
https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
> File chrome/browser/android/vr_shell/vr_shell_gl.h (right):
> 
>
https://codereview.chromium.org/2878543002/diff/1/chrome/browser/android/vr_s...
> chrome/browser/android/vr_shell/vr_shell_gl.h:221: // TODO(mthiesse): Whether
a
> target is flinging should be a property on the
> On 2017/05/11 12:31:12, asimjour1 wrote:
> > I don't get this comment. If you mean "whether a target is flinging at this
> > moment", I think you are wrong. At least for the content we shouldn't need
> that
> > flag.
> 
> You're right, changed the TODO.

lgtm

[mthiesse, 2017-05-11 14:14:24]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-05-11 14:14:24]

The patchset sent to the CQ was uploaded after l-g-t-m from cjgrant@chromium.org
Link to the patchset: https://codereview.chromium.org/2878543002/#ps140001
(title: "Address comments")

[commit-bot: I haz the power, 2017-05-11 14:15:03]

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, 2017-05-11 16:38:29]

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

[commit-bot: I haz the power, 2017-05-11 16:38:30]

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, 2017-05-11 16:40:31]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2017-05-11 16:41:32]

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, 2017-05-11 18:40:18]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494520831783660,
"parent_rev": "6a43b93049d96b053514530d99754a24aeaaa4fd", "commit_rev":
"fb2492ce3f4bf8d9e151f3759b42be6aca6155c4"}

[commit-bot: I haz the power, 2017-05-11 18:40:25]

Description was changed from

==========
Refactor VR Shell Input. Locks input to click/scroll targets.

This CL refactors input paths to more closely align the content input and skia
input paths.
We also now lock input to the scroll/click target.
This CL also adds a button with hover/down states.

TODO: Actually look at the passed in coordinates to see if the hover/down states
should be applied.
Thanks to cjgrant@ for the round button implementation.
==========

to

==========
Refactor VR Shell Input. Locks input to click/scroll targets.

This CL refactors input paths to more closely align the content input and skia
input paths.
We also now lock input to the scroll/click target.
This CL also adds a button with hover/down states.

TODO: Actually look at the passed in coordinates to see if the hover/down states
should be applied.
Review-Url: https://codereview.chromium.org/2878543002
Cr-Commit-Position: refs/heads/master@{#471019}
Committed:
https://chromium.googlesource.com/chromium/src/+/fb2492ce3f4bf8d9e151f3759b42...
==========

[commit-bot: I haz the power, 2017-05-11 18:40:26]

Committed patchset #5 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/fb2492ce3f4bf8d9e151f3759b42...

[findit-for-me, 2017-05-12 02:13:11]

Findit (https://goo.gl/kROfz5) identified this CL at revision 471019 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...