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

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

Index: chrome/browser/ui/views/omnibox/omnibox_view_views.cc
diff --git a/chrome/browser/ui/views/omnibox/omnibox_view_views.cc b/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
index 0f22b6ee6e9593c66759e2e7ab82f406c59e5b0f..3187232839db9b3b9ae7ea6c6adca5f04814a025 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
@@ -586,14 +586,27 @@ void OmniboxViewViews::EmphasizeURLComponents() {
   url::Component scheme, host;
   AutocompleteInput::ParseForEmphasizeComponents(
       text(), ChromeAutocompleteSchemeClassifier(profile_), &scheme, &host);
-  bool grey_out_url = text().substr(scheme.begin, scheme.len) ==
-      base::UTF8ToUTF16(extensions::kExtensionScheme);
+
+  const base::string16 url_scheme = text().substr(scheme.begin, scheme.len);
+
+  // Is the scheme one that might be abused for spoofing purposes?

[Peter Kasting, 2017/02/06 22:42:28]

ALl this code feels a little piecemeal to me; the comments explain very locally
what individual temps mean, but there's not much holistic picture of what we're
trying to do, so it feels a bit error-prone for future modification.

I tried rewriting with better comments and (possibly) structure by using a local
enum.  See what you think:

  // When possible, de-emphasize parts of the URL to draw attention to
  // whatever the best proxy is for the "identity" of the URL.
  enum Deemphasize {
    EVERYTHING,
    ALL_BUT_SCHEME,
    ALL_BUT_HOST,
    NOTHING,
  } deemphasize = NOTHING;
  if (text_is_url) {
    // Extension IDs are not human-readable, so de-emphasize everything to draw
    // attention to the human-readable name in the location icon text.
    if (url_scheme == base::UTF8ToUTF16(extensions::kExtensionScheme))
      deemphasize = EVERYTHING;
    // 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".
    else if (url_scheme == base::UTF8ToUTF16(url::kDataScheme))
      deemphasize = ALL_BUT_SCHEME;
    // For normal URLs, the host is the best proxy for "identity".
    else if (host.is_nonempty())
      deemphasize = ALL_BUT_HOST;
  }

  const SkColor color = location_bar_view_->GetColor(LocationBarView::TEXT);
  SetColor(deemphasize == NOTHING
               ? text
               : location_bar_view_->GetColor(DEEMPHASIZED_TEXT));
  if (deemphasize == ALL_BUT_SCHEME)
    ApplyColor(color, gfx::Range(scheme.begin, scheme.end()));
  else if (deemphasize == ALL_BUT_HOST)
    ApplyColor(color, gfx::Range(host.begin, host.end()));

There are still problems in this function -- SetDirectionalityMode() doesn't
belong in a function called EmphasizeURLComponents() (we should probably rename
the function), the comments in the blocks above this are rather misplaced (e.g.
by talking about "ask the model" in the block after the one that asks the
model), etc. -- but those don't have to be fixed in this CL.

+  bool spoofy_scheme = url_scheme == base::UTF8ToUTF16(url::kDataScheme);

[Peter Kasting, 2017/02/06 22:42:28]

Should we just go ahead and include blob: in this?

+
+  // Should the entire URL be deemphasized?
+  bool grey_out_url =
+      (spoofy_scheme ||
+       url_scheme == base::UTF8ToUTF16(extensions::kExtensionScheme));
+
   bool grey_base = text_is_url && (host.is_nonempty() || grey_out_url);
+

[Peter Kasting, 2017/02/06 22:42:28]

Nit: I wouldn't add this

   SetColor(location_bar_view_->GetColor(
       grey_base ? LocationBarView::DEEMPHASIZED_TEXT : LocationBarView::TEXT));
   if (grey_base && !grey_out_url) {
     ApplyColor(location_bar_view_->GetColor(LocationBarView::TEXT),
                gfx::Range(host.begin, host.end()));
+  } else if (spoofy_scheme) {
+    ApplyColor(location_bar_view_->GetColor(LocationBarView::TEXT),
+               gfx::Range(scheme.begin, scheme.end()));
   }
 
   // Emphasize the scheme for security UI display purposes (if necessary).