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..d79e301b1f13f98aef1ea01966f6dcbc31fc1b67 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc
@@ -28,57 +28,30 @@
 
 namespace {
 
+// TestingOmniboxViewViews ----------------------------------------------------
+
 class TestingOmniboxViewViews : 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_);
-  }
+                          CommandUpdater* command_updater);
 
-  void EmphasizeURLComponents() override {
-    UpdateTextStyle(text(), ChromeAutocompleteSchemeClassifier(profile_));
-  }
+  void CheckUpdatePopupCallInfo(size_t call_count,

[Mark P, 2017/03/06 05:23:26]

pref inline

[Peter Kasting, 2017/03/06 21:10:44]

I'd like to avoid inlining methods where the body is several lines.  Same reply
on UpdatePopup() below.

+                                const base::string16& text,
+                                const gfx::Range& selection_range);
 
   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_; }
 
- private:
-  // OmniboxView:
-  void UpdatePopup() override {
-    ++update_popup_call_count_;
-    update_popup_text_ = text();
-    update_popup_selection_range_ = GetSelectedRange();
-  }
-
-  void SetEmphasis(bool emphasize, const gfx::Range& range) override {
-    if (range == gfx::Range::InvalidRange()) {
-      base_text_is_emphasized_ = emphasize;
-      return;
-    }
+  // OmniboxViewViews:
+  void EmphasizeURLComponents() override;

[Mark P, 2017/03/06 05:23:26]

pref inline

[Peter Kasting, 2017/03/06 21:10:44]

I think this one makes sense, and the only reason I'm not doing it is because
after my upcoming CLs, there will be no other inline methods that aren't "cheap
accessors" and the like, so inlining this one would be inconsistent.

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

Can you give it a better name or comment then?
I would never guess EmphasizeURLComponents does something like UpdateTextStyle
on a variable called text.

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

Since this is a virtual override, the names are set by the base class, which
also gives the comments on what the functions generally do.  Based on those,
calling UpdateTextStyle() and passing in the current text of the control as the
displayed text make sense to me.  I dunno what to say here :(

I don't love this name for this function, honestly.  It's a bit overly-specific;
really the names of these two methods should perhaps be swapped.  However, I
think making API changes to the core OmniboxView class is out of scope for this
CL.  If you feel strongly that this could be improved in some way (I don't feel
strongly enough to do something about it myself), file a bug against me with
whatever suggestion you might have, and I can try to implement it in a followup.

 
-    EXPECT_TRUE(emphasize);
-    emphasis_range_ = range;
-  }
-
-  void UpdateSchemeStyle(const gfx::Range& range) override {
-    scheme_range_ = range;
-  }
+ private:
+  // OmniboxViewViews:
+  void UpdatePopup() override;

[Mark P, 2017/03/06 05:23:26]

pref inline

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

The behavior is not what I'd expect an UpdatePopup() call to do, that's why I
suggested it should be inline.  If not inline, it should be commented.

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

Done.

+  void SetEmphasis(bool emphasize, const gfx::Range& range) override;
+  void UpdateSchemeStyle(const gfx::Range& range) override;
 
   // Simplistic test override returns whether a given string looks like a URL
   // without having to mock AutocompleteClassifier objects and their
@@ -106,12 +79,62 @@ class TestingOmniboxViewViews : public OmniboxViewViews {
   DISALLOW_COPY_AND_ASSIGN(TestingOmniboxViewViews);
 };
 
+TestingOmniboxViewViews::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 TestingOmniboxViewViews::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_);
+}
+
+void TestingOmniboxViewViews::EmphasizeURLComponents() {
+  UpdateTextStyle(text(), ChromeAutocompleteSchemeClassifier(profile_));
+}
+
+void TestingOmniboxViewViews::UpdatePopup() {
+  ++update_popup_call_count_;
+  update_popup_text_ = text();
+  update_popup_selection_range_ = GetSelectedRange();
+}
+
+void TestingOmniboxViewViews::SetEmphasis(bool emphasize,
+                                          const gfx::Range& range) {
+  if (range == gfx::Range::InvalidRange()) {
+    base_text_is_emphasized_ = emphasize;
+    return;
+  }
+
+  EXPECT_TRUE(emphasize);
+  emphasis_range_ = range;
+}
+
+void TestingOmniboxViewViews::UpdateSchemeStyle(const gfx::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,55 +142,30 @@ 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();
 
+  TestingOmniboxViewViews* 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 text in the omnibox.

[Mark P, 2017/03/06 05:23:26]

nit: ... and emphasizes it appropriately.

[Peter Kasting, 2017/03/06 21:10:44]

Done.

+  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_;
@@ -179,6 +177,34 @@ class OmniboxViewViewsTest : public testing::Test {
   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()->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<TestingOmniboxViewViews>(
+      &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) {