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

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

 // Copyright (c) 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/omnibox/omnibox_view_views.h"
 
 #include <stddef.h>
 
 #include <memory>
 
@@ skipping 10 matching lines @@
 #include "ui/events/keycodes/dom/dom_code.h"
 #include "ui/views/controls/textfield/textfield_test_api.h"
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/input_method/input_method_configuration.h"
 #include "chrome/browser/chromeos/input_method/mock_input_method_manager_impl.h"
 #endif
 
 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) {}
+                          CommandUpdater* command_updater);
 
-  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_);
-  }
-
-  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_; }
 
+  // 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.

+
  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;
-    }
-
-    EXPECT_TRUE(emphasize);
-    emphasis_range_ = range;
-  }
-
-  void UpdateSchemeStyle(const gfx::Range& range) override {
-    scheme_range_ = range;
-  }
+  // 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
   // dependencies.
   bool CurrentTextIsURL() override {
     bool looks_like_url = (text().find(':') != std::string::npos ||
                            text().find('/') != std::string::npos);
     return looks_like_url;
   }
 
   size_t update_popup_call_count_;
   base::string16 update_popup_text_;
   gfx::Range update_popup_selection_range_;
   Profile* profile_;

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

nit: prefer blank line above here, to separate testing / result variables with
context-type variables.

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

Ignoring this because I'm about to rip this variable out entirely in the next
CL.

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

Acknowledged.

 
   // Range of the last scheme logged by UpdateSchemeStyle().
   gfx::Range scheme_range_;
 
   // Range of the last text emphasized by SetEmphasis().
   gfx::Range emphasis_range_;
 
   // SetEmphasis() logs whether the base color of the text is emphasized.
   bool base_text_is_emphasized_;
 
   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 {}
   ToolbarModel* GetToolbarModel() override { return nullptr; }
   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_) {
-  }
+  OmniboxViewViewsTest();
 
-  TestingOmniboxViewViews* omnibox_view() {
-    return omnibox_view_.get();
-  }
-
-  views::Textfield* omnibox_textfield() {
-    return omnibox_view();
-  }
-
+  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_;
   CommandUpdater command_updater_;
   TestingOmniboxEditController omnibox_edit_controller_;
   std::unique_ptr<TestingOmniboxViewViews> 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()->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) {
   ui::KeyEvent char_event(ui::ET_KEY_PRESSED, ui::VKEY_A, ui::DomCode::US_A, 0,
                           ui::DomKey::FromCharacter('a'),
                           ui::EventTimeForNow());
   omnibox_textfield()->InsertChar(char_event);
   omnibox_view()->CheckUpdatePopupCallInfo(
       1, base::ASCIIToUTF16("a"), gfx::Range(1));
 
@@ skipping 81 matching lines @@
 }
 
 // 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());
 }