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

components/omnibox/browser/omnibox_view.cc

Index: components/omnibox/browser/omnibox_view.cc
diff --git a/components/omnibox/browser/omnibox_view.cc b/components/omnibox/browser/omnibox_view.cc
index c05c1aeb1de32f491c94849fc7931fe76bcbaacb..267c8b4439f4e8fe308aeab337d10e6a965f1b46 100644
--- a/components/omnibox/browser/omnibox_view.cc
+++ b/components/omnibox/browser/omnibox_view.cc
@@ -19,6 +19,7 @@
 #include "components/omnibox/browser/omnibox_edit_controller.h"
 #include "components/omnibox/browser/omnibox_edit_model.h"
 #include "components/toolbar/toolbar_model.h"
+#include "extensions/common/constants.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/vector_icons_public.h"
 
@@ -185,3 +186,72 @@ void OmniboxView::TextChanged() {
   if (model_.get())
     model_->OnChanged();
 }
+
+void OmniboxView::SetEmphasis(bool emphasize, gfx::Range) {
+  // No behavior if not overridden by subclasses.
+}
+
+void OmniboxView::UpdateSchemeEmphasis(gfx::Range) {
+  // No behavior if not overridden by subclasses.
+}
+
+void OmniboxView::ApplyEmphasis(
+    const base::string16& display_text,
+    const AutocompleteSchemeClassifier& classifier) {
+  enum DemphasizeComponents {
+    EVERYTHING,
+    ALL_BUT_SCHEME,
+    ALL_BUT_HOST,
+    NOTHING,
+  } deemphasize = NOTHING;
+
+  url::Component scheme, host;
+  AutocompleteInput::ParseForEmphasizeComponents(display_text, classifier,
+                                                 &scheme, &host);
+
+  const base::string16 url_scheme =

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

Nit: Move inside the next conditional

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

Done.

+      display_text.substr(scheme.begin, scheme.len);
+
+  if (model_->CurrentTextIsURL()) {
+    // Extension IDs are not human-readable, so deemphasize everything to draw
+    // attention to the human-readable name in the location icon text.
+    // Data URLs are rarely human-readable and can be used for spoofing, so draw
+    // attention to the scheme to emphasize "this is just a bunch of data".
+    // For normal URLs, the host is the best proxy for "identity".
+    if (url_scheme == base::UTF8ToUTF16(extensions::kExtensionScheme))
+      deemphasize = EVERYTHING;
+    else if (url_scheme == base::UTF8ToUTF16(url::kDataScheme))
+      deemphasize = ALL_BUT_SCHEME;
+    else if (host.is_nonempty())
+      deemphasize = ALL_BUT_HOST;
+  }
+
+  gfx::Range scheme_range = gfx::Range(scheme.begin, scheme.end());
+  switch (deemphasize) {
+    case EVERYTHING:
+      SetEmphasis(false, gfx::Range::InvalidRange());
+      break;
+    case NOTHING:
+      SetEmphasis(true, gfx::Range::InvalidRange());
+      break;
+    case ALL_BUT_SCHEME:
+      SetEmphasis(false, gfx::Range::InvalidRange());
+      SetEmphasis(true, scheme_range);
+      break;
+    case ALL_BUT_HOST:
+      SetEmphasis(false, gfx::Range::InvalidRange());
+      SetEmphasis(true, gfx::Range(host.begin, host.end()));
+      break;
+  }
+
+  // Emphasize the scheme for security UI display purposes (if necessary).
+  // Note that we check CurrentTextIsURL() because if we're replacing search
+  // URLs with search terms, we may have a non-URL even when the user is not
+  // editing; and in some cases, e.g. for "site:foo.com" searches, the parser
+  // may have incorrectly identified a qualifier as a scheme.
+  if (model()->user_input_in_progress() || !model_->CurrentTextIsURL() ||

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

I think we can nuke the CurrentTextIsURL() check and everything but the first
line of the above comment; it was only applicable for Search Term Replacement,
which is dead.

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

Interesting. Updated.

+      !scheme.is_nonempty()) {

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

Nit: {} unnecessary

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

Technically yes, although the style guide seems to allow it and Eugene asked for
{} elsewhere where the conditional test was multiple lines. Now that it's not, I
guess I'll drop them.

+    scheme_range = gfx::Range::InvalidRange();
+  }
+  UpdateSchemeEmphasis(scheme_range);

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

Do we always have to call this method?  It seems like it should only be
necessary if the range is nonempty, as long as in the views implementation we
clear the diagonal strike state before calling ApplyEmphasis() (which I think is
reasonable).

Then we could avoid checking range validity in the callers, and just do this
here:

  if (!model()->user_input_in_progress() && scheme_range.IsValid())
    UpdateSchemeEmphasis(scheme_range);

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

I'm inclined to leave this as it is, because it makes for a more clear
behavioral contract for subclasses-- we'll always call both virtual methods,
even when the scheme isn't present. As you noted, we could just change this and
update ViewViews to clear the strike state before asking for emphasis, but we'd
have to revise this if a subclass ever needed to change some state when the
scheme was removed. Any strong objections?

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

It really depends how one thinks about these methods.  Are they intended to set
state to specific values, or are they intended to be generic always-called hook
points that subclasses can use to do any twiddling they may or may not need to
do?

I made my suggestion because I think the former is clearer (call methods to have
them do a specific act, rather than to provide a hook at a specific control flow
point).  I think you're suggesting the latter.  Either route is arguably
reasonable.

I would say, consider in the abstract which route you think is easiest to name
and document the functions to reflect: "what does this do" versus "when is this
called"; and which will lead to the fewest bugs down the road.  Then go
whichever route those considerations lead you to.

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

Done.

+}