VR Shell: Allow UI elements to determine hit testing.

chrome/browser/android/vr_shell/ui_elements/close_button.cc

Index: chrome/browser/android/vr_shell/ui_elements/close_button.cc
diff --git a/chrome/browser/android/vr_shell/ui_elements/close_button.cc b/chrome/browser/android/vr_shell/ui_elements/close_button.cc
index ff71c55168fcd5baa4466a3301c17e07f02f9d4b..1489958dd6f7a0d5c585cdd9c407def81fee384b 100644
--- a/chrome/browser/android/vr_shell/ui_elements/close_button.cc
+++ b/chrome/browser/android/vr_shell/ui_elements/close_button.cc
@@ -17,37 +17,43 @@ CloseButton::CloseButton(base::Callback<void()> click_handler)
 CloseButton::~CloseButton() = default;
 
 void CloseButton::OnHoverEnter(gfx::PointF position) {
-  hover_ = true;
-  OnStateUpdated();
+  OnStateUpdated(position);
 }
 
 void CloseButton::OnHoverLeave() {
-  hover_ = false;
-  OnStateUpdated();
+  OnStateUpdated(gfx::PointF(-1000, -1000));

[cjgrant, 2017/05/15 16:55:09]

Should probably use language numeric limits here, for correctness and
readability.

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

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.

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

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

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

Done.

+}
+
+void CloseButton::OnMove(gfx::PointF position) {
+  OnStateUpdated(position);
 }
 
 void CloseButton::OnButtonDown(gfx::PointF position) {
   down_ = true;
-  OnStateUpdated();
+  OnStateUpdated(position);
 }
 
 void CloseButton::OnButtonUp(gfx::PointF position) {
   down_ = false;
-  OnStateUpdated();
-  if (position.x() < 0 || position.x() > 1.0f)
-    return;
-  if (position.y() < 0 || position.y() > 1.0f)
-    return;
-  click_handler_.Run();
+  OnStateUpdated(position);
+  if (HitTest(position))

[cjgrant, 2017/05/15 16:55:09]

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.

[mthiesse, 2017/05/15 21:11:26]

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.

+    click_handler_.Run();
+}
+
+bool CloseButton::HitTest(gfx::PointF point) const {
+  return (point - gfx::PointF(0.5, 0.5)).LengthSquared() < 0.25;
 }
 
 UiTexture* CloseButton::GetTexture() const {
   return texture_.get();
 }
 
-void CloseButton::OnStateUpdated() {
-  int flags = hover_ ? CloseButtonTexture::FLAG_HOVER : 0;
-  flags |= (down_ && hover_) ? CloseButtonTexture::FLAG_DOWN : 0;
+void CloseButton::OnStateUpdated(gfx::PointF position) {
+  bool hitting = HitTest(position);
+  bool down = hitting ? down_ : false;
+
+  int flags = hitting ? CloseButtonTexture::FLAG_HOVER : 0;
+  flags |= down ? CloseButtonTexture::FLAG_DOWN : 0;
   if (!texture_->SetDrawFlags(flags))
     return;
   UpdateTexture();