VR: Text field improvements and cleanup.

- If PrepareToDraw indicates a scene change, do layout.
- Trigger scene rendering if the cursor moved.
- Move a static assert to live with TextInputInfo.
- Add more tests.

BUG=
R=mthiesse, ymalik

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I4f05832610c0d8e0676599bc92b9a926afbbca57
Reviewed-on: https://chromium-review.googlesource.com/809827
Commit-Queue: Christopher Grant <cjgrant@chromium.org>
Reviewed-by: Yash Malik <ymalik@chromium.org>
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522249}

chrome/browser/vr/elements/text.cc

@@ -105,7 +105,11 @@ void Text::SetCursorPosition(int position) {
   texture_->SetCursorPosition(position);
 }
 
-gfx::RectF Text::GetCursorBounds() {
+gfx::Rect Text::GetRawCursorBounds() const {
+  return texture_->get_cursor_bounds();
+}
+
+gfx::RectF Text::GetCursorBounds() const {
   // Note that gfx:: cursor bounds always indicate a one-pixel width, so we
   // override the width here to be a percentage of height for the sake of
   // arbitrary texture sizes.

chrome/browser/vr/elements/text.h

@@ -37,9 +37,13 @@ class Text : public TexturedElement {
   void SetCursorEnabled(bool enabled);
   void SetCursorPosition(int position);
 
-  // Returns the most recently computed cursor position, in DMM, relative to the
-  // corner of the element.
-  gfx::RectF GetCursorBounds();
+  // Returns the most recently computed cursor position, in pixels.  This is
+  // used for scene dirtiness and testing.
+  gfx::Rect GetRawCursorBounds() const;
+
+  // Returns the most recently computed cursor position, in fractions of the
+  // texture size, relative to the upper-left corner of the element.
+  gfx::RectF GetCursorBounds() const;
 
   void OnSetSize(gfx::SizeF size) override;
 

chrome/browser/vr/elements/text_input.cc

@@ -48,6 +48,7 @@ TextInput::TextInput(int maximum_width_pixels,
   this->AddChild(std::move(text));
 
   auto cursor = base::MakeUnique<Rect>();
+  cursor->SetVisible(false);
   cursor->set_type(kTypeTextInputCursor);
   cursor->set_draw_phase(kPhaseForeground);
   cursor->set_hit_testable(false);
@@ -81,7 +82,8 @@ void TextInput::OnFocusChanged(bool focused) {
   if (delegate_ && focused)
     delegate_->UpdateInput(text_info_);
 
-  focus_changed_callback_.Run(focused);
+  if (focus_changed_callback_)
+    focus_changed_callback_.Run(focused);
 }
 
 void TextInput::RequestFocus() {

chrome/browser/vr/elements/text_input.h

@@ -50,6 +50,10 @@ class TextInput : public UiElement {
   void OnSetSize(gfx::SizeF size) final;
   void OnSetName() final;
 
+  Text* get_hint_element() { return hint_element_; }
+  Text* get_text_element() { return text_element_; }
+  Rect* get_cursor_element() { return cursor_element_; }
+
  private:
   void LayOutChildren() final;
   bool SetCursorBlinkState(const base::TimeTicks& time);

chrome/browser/vr/model/text_input_info.cc

@@ -19,4 +19,8 @@ bool TextInputInfo::operator!=(const TextInputInfo& other) const {
   return !(*this == other);
 }
 
+static_assert(sizeof(base::string16) + 8 == sizeof(TextInputInfo),
+              "If new fields are added to TextInputInfo, we must explicitly "
+              "bump this size and update operator==");
+
 }  // namespace vr

chrome/browser/vr/text_input_unittest.cc

@@ -11,10 +11,12 @@
 #include "build/build_config.h"
 #include "chrome/browser/vr/databinding/binding.h"
 #include "chrome/browser/vr/elements/keyboard.h"
+#include "chrome/browser/vr/elements/text.h"
 #include "chrome/browser/vr/elements/ui_element.h"
 #include "chrome/browser/vr/keyboard_delegate.h"
 #include "chrome/browser/vr/model/camera_model.h"
 #include "chrome/browser/vr/model/model.h"
+#include "chrome/browser/vr/test/animation_utils.h"
 #include "chrome/browser/vr/test/constants.h"
 #include "chrome/browser/vr/test/ui_test.h"
 #include "chrome/browser/vr/text_input_delegate.h"
@@ -23,6 +25,7 @@
 #include "chrome/browser/vr/ui_scene_creator.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "third_party/WebKit/public/platform/WebGestureEvent.h"
+#include "ui/gfx/render_text.h"
 
 using ::testing::_;
 using ::testing::InSequence;
@@ -30,21 +33,6 @@ using ::testing::StrictMock;
 
 namespace vr {
 
-namespace {
-
-#if defined(OS_LINUX)
-// The purpose here is to catch forgetting to update operator==. Ideally this
-// should go in TextInputInfo, but the size of base::string16 varies accross
-// platforms (i.e 24 bytes on linux and 20 bytes on android), so we can't add it
-// there in a generic way.
-static constexpr size_t kTextInputInfoSize = 32;
-static_assert(kTextInputInfoSize == sizeof(TextInputInfo),
-              "If new fields are added to TextInputInfo, we must explicitly "
-              "bump this size and update operator== below");
-#endif
-
-}  // namespace
-
 class MockTextInputDelegate : public TextInputDelegate {
  public:
   MockTextInputDelegate() = default;
@@ -74,7 +62,7 @@ class MockKeyboardDelegate : public KeyboardDelegate {
   DISALLOW_COPY_AND_ASSIGN(MockKeyboardDelegate);
 };
 
-class TextInputTest : public UiTest {
+class TextInputSceneTest : public UiTest {
  public:
   void SetUp() override {
     UiTest::SetUp();
@@ -98,7 +86,7 @@ class TextInputTest : public UiTest {
   testing::Sequence in_sequence_;
 };
 
-TEST_F(TextInputTest, InputFieldFocus) {
+TEST_F(TextInputSceneTest, InputFieldFocus) {
   // Set mock delegates.
   auto* kb = static_cast<Keyboard*>(scene_->GetUiElementByName(kKeyboard));
   auto kb_delegate = base::MakeUnique<StrictMock<MockKeyboardDelegate>>();
@@ -126,7 +114,7 @@ TEST_F(TextInputTest, InputFieldFocus) {
   EXPECT_TRUE(OnBeginFrame());
 }
 
-TEST_F(TextInputTest, InputFieldEdit) {
+TEST_F(TextInputSceneTest, InputFieldEdit) {
   // UpdateInput should not be called if the text input doesn't have focus.
   EXPECT_CALL(*text_input_delegate_, UpdateInput(_))
       .Times(0)
@@ -145,4 +133,74 @@ TEST_F(TextInputTest, InputFieldEdit) {
   EXPECT_EQ(info, *text_input_info_);
 }
 
+TEST(TextInputTest, HintText) {
+  UiScene scene;
+
+  auto instance =
+      base::MakeUnique<TextInput>(512, 10, TextInput::OnFocusChangedCallback(),
+                                  TextInput::OnInputEditedCallback());
+  instance->set_name(kOmniboxTextField);
+  instance->SetSize(1, 0);
+  TextInput* element = instance.get();
+  scene.root_element().AddChild(std::move(instance));
+
+  // Text field is empty, so we should be showing hint text.
+  scene.OnBeginFrame(base::TimeTicks(), kForwardVector);
+  EXPECT_GT(element->get_hint_element()->GetTargetOpacity(), 0);
+
+  // When text enters the field, the hint should disappear.
+  TextInputInfo info;
+  info.text = base::UTF8ToUTF16("text");
+  element->UpdateInput(info);
+  scene.OnBeginFrame(base::TimeTicks(), kForwardVector);
+  EXPECT_EQ(element->get_hint_element()->GetTargetOpacity(), 0);
+}
+
+TEST(TextInputTest, Cursor) {
+  UiScene scene;
+
+  auto instance =
+      base::MakeUnique<TextInput>(512, 10, TextInput::OnFocusChangedCallback(),
+                                  TextInput::OnInputEditedCallback());
+  instance->set_name(kOmniboxTextField);
+  instance->SetSize(1, 0);
+  TextInput* element = instance.get();
+  scene.root_element().AddChild(std::move(instance));
+
+  // The cursor should not be blinking or visible.
+  float initial = element->get_cursor_element()->GetTargetOpacity();
+  EXPECT_EQ(initial, 0.f);
+  for (int ms = 0; ms <= 2000; ms += 100) {
+    scene.OnBeginFrame(MsToTicks(ms), kForwardVector);
+    EXPECT_EQ(initial, element->get_cursor_element()->GetTargetOpacity());
+  }
+
+  // When focused, the cursor should start blinking.
+  element->OnFocusChanged(true);
+  initial = element->get_cursor_element()->GetTargetOpacity();
+  bool toggled = false;
+  for (int ms = 0; ms <= 2000; ms += 100) {
+    scene.OnBeginFrame(MsToTicks(ms), kForwardVector);
+    if (initial != element->get_cursor_element()->GetTargetOpacity())
+      toggled = true;
+  }
+  EXPECT_TRUE(toggled);
+
+// TODO(cjgrant): Continue with the test cases below, when they're debugged.
+#if 0
+  TextInputInfo info;
+  info.text = base::UTF8ToUTF16("text");
+
+  // When the cursor position moves, the cursor element should move.
+  element->UpdateInput(info);
+  auto result = element->get_text_element()->LayOutTextForTest({512, 512});
+  auto position1 = element->get_text_element()->GetRawCursorBounds();
+  info.selection_end = 1;
+  element->UpdateInput(info);
+  element->get_text_element()->LayOutTextForTest({512, 512});
+  auto position2 = element->get_text_element()->GetRawCursorBounds();
+  EXPECT_NE(position1, position2);
+#endif
+}
+
 }  // namespace vr

chrome/browser/vr/ui_scene.cc

@@ -98,7 +98,6 @@ std::unique_ptr<UiElement> UiScene::RemoveUiElement(int element_id) {
 bool UiScene::OnBeginFrame(const base::TimeTicks& current_time,
                            const gfx::Vector3dF& look_at) {
   bool scene_dirty = !initialized_scene_ || is_dirty_;
-  bool needs_redraw = false;
   initialized_scene_ = true;
   is_dirty_ = false;
 
@@ -140,7 +139,7 @@ bool UiScene::OnBeginFrame(const base::TimeTicks& current_time,
     // synchronously in response to input should be prohibited.
     for (auto& element : *root_element_) {
       if (element.PrepareToDraw())
-        needs_redraw = true;
+        scene_dirty = true;
       element.set_update_phase(UiElement::kUpdatedTexturesAndSizes);
     }
   }
@@ -151,7 +150,7 @@ bool UiScene::OnBeginFrame(const base::TimeTicks& current_time,
     for (auto& element : *root_element_) {
       element.set_update_phase(UiElement::kUpdatedWorldSpaceTransform);
     }
-    return needs_redraw;
+    return false;
   }
 
   {
@@ -175,7 +174,7 @@ bool UiScene::OnBeginFrame(const base::TimeTicks& current_time,
     root_element_->UpdateWorldSpaceTransformRecursive();
   }
 
-  return scene_dirty || needs_redraw;
+  return scene_dirty;
 }
 
 bool UiScene::UpdateTextures() {