Show scheme in black and content in gray for data: protocol urls

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 99e1c1c9e30facdc57f328f9d7fb18cf20a9f772..bfd6d126d85462b6480702b03b2baac4a064bdbb 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc
@@ -10,6 +10,7 @@
 
 #include "base/macros.h"
 #include "build/build_config.h"
+#include "chrome/browser/autocomplete/chrome_autocomplete_scheme_classifier.h"
 #include "chrome/browser/command_updater.h"
 #include "chrome/browser/ui/omnibox/chrome_omnibox_edit_controller.h"
 #include "chrome/test/base/testing_profile.h"
@@ -32,10 +33,14 @@ class TestingOmniboxViewViews : public OmniboxViewViews {
   TestingOmniboxViewViews(OmniboxEditController* controller,
                           Profile* profile,
                           CommandUpdater* command_updater)
-      : OmniboxViewViews(controller, profile, command_updater, false, NULL,
+      : OmniboxViewViews(controller,
+                         profile,
+                         command_updater,
+                         false,
+                         NULL,

[Peter Kasting, 2017/03/01 02:39:11]

Nit: While here: nullptr

[elawrence, 2017/03/01 21:49:01]

Done.

                          gfx::FontList()),
-        update_popup_call_count_(0) {
-  }
+        update_popup_call_count_(0),
+        profile_(profile) {}
 
   void CheckUpdatePopupCallInfo(size_t call_count, const base::string16& text,
                                 const gfx::Range& selection_range) {
@@ -44,6 +49,30 @@ class TestingOmniboxViewViews : public OmniboxViewViews {
     EXPECT_EQ(selection_range, update_popup_selection_range_);
   }
 
+  void EmphasizeURLComponents() override {
+    ApplyEmphasis(text(), ChromeAutocompleteSchemeClassifier(profile_));
+  }
+
+  void SetEmphasis(bool emphasize, gfx::Range range) override {
+    if (range == gfx::Range::InvalidRange()) {
+      base_text_is_emphasized_ = emphasize;
+      return;
+    }
+
+    EXPECT_TRUE(emphasize);
+    emphasis_range_ = range;
+  }
+
+  void UpdateSchemeEmphasis(gfx::Range range) override {
+    scheme_range_ = range;
+  }
+
+  gfx::Range GetSchemeRange() { return scheme_range_; }

[Peter Kasting, 2017/03/01 02:39:11]

Nit: Next three can be const.

I suggest naming these like inlined accessors as well, e.g. "scheme_range()",
and omitting the blank lines between them.

[elawrence, 2017/03/01 21:49:01]

Done.

+
+  gfx::Range GetEmphasisRange() { return emphasis_range_; }
+
+  bool IsBaseTextEmphasized() { return base_text_is_emphasized_; }
+
  private:
   // OmniboxView:
   void UpdatePopup() override {
@@ -55,6 +84,16 @@ class TestingOmniboxViewViews : public OmniboxViewViews {
   size_t update_popup_call_count_;
   base::string16 update_popup_text_;
   gfx::Range update_popup_selection_range_;
+  Profile* profile_;
+
+  // Range of the last scheme logged by |UpdateSchemeEmphasis()|.
+  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);
 };
@@ -163,3 +202,81 @@ TEST_F(OmniboxViewViewsTest, ScheduledTextEditCommand) {
   EXPECT_EQ(ui::TextEditCommand::INVALID_COMMAND,
             scheduled_text_edit_command());
 }
+
+// Ensure only scheme is emphasized for data: URLs.
+TEST_F(OmniboxViewViewsTest, TestEmphasisForDATA) {
+  omnibox_textfield()->SetText(
+      base::ASCIIToUTF16("data:text/html,Hello%20World"));
+  omnibox_view()->EmphasizeURLComponents();
+  EXPECT_EQ(gfx::Range(0, 4), omnibox_view()->GetSchemeRange());
+  EXPECT_FALSE(omnibox_view()->IsBaseTextEmphasized());
+  EXPECT_EQ(gfx::Range(0, 4), omnibox_view()->GetEmphasisRange());
+}
+
+// Ensure only origin is emphasized for http: URLs.
+TEST_F(OmniboxViewViewsTest, TestEmphasisForHTTP) {
+  omnibox_textfield()->SetText(
+      base::ASCIIToUTF16("http://www.example.com/path/file.htm"));
+  omnibox_view()->EmphasizeURLComponents();
+  EXPECT_EQ(gfx::Range(0, 4), omnibox_view()->GetSchemeRange());
+  EXPECT_FALSE(omnibox_view()->IsBaseTextEmphasized());
+  EXPECT_EQ(gfx::Range(7, 22), omnibox_view()->GetEmphasisRange());
+}
+
+// Ensure only origin is emphasized for https: URLs.
+TEST_F(OmniboxViewViewsTest, TestEmphasisForHTTPS) {
+  omnibox_textfield()->SetText(
+      base::ASCIIToUTF16("https://www.example.com/path/file.htm"));
+  omnibox_view()->EmphasizeURLComponents();
+  EXPECT_EQ(gfx::Range(0, 5), omnibox_view()->GetSchemeRange());
+  EXPECT_FALSE(omnibox_view()->IsBaseTextEmphasized());
+  EXPECT_EQ(gfx::Range(8, 23), omnibox_view()->GetEmphasisRange());
+}
+
+// Ensure that nothing is emphasized for chrome-extension: URLs.
+TEST_F(OmniboxViewViewsTest, TestEmphasisForChromeExtensionScheme) {
+  omnibox_textfield()->SetText(base::ASCIIToUTF16(
+      "chrome-extension://ldfbacdbackkjhclmhnjabngnppnkagl"));
+  omnibox_view()->EmphasizeURLComponents();
+  EXPECT_EQ(gfx::Range(0, 16), omnibox_view()->GetSchemeRange());
+  EXPECT_FALSE(omnibox_view()->IsBaseTextEmphasized());
+  EXPECT_EQ(gfx::Range(), omnibox_view()->GetEmphasisRange());
+}
+
+// Ensure that everything is emphasized for unknown scheme heirarchical URLs.

[Peter Kasting, 2017/03/01 02:39:11]

Nit: hierarchical

[elawrence, 2017/03/01 21:49:01]

Done.

+TEST_F(OmniboxViewViewsTest, TestEmphasisForUnknownHierarchicalScheme) {
+  omnibox_textfield()->SetText(
+      base::ASCIIToUTF16("nosuchscheme://opaque/string"));
+  omnibox_view()->EmphasizeURLComponents();
+  EXPECT_EQ(gfx::Range(0, 12), omnibox_view()->GetSchemeRange());
+  EXPECT_TRUE(omnibox_view()->IsBaseTextEmphasized());
+  EXPECT_EQ(gfx::Range(), omnibox_view()->GetEmphasisRange());
+}
+
+// Ensure that everything is emphasized for unknown scheme URLs.
+TEST_F(OmniboxViewViewsTest, TestEmphasisForUnknownScheme) {
+  omnibox_textfield()->SetText(base::ASCIIToUTF16("nosuchscheme:opaquestring"));
+  omnibox_view()->EmphasizeURLComponents();
+  EXPECT_EQ(gfx::Range(0, 12), omnibox_view()->GetSchemeRange());
+  EXPECT_TRUE(omnibox_view()->IsBaseTextEmphasized());
+  EXPECT_EQ(gfx::Range(), omnibox_view()->GetEmphasisRange());
+}
+
+// Ensure that Origin is emphasized for URL-like text.

[Peter Kasting, 2017/03/01 02:39:10]

Nit: Origin -> the origin

[elawrence, 2017/03/01 21:49:01]

Done.

+TEST_F(OmniboxViewViewsTest, TestEmphasisForPartialURLs) {
+  omnibox_textfield()->SetText(base::ASCIIToUTF16("example/path/file"));
+  omnibox_view()->EmphasizeURLComponents();
+  EXPECT_EQ(gfx::Range::InvalidRange(), omnibox_view()->GetSchemeRange());
+  EXPECT_FALSE(omnibox_view()->IsBaseTextEmphasized());
+  EXPECT_EQ(gfx::Range(0, 7), omnibox_view()->GetEmphasisRange());
+}
+
+// Non-URL text gets treated as a URL where the entire string is interpreted
+// as a hostname.

[Peter Kasting, 2017/03/01 02:39:10]

That doesn't sound right.  CurrentTextIsURL() ought to be false for this,
leading us to emphasize the whole string (rather than a range that includes the
whole string), I'd think.

[elawrence, 2017/03/01 21:49:01]

The problem is that OmniboxEditModel::CurrentTextIsURL() returns true if
(!user_input_in_progress_). CurrentTextIsURL() isn't virtual, and even if I
expose tweak the TestingOmniboxEditController so that calling SetInputInProgress
doesn't crash, CurrentTextIsURL ends up crashing inside a call to
TemplateURLService::Load from KeywordProvider::GetTemplateURLService.

For now, I've just removed this test.

[Peter Kasting, 2017/03/01 23:18:47]

That's because you're calling SetText() in all of these, which is really a
low-level Textfield function rather than an OmniboxView one.  I would attempt to
use omnibox_view()->SetUserText() instead, which is the intended API to change
the text in this control.  That's a suggestion not just for this test, but all
the ones above.

This may still cause problems if this test doesn't instantiate enough objects
(as mocks/fakes) to avoid crashing today.  My hope is that this would be fairly
easily solved, especially since the test base is already instantiating a
TestingProfile and such.

[elawrence, 2017/03/01 23:50:53]

Yes, SetUserText() code path that leads to the described crashes. I didn't spend
a lot of time looking into how to make the TemplateURLService call not crash,
but it doesn't seem like the OmniBoxEditController is well set up for
unittesting today, and it's not clear that a security fix should block on an
broader rearchitecture. 

[elawrence, 2017/03/02 19:08:01]

Exposing CurrentTextIsURL as a virtual function on the View and checking that is
the only reasonable path I've found for unblocking the unit test. 

+TEST_F(OmniboxViewViewsTest, TestEmphasisForNonURLs) {
+  omnibox_textfield()->SetText(base::ASCIIToUTF16("This is plain text"));
+  omnibox_view()->EmphasizeURLComponents();
+  EXPECT_EQ(gfx::Range::InvalidRange(), omnibox_view()->GetSchemeRange());
+  EXPECT_FALSE(omnibox_view()->IsBaseTextEmphasized());
+  EXPECT_EQ(gfx::Range(0, 18), omnibox_view()->GetEmphasisRange());
+}