Cleanup of omnibox_view_views_unittest.cc in advance of adding more to it.

chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc

Index: chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc
diff --git a/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc b/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc
index f6718c78ed2d131abe6579a95317629920ecd5dc..2499ff045ed11b81baee569cc4d6c3a7a4f4d130 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc
@@ -26,59 +26,46 @@
 #include "chrome/browser/chromeos/input_method/mock_input_method_manager_impl.h"
 #endif
 
+using gfx::Range;
+
 namespace {
 
-class TestingOmniboxViewViews : public OmniboxViewViews {
+// TestingOmniboxView ---------------------------------------------------------
+
+class TestingOmniboxView : public OmniboxViewViews {
  public:
-  TestingOmniboxViewViews(OmniboxEditController* controller,
-                          Profile* profile,
-                          CommandUpdater* command_updater)
-      : OmniboxViewViews(controller,
-                         profile,
-                         command_updater,
-                         false,
-                         nullptr,
-                         gfx::FontList()),
-        update_popup_call_count_(0),
-        profile_(profile),
-        base_text_is_emphasized_(false) {}
-
-  void CheckUpdatePopupCallInfo(size_t call_count, const base::string16& text,
-                                const gfx::Range& selection_range) {
-    EXPECT_EQ(call_count, update_popup_call_count_);
-    EXPECT_EQ(text, update_popup_text_);
-    EXPECT_EQ(selection_range, update_popup_selection_range_);
+  enum BaseTextEmphasis {
+    DEEMPHASIZED,
+    EMPHASIZED,
+    UNSET,
+  };
+
+  TestingOmniboxView(OmniboxEditController* controller,
+                     Profile* profile,
+                     CommandUpdater* command_updater);
+
+  static BaseTextEmphasis to_emphasis(bool emphasize) {

[Mark P, 2017/03/07 21:10:18]

nit: awkward function name, hard to read outside of this declaration context. 
perhaps to_base_text_emphasis()?

Also, consider moving this into a namespace, out of the class.

Or--an alternative idea, less good in my opinion--simply instead have functions
like SetEmphasis(bool) and VerifyEmphasis(bool) that looks at the local field
directly.

[Peter Kasting, 2017/03/08 10:57:31]

Done.
It can be done if I also move the enum out of the class.  I didn't want to do
that, since the enum is only meaningful inside this specific class.  Without
that, there's no real way to do this, so left it as-is.

+    return emphasize ? EMPHASIZED : DEEMPHASIZED;
   }
 
-  void EmphasizeURLComponents() override {
-    UpdateTextStyle(text(), ChromeAutocompleteSchemeClassifier(profile_));
-  }
+  void ResetEmphasisTestState();
 
-  gfx::Range scheme_range() const { return scheme_range_; }
-  gfx::Range emphasis_range() const { return emphasis_range_; }
-  bool base_text_is_emphasized() const { return base_text_is_emphasized_; }
+  void CheckUpdatePopupCallInfo(size_t call_count,
+                                const base::string16& text,
+                                const Range& selection_range);
 
- private:
-  // OmniboxView:
-  void UpdatePopup() override {
-    ++update_popup_call_count_;
-    update_popup_text_ = text();
-    update_popup_selection_range_ = GetSelectedRange();
-  }
+  Range scheme_range() const { return scheme_range_; }
+  Range emphasis_range() const { return emphasis_range_; }
+  BaseTextEmphasis base_text_emphasis() const { return base_text_emphasis_; }
 
-  void SetEmphasis(bool emphasize, const gfx::Range& range) override {
-    if (range == gfx::Range::InvalidRange()) {
-      base_text_is_emphasized_ = emphasize;
-      return;
-    }
+  // OmniboxViewViews:
+  void EmphasizeURLComponents() override;
 
-    EXPECT_TRUE(emphasize);
-    emphasis_range_ = range;
-  }
-
-  void UpdateSchemeStyle(const gfx::Range& range) override {
-    scheme_range_ = range;
-  }
+ private:
+  // OmniboxViewViews:
+  void UpdatePopup() override;
+  void SetEmphasis(bool emphasize, const Range& range) override;
+  void UpdateSchemeStyle(const Range& range) override;
 
   // Simplistic test override returns whether a given string looks like a URL
   // without having to mock AutocompleteClassifier objects and their
@@ -89,29 +76,81 @@ class TestingOmniboxViewViews : public OmniboxViewViews {
     return looks_like_url;
   }
 
-  size_t update_popup_call_count_;
+  size_t update_popup_call_count_ = 0;
   base::string16 update_popup_text_;
-  gfx::Range update_popup_selection_range_;
+  Range update_popup_selection_range_;
   Profile* profile_;
 
   // Range of the last scheme logged by UpdateSchemeStyle().
-  gfx::Range scheme_range_;
+  Range scheme_range_;
 
   // Range of the last text emphasized by SetEmphasis().
-  gfx::Range emphasis_range_;
+  Range emphasis_range_;
 
   // SetEmphasis() logs whether the base color of the text is emphasized.
-  bool base_text_is_emphasized_;
+  BaseTextEmphasis base_text_emphasis_ = UNSET;
 
-  DISALLOW_COPY_AND_ASSIGN(TestingOmniboxViewViews);
+  DISALLOW_COPY_AND_ASSIGN(TestingOmniboxView);
 };
 
+TestingOmniboxView::TestingOmniboxView(OmniboxEditController* controller,
+                                       Profile* profile,
+                                       CommandUpdater* command_updater)
+    : OmniboxViewViews(controller,
+                       profile,
+                       command_updater,
+                       false,
+                       nullptr,
+                       gfx::FontList()),
+      profile_(profile) {}
+
+void TestingOmniboxView::ResetEmphasisTestState() {
+  base_text_emphasis_ = UNSET;
+  emphasis_range_ = Range::InvalidRange();
+  scheme_range_ = Range::InvalidRange();
+}
+
+void TestingOmniboxView::CheckUpdatePopupCallInfo(
+    size_t call_count,
+    const base::string16& text,
+    const Range& selection_range) {
+  EXPECT_EQ(call_count, update_popup_call_count_);
+  EXPECT_EQ(text, update_popup_text_);
+  EXPECT_EQ(selection_range, update_popup_selection_range_);
+}
+
+void TestingOmniboxView::EmphasizeURLComponents() {
+  UpdateTextStyle(text(), ChromeAutocompleteSchemeClassifier(profile_));
+}
+
+void TestingOmniboxView::UpdatePopup() {
+  ++update_popup_call_count_;
+  update_popup_text_ = text();
+  update_popup_selection_range_ = GetSelectedRange();
+}
+
+void TestingOmniboxView::SetEmphasis(bool emphasize, const Range& range) {
+  if (range == Range::InvalidRange()) {
+    base_text_emphasis_ = to_emphasis(emphasize);
+    return;
+  }
+
+  EXPECT_TRUE(emphasize);
+  emphasis_range_ = range;
+}
+
+void TestingOmniboxView::UpdateSchemeStyle(const Range& range) {
+  scheme_range_ = range;
+}
+
+// TestingOmniboxEditController -----------------------------------------------
+
 class TestingOmniboxEditController : public ChromeOmniboxEditController {
  public:
   explicit TestingOmniboxEditController(CommandUpdater* command_updater)
       : ChromeOmniboxEditController(command_updater) {}
 
- protected:
+ private:
   // ChromeOmniboxEditController:
   void UpdateWithoutTabRestore() override {}
   void OnChanged() override {}
@@ -119,66 +158,70 @@ class TestingOmniboxEditController : public ChromeOmniboxEditController {
   const ToolbarModel* GetToolbarModel() const override { return nullptr; }
   content::WebContents* GetWebContents() override { return nullptr; }
 
- private:
   DISALLOW_COPY_AND_ASSIGN(TestingOmniboxEditController);
 };
 
 }  // namespace
 
+// OmniboxViewViewsTest -------------------------------------------------------
+
 class OmniboxViewViewsTest : public testing::Test {
  public:
-  OmniboxViewViewsTest()
-      : command_updater_(NULL),
-        omnibox_edit_controller_(&command_updater_) {
-  }
-
-  TestingOmniboxViewViews* omnibox_view() {
-    return omnibox_view_.get();
-  }
-
-  views::Textfield* omnibox_textfield() {
-    return omnibox_view();
-  }
+  OmniboxViewViewsTest();
 
+  TestingOmniboxView* omnibox_view() { return omnibox_view_.get(); }
+  views::Textfield* omnibox_textfield() { return omnibox_view(); }
   ui::TextEditCommand scheduled_text_edit_command() const {
     return test_api_->scheduled_text_edit_command();
   }
 
-  void SetAndEmphasizeText(const std::string& new_text) {
-    omnibox_textfield()->SetText(base::ASCIIToUTF16(new_text));
-    omnibox_view()->EmphasizeURLComponents();
-  }
+  // Sets |new_text| as the omnibox text, and emphasizes it appropriately.
+  void SetAndEmphasizeText(const std::string& new_text);
 
  private:
   // testing::Test:
-  void SetUp() override {
-#if defined(OS_CHROMEOS)
-    chromeos::input_method::InitializeForTesting(
-        new chromeos::input_method::MockInputMethodManagerImpl);
-#endif
-    omnibox_view_.reset(new TestingOmniboxViewViews(
-        &omnibox_edit_controller_, &profile_, &command_updater_));
-    test_api_.reset(new views::TextfieldTestApi(omnibox_view_.get()));
-    omnibox_view_->Init();
-  }
-
-  void TearDown() override {
-    omnibox_view_.reset();
-#if defined(OS_CHROMEOS)
-    chromeos::input_method::Shutdown();
-#endif
-  }
+  void SetUp() override;
+  void TearDown() override;
 
   content::TestBrowserThreadBundle thread_bundle_;
   TestingProfile profile_;
   CommandUpdater command_updater_;
   TestingOmniboxEditController omnibox_edit_controller_;
-  std::unique_ptr<TestingOmniboxViewViews> omnibox_view_;
+  std::unique_ptr<TestingOmniboxView> omnibox_view_;
   std::unique_ptr<views::TextfieldTestApi> test_api_;
 
   DISALLOW_COPY_AND_ASSIGN(OmniboxViewViewsTest);
 };
 
+OmniboxViewViewsTest::OmniboxViewViewsTest()
+    : command_updater_(nullptr), omnibox_edit_controller_(&command_updater_) {}
+
+void OmniboxViewViewsTest::SetAndEmphasizeText(const std::string& new_text) {
+  omnibox_view()->ResetEmphasisTestState();
+  omnibox_view()->SetText(base::ASCIIToUTF16(new_text));
+  omnibox_view()->EmphasizeURLComponents();
+}
+
+void OmniboxViewViewsTest::SetUp() {
+#if defined(OS_CHROMEOS)
+  chromeos::input_method::InitializeForTesting(
+      new chromeos::input_method::MockInputMethodManagerImpl);
+#endif
+  omnibox_view_ = base::MakeUnique<TestingOmniboxView>(
+      &omnibox_edit_controller_, &profile_, &command_updater_);
+  test_api_ = base::MakeUnique<views::TextfieldTestApi>(omnibox_view_.get());
+  omnibox_view_->Init();
+}
+
+void OmniboxViewViewsTest::TearDown() {
+  omnibox_view_.reset();
+#if defined(OS_CHROMEOS)
+  chromeos::input_method::Shutdown();
+#endif
+}
+
+// Actual tests ---------------------------------------------------------------
+
 // Checks that a single change of the text in the omnibox invokes
 // only one call to OmniboxViewViews::UpdatePopup().
 TEST_F(OmniboxViewViewsTest, UpdatePopupCall) {
@@ -186,20 +229,20 @@ TEST_F(OmniboxViewViewsTest, UpdatePopupCall) {
                           ui::DomKey::FromCharacter('a'),
                           ui::EventTimeForNow());
   omnibox_textfield()->InsertChar(char_event);
-  omnibox_view()->CheckUpdatePopupCallInfo(
-      1, base::ASCIIToUTF16("a"), gfx::Range(1));
+  omnibox_view()->CheckUpdatePopupCallInfo(1, base::ASCIIToUTF16("a"),
+                                           Range(1));
 
   char_event =
       ui::KeyEvent(ui::ET_KEY_PRESSED, ui::VKEY_B, ui::DomCode::US_B, 0,
                    ui::DomKey::FromCharacter('b'), ui::EventTimeForNow());
   omnibox_textfield()->InsertChar(char_event);
-  omnibox_view()->CheckUpdatePopupCallInfo(
-      2, base::ASCIIToUTF16("ab"), gfx::Range(2));
+  omnibox_view()->CheckUpdatePopupCallInfo(2, base::ASCIIToUTF16("ab"),
+                                           Range(2));
 
   ui::KeyEvent pressed(ui::ET_KEY_PRESSED, ui::VKEY_BACK, 0);
   omnibox_textfield()->OnKeyEvent(&pressed);
-  omnibox_view()->CheckUpdatePopupCallInfo(
-      3, base::ASCIIToUTF16("a"), gfx::Range(1));
+  omnibox_view()->CheckUpdatePopupCallInfo(3, base::ASCIIToUTF16("a"),
+                                           Range(1));
 }
 
 // Test that the scheduled text edit command is cleared when Textfield receives
@@ -216,67 +259,35 @@ TEST_F(OmniboxViewViewsTest, ScheduledTextEditCommand) {
             scheduled_text_edit_command());
 }
 
-// Ensure that the scheme is emphasized for data: URLs.
-TEST_F(OmniboxViewViewsTest, TestEmphasisForDATA) {
-  SetAndEmphasizeText("data:text/html,Hello%20World");
-  EXPECT_EQ(gfx::Range(0, 4), omnibox_view()->scheme_range());
-  EXPECT_FALSE(omnibox_view()->base_text_is_emphasized());
-  EXPECT_EQ(gfx::Range(0, 4), omnibox_view()->emphasis_range());
-}
-
-// Ensure that the origin is emphasized for http: URLs.
-TEST_F(OmniboxViewViewsTest, TestEmphasisForHTTP) {
-  SetAndEmphasizeText("http://www.example.com/path/file.htm");
-  EXPECT_EQ(gfx::Range(0, 4), omnibox_view()->scheme_range());
-  EXPECT_FALSE(omnibox_view()->base_text_is_emphasized());
-  EXPECT_EQ(gfx::Range(7, 22), omnibox_view()->emphasis_range());
-}
-
-// Ensure that the origin is emphasized for https: URLs.
-TEST_F(OmniboxViewViewsTest, TestEmphasisForHTTPS) {
-  SetAndEmphasizeText("https://www.example.com/path/file.htm");
-  EXPECT_EQ(gfx::Range(0, 5), omnibox_view()->scheme_range());
-  EXPECT_FALSE(omnibox_view()->base_text_is_emphasized());
-  EXPECT_EQ(gfx::Range(8, 23), omnibox_view()->emphasis_range());
-}
-
-// Ensure that nothing is emphasized for chrome-extension: URLs.
-TEST_F(OmniboxViewViewsTest, TestEmphasisForChromeExtensionScheme) {
-  SetAndEmphasizeText("chrome-extension://ldfbacdbackkjhclmhnjabngnppnkagl");
-  EXPECT_EQ(gfx::Range(0, 16), omnibox_view()->scheme_range());
-  EXPECT_FALSE(omnibox_view()->base_text_is_emphasized());
-  EXPECT_EQ(gfx::Range(), omnibox_view()->emphasis_range());
-}
-
-// Ensure that everything is emphasized for unknown scheme hierarchical URLs.
-TEST_F(OmniboxViewViewsTest, TestEmphasisForUnknownHierarchicalScheme) {
-  SetAndEmphasizeText("nosuchscheme://opaque/string");
-  EXPECT_EQ(gfx::Range(0, 12), omnibox_view()->scheme_range());
-  EXPECT_TRUE(omnibox_view()->base_text_is_emphasized());
-  EXPECT_EQ(gfx::Range(), omnibox_view()->emphasis_range());
-}
-
-// Ensure that everything is emphasized for unknown scheme URLs.
-TEST_F(OmniboxViewViewsTest, TestEmphasisForUnknownScheme) {
-  SetAndEmphasizeText("nosuchscheme:opaquestring");
-  EXPECT_EQ(gfx::Range(0, 12), omnibox_view()->scheme_range());
-  EXPECT_TRUE(omnibox_view()->base_text_is_emphasized());
-  EXPECT_EQ(gfx::Range(), omnibox_view()->emphasis_range());
-}
-
-// Ensure that the origin is emphasized for URL-like text.
-TEST_F(OmniboxViewViewsTest, TestEmphasisForPartialURLs) {
-  SetAndEmphasizeText("example/path/file");
-  EXPECT_EQ(gfx::Range(), omnibox_view()->scheme_range());
-  EXPECT_FALSE(omnibox_view()->base_text_is_emphasized());
-  EXPECT_EQ(gfx::Range(0, 7), omnibox_view()->emphasis_range());
-}
-
-// Ensure that everything is emphasized for plain text.
-TEST_F(OmniboxViewViewsTest, TestEmphasisForNonURLs) {
-  SetAndEmphasizeText("This is plain text");
-
-  EXPECT_EQ(gfx::Range(), omnibox_view()->scheme_range());
-  EXPECT_TRUE(omnibox_view()->base_text_is_emphasized());
-  EXPECT_EQ(gfx::Range(), omnibox_view()->emphasis_range());
+TEST_F(OmniboxViewViewsTest, Emphasis) {
+  const struct {
+    const char* input;
+    bool base_text_emphasized;

[Mark P, 2017/03/07 21:10:18]

nit: consider expected_ as appropriate to distinguish input from output.

[Peter Kasting, 2017/03/08 10:57:31]

Done.

+    Range emphasis_range;
+    Range scheme_range;
+  } test_cases[] = {
+      {"data:text/html,Hello%20World", false, Range(0, 4), Range(0, 4)},
+      {"http://www.example.com/path/file.htm", false, Range(7, 22),
+       Range(0, 4)},
+      {"https://www.example.com/path/file.htm", false, Range(8, 23),
+       Range(0, 5)},
+      {"chrome-extension://ldfbacdbackkjhclmhnjabngnppnkagl", false,
+       Range::InvalidRange(), Range(0, 16)},
+      {"nosuchscheme://opaque/string", true, Range::InvalidRange(),
+       Range(0, 12)},
+      {"nosuchscheme:opaquestring", true, Range::InvalidRange(), Range(0, 12)},
+      {"host.com/path/file", false, Range(0, 8), Range::InvalidRange()},
+      {"This is plain text", true, Range::InvalidRange(),
+       Range::InvalidRange()},
+  };
+
+  for (const auto& test_case : test_cases) {
+    SCOPED_TRACE(test_case.input);
+
+    SetAndEmphasizeText(test_case.input);
+    EXPECT_EQ(TestingOmniboxView::to_emphasis(test_case.base_text_emphasized),
+              omnibox_view()->base_text_emphasis());
+    EXPECT_EQ(test_case.emphasis_range, omnibox_view()->emphasis_range());
+    EXPECT_EQ(test_case.scheme_range, omnibox_view()->scheme_range());
+  }
 }