VR Shell: Allow UI elements to determine hit testing.

VR Shell: Allow UI elements to determine hit testing.

BUG=715554
Review-Url: https://codereview.chromium.org/2878083003
Cr-Commit-Position: refs/heads/master@{#471954}
Committed: https://chromium.googlesource.com/chromium/src/+/ee675c3aea2cccb7547b1b46b848374caef5f6be

[mthiesse, 2017-05-12 20:46:04]

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

[mthiesse, 2017-05-12 20:46:05]

PTAL

[mthiesse, 2017-05-12 20:46:15]

Description was changed from

==========
VR Shell: Allow UI elements to determing hit testing.

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

to

==========
VR Shell: Allow UI elements to determine hit testing.

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

[mthiesse, 2017-05-12 20:48:33]

Patchset #2 (id:20001) has been deleted

[cjgrant, 2017-05-15 16:55:10]

https://codereview.chromium.org/2878083003/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/close_button.cc (right):

https://codereview.chromium.org/2878083003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/close_button.cc:24:
OnStateUpdated(gfx::PointF(-1000, -1000));
Should probably use language numeric limits here, for correctness and
readability.

https://codereview.chromium.org/2878083003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/close_button.cc:39: if
(HitTest(position))
At first glance, I'd have thought the common code would do the hit test, and
only call OnButtonUp if we're on the button.  But this is our way of working
around OnButtonDownCancel(), I guess.  The fact that HitTest needs to be used
both in and out of this class is weird to me.

https://codereview.chromium.org/2878083003/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/close_button.h (right):

https://codereview.chromium.org/2878083003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/close_button.h:29: bool
HitTest(gfx::PointF point) const override;
How are we going to handle elements that have two hover/clickable regions
internally?  Hopefully we won't have any for BBB, but if we did, maybe some of
the input code that was just refactored in vr_shell_gl.cc should live in a
helper class, that can be used recursively by vr_shell_gl.cc and individual
elements if needed.

That way, two hoverable/clickable buttons on (say the URL bar) would be
guaranteed to work the same as two hoverable/clickable UiElements.

Thoughts (other than "not in this CL!")?

https://codereview.chromium.org/2878083003/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/ui_element.h (right):

https://codereview.chromium.org/2878083003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/ui_element.h:118: // Whether the
point (relative to the origin of the element), should be
- Typo at "by considered".
- What does "local" mean?
- Maybe say that the incoming 0:1 numbers are relative to the quad, and it's up
to the element to consider those according to its specific shape (round button
is a great example).

[cjgrant, 2017-05-15 16:58:04]

Also, this should probably be BUG=715554

[cjgrant, 2017-05-15 16:58:05]

Also, this should probably be BUG=715554

[mthiesse, 2017-05-15 20:55:44]

Description was changed from

==========
VR Shell: Allow UI elements to determine hit testing.

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

to

==========
VR Shell: Allow UI elements to determine hit testing.

BUG=715554
==========

[mthiesse, 2017-05-15 21:11:27]

I've added the omnibox hit testing and sizing into this CL for demonstration,
but will be splitting both out into separate CLs before landing.

https://codereview.chromium.org/2878083003/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/close_button.cc (right):

https://codereview.chromium.org/2878083003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/close_button.cc:24:
OnStateUpdated(gfx::PointF(-1000, -1000));
On 2017/05/15 16:55:09, cjgrant wrote:
> Should probably use language numeric limits here, for correctness and
> readability.

I didn't want the code to have to think about infinities or NaNs. This is just
some position that's outside of the element's hit-testable region. Sure NaN is
technically more correct, but having to check isNaN everywhere would just be
tedious.

https://codereview.chromium.org/2878083003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/close_button.cc:39: if
(HitTest(position))
On 2017/05/15 16:55:09, cjgrant wrote:
> At first glance, I'd have thought the common code would do the hit test, and
> only call OnButtonUp if we're on the button.  But this is our way of working
> around OnButtonDownCancel(), I guess.  The fact that HitTest needs to be used
> both in and out of this class is weird to me.

I think this will make more sense once you see the URL bar? HitTest determines
whether the element should be handling input at all, which happens to overlap
with the clickable area in the case of this button. With the URL bar HitTest
isn't the same as the check in ButtonUp.

https://codereview.chromium.org/2878083003/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/close_button.h (right):

https://codereview.chromium.org/2878083003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/close_button.h:29: bool
HitTest(gfx::PointF point) const override;
On 2017/05/15 16:55:09, cjgrant wrote:
> How are we going to handle elements that have two hover/clickable regions
> internally?  Hopefully we won't have any for BBB, but if we did, maybe some of
> the input code that was just refactored in vr_shell_gl.cc should live in a
> helper class, that can be used recursively by vr_shell_gl.cc and individual
> elements if needed.
> 
> That way, two hoverable/clickable buttons on (say the URL bar) would be
> guaranteed to work the same as two hoverable/clickable UiElements.
> 
> Thoughts (other than "not in this CL!")?

Don't like it, and also "not in this CL". An element should handle it's own
internals, though if something happens to be re-usable then of course we should
reuse it. The main reason not to share code is in-element hit-testing is 2D, and
scene hit-testing is 3D.

https://codereview.chromium.org/2878083003/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/ui_element.h (right):

https://codereview.chromium.org/2878083003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/ui_element.h:118: // Whether the
point (relative to the origin of the element), should be
On 2017/05/15 16:55:09, cjgrant wrote:
> - Typo at "by considered".
> - What does "local" mean?
> - Maybe say that the incoming 0:1 numbers are relative to the quad, and it's
up
> to the element to consider those according to its specific shape (round button
> is a great example).

What typo?

Clarified the comment and got rid of the term "local".

[cjgrant, 2017-05-15 21:51:52]

lgtm

https://codereview.chromium.org/2878083003/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/close_button.cc (right):

https://codereview.chromium.org/2878083003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/close_button.cc:24:
OnStateUpdated(gfx::PointF(-1000, -1000));
On 2017/05/15 21:11:27, mthiesse wrote:
> On 2017/05/15 16:55:09, cjgrant wrote:
> > Should probably use language numeric limits here, for correctness and
> > readability.
> 
> I didn't want the code to have to think about infinities or NaNs. This is just
> some position that's outside of the element's hit-testable region. Sure NaN is
> technically more correct, but having to check isNaN everywhere would just be
> tedious.

I just meant to use the modern equivalent of INT_MAX (or whatever) instead of
-1000, not to use isNan for checks.

https://codereview.chromium.org/2878083003/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/ui_element.h (right):

https://codereview.chromium.org/2878083003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/ui_element.h:118: // Whether the
point (relative to the origin of the element), should be
On 2017/05/15 21:11:27, mthiesse wrote:
> On 2017/05/15 16:55:09, cjgrant wrote:
> > - Typo at "by considered".
> > - What does "local" mean?
> > - Maybe say that the incoming 0:1 numbers are relative to the quad, and it's
> up
> > to the element to consider those according to its specific shape (round
button
> > is a great example).
> 
> What typo?
> 
> Clarified the comment and got rid of the term "local".

"All elements are by considered rectangular by default"

[mthiesse, 2017-05-15 22:23:32]

https://codereview.chromium.org/2878083003/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/close_button.cc (right):

https://codereview.chromium.org/2878083003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/close_button.cc:24:
OnStateUpdated(gfx::PointF(-1000, -1000));
On 2017/05/15 21:51:52, cjgrant wrote:
> On 2017/05/15 21:11:27, mthiesse wrote:
> > On 2017/05/15 16:55:09, cjgrant wrote:
> > > Should probably use language numeric limits here, for correctness and
> > > readability.
> > 
> > I didn't want the code to have to think about infinities or NaNs. This is
just
> > some position that's outside of the element's hit-testable region. Sure NaN
is
> > technically more correct, but having to check isNaN everywhere would just be
> > tedious.
> 
> I just meant to use the modern equivalent of INT_MAX (or whatever) instead of
> -1000, not to use isNan for checks.

Done.

https://codereview.chromium.org/2878083003/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/ui_element.h (right):

https://codereview.chromium.org/2878083003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/ui_element.h:118: // Whether the
point (relative to the origin of the element), should be
On 2017/05/15 21:51:52, cjgrant wrote:
> On 2017/05/15 21:11:27, mthiesse wrote:
> > On 2017/05/15 16:55:09, cjgrant wrote:
> > > - Typo at "by considered".
> > > - What does "local" mean?
> > > - Maybe say that the incoming 0:1 numbers are relative to the quad, and
it's
> > up
> > > to the element to consider those according to its specific shape (round
> button
> > > is a great example).
> > 
> > What typo?
> > 
> > Clarified the comment and got rid of the term "local".
> 
> "All elements are by considered rectangular by default"

I do not see the the problem.

fixed ;)

[mthiesse, 2017-05-15 22:27:14]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-05-15 22:27:15]

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/2878083003/#ps120001
(title: "Remove unrelated changes")

[commit-bot: I haz the power, 2017-05-15 22:28:21]

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-16 00:20:41]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494887234700030,
"parent_rev": "6bbc32fc27c0b7eb6fb416fe65ead0cefdcf6f13", "commit_rev":
"ee675c3aea2cccb7547b1b46b848374caef5f6be"}

[commit-bot: I haz the power, 2017-05-16 00:20:53]

Description was changed from

==========
VR Shell: Allow UI elements to determine hit testing.

BUG=715554
==========

to

==========
VR Shell: Allow UI elements to determine hit testing.

BUG=715554

Review-Url: https://codereview.chromium.org/2878083003
Cr-Commit-Position: refs/heads/master@{#471954}
Committed:
https://chromium.googlesource.com/chromium/src/+/ee675c3aea2cccb7547b1b46b848...
==========

[commit-bot: I haz the power, 2017-05-16 00:20:54]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/ee675c3aea2cccb7547b1b46b848...