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

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

 // Copyright (c) 2012 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 <set>
 
 #include "base/command_line.h"
 #include "base/logging.h"
@@ skipping 568 matching lines @@
 
   // See whether the contents are a URL with a non-empty host portion, which we
   // should emphasize.  To check for a URL, rather than using the type returned
   // by Parse(), ask the model, which will check the desired page transition for
   // this input.  This can tell us whether an UNKNOWN input string is going to
   // be treated as a search or a navigation, and is the same method the Paste
   // And Go system uses.
   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).
   // 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.
   SetStyle(gfx::DIAGONAL_STRIKE, false);
   if (!model()->user_input_in_progress() && text_is_url &&
       scheme.is_nonempty() && (security_level_ != security_state::NONE)) {
@@ skipping 458 matching lines @@
       paste_position + 1, IDS_PASTE_AND_GO, IDS_PASTE_AND_GO);
 
   menu_contents->AddSeparator(ui::NORMAL_SEPARATOR);
 
   // Minor note: We use IDC_ for command id here while the underlying textfield
   // is using IDS_ for all its command ids. This is because views cannot depend
   // on IDC_ for now.
   menu_contents->AddItemWithStringId(IDC_EDIT_SEARCH_ENGINES,
       IDS_EDIT_SEARCH_ENGINES);
 }