Create FormatUrlForSecurityDisplay helper function.

chrome/browser/ui/elide_url.cc

Index: chrome/browser/ui/elide_url.cc
diff --git a/chrome/browser/ui/elide_url.cc b/chrome/browser/ui/elide_url.cc
index 88b393c4165555de6cbc73e7e4021ae319e65916..9254aaf447a0db73cc97bc129676cd38d913ff9b 100644
--- a/chrome/browser/ui/elide_url.cc
+++ b/chrome/browser/ui/elide_url.cc
@@ -301,3 +301,48 @@ base::string16 ElideHost(const GURL& url,
       url_subdomain, font_list, subdomain_width, gfx::ELIDE_HEAD);
   return elided_subdomain + url_domain;
 }
+
+base::string16 FormatOriginForDisplay(const GURL& url,
+                                      const std::string& languages,
+                                      bool omit_scheme) {
+  if (!url.IsStandard() || url.is_empty() || !url.is_valid())

[Ryan Sleevi, 2015/05/29 18:28:31]

BUG: You want to reverse the order of these checks.

While arguably an API wart, IsStandard() is only safe to be called if is_valid()
is true. You can see this in the //url implementation, and in those that use
//url [Yes, it's "weird" that GURL can have partially initialized state - this
is the grand debate about Factory Functions vs Constructors, for which
reasonable arguments exist for both]

Because IsStandard() requires a scheme component to be parsed, and that won't be
true for a !is_valid() URL (canonical example being "GURL url;"), you want to
work through each of these checks

if (!url.is_valid() || url.is_empty() || !url.IsStandard())
  return ...

[palmer, 2015/05/29 20:40:10]

Done.

+    return net::FormatUrl(url, languages);
+
+  if (url.SchemeIsFile()) {
+    return (omit_scheme ? base::string16() : base::ASCIIToUTF16("file://")) +
+           base::UTF8ToUTF16(url.path());

[Ryan Sleevi, 2015/05/29 18:28:31]

So this is a little weird for a few reasons

As crazy as it may sound, file URLs can be interpreted as having a host portion
(e.g. a UNC path can be parsed as a host+path), and this would make them appear
as local files

Off the top-of-my-head, I'm not quite sure what you want to happen, so I'm not
quite sure what to suggest here, other than to point out the host is relevant.

[palmer, 2015/05/29 20:40:10]

Although we humans know that (imagine Windows shares here)
\\CONTOSO\someshare\goat.gif is potentially a different resource than
\\127.0.0.1\someshare\goat.gif, I think it's the case that Chrome treats only
the pathname as the origin. (Not that GURL::GetOrigin agrees...)

I'm happy to change the code to do whatever the right thing is. But if the
hostname does not change the origin of 2 otherwise identical file: URLs, then I
think we should not display it.

[brettw, 2015/05/29 20:43:08]

If true, this seems like a big bug.

+  }
+
+  if (url.SchemeIsFileSystem()) {
+    const GURL* inner_url = url.inner_url();
+    const GURL& inner_origin = inner_url->GetOrigin();
+    if (inner_origin.SchemeIsFile()) {
+      return base::ASCIIToUTF16("filesystem:") +
+             (omit_scheme ? base::string16() : base::ASCIIToUTF16("file://")) +
+             base::UTF8ToUTF16(inner_url->path()) +

[Ryan Sleevi, 2015/05/29 18:28:31]

Does it make sense to recurse in to this function for inner_url? It seems like
you're duplicating the logic of these two lines and 312/313, and you'll want
them to stay in sync?

Only if and when you need to distinguish these two would it seem good to break
this out.

[palmer, 2015/05/29 20:40:10]

I don't think so; not sure. inner_url behaves extremely oddly. This is the best
I could do — short of string bashing, which you liked even less.

+             base::UTF8ToUTF16(url.path());
+    }
+    return base::ASCIIToUTF16("filesystem:") +

[Ryan Sleevi, 2015/05/29 18:28:31]

I'd recommend you replace all of these constants with the ones from
//url/url_constants.h

Beyond being existing constants (for example, you've duplicated "filesystem" and
"file" here a few times in this function), it also gives the compiler enough
grace to know these are intentionally duplicate constants, rather than hoping
the linker will fold them with string constant reduction (which it wouldn't be
able to do across translation units in a shared library build, but it would be
able to do if you used the constants)

[palmer, 2015/05/29 20:40:10]

Done.

+           FormatOriginForDisplay(inner_origin, languages, omit_scheme);
+  }
+
+  const GURL origin = url.GetOrigin();
+  const std::string& scheme = origin.scheme();
+  const std::string& host = origin.host();
+  if (scheme.empty() || host.empty())

[Ryan Sleevi, 2015/05/29 18:28:31]

Perhaps you could expand a comment about why this is falling in to
net::FormatURL?

What are the situations where this could happen, and why is FormatUrl (which
would include the path, presumably) appropriate?

[palmer, 2015/05/29 20:40:10]

I was trying to be maximally defensive, but it turns out not to be necessary.
Removed this code.

+    return net::FormatUrl(url, languages);
+
+  base::string16 result;
+  if (!omit_scheme)
+    result = base::UTF8ToUTF16(scheme) + base::ASCIIToUTF16("://");

[Ryan Sleevi, 2015/05/29 18:28:31]

This is kStandardSchemeSeparator, fwiw

[palmer, 2015/05/29 20:40:10]

Done.

+
+  result += base::UTF8ToUTF16(host);
+
+  const int port = origin.IntPort();
+  const int default_port = url::DefaultPortForScheme(origin.scheme().c_str(),
+                                                     origin.scheme().length());
+  if (origin.port().length() > 0 && port != 0 && port != default_port)

[Ryan Sleevi, 2015/05/29 18:28:31]

This != 0 check seems odd. If the port isn't specified, the API contract is that
it returns PORT_UNSPECIFIED. Is that what you meant to check?

Also, it seems odd to check the .length() > 0 - is that short-hand for checking
port != PORT_UNSPECIFIED? It just seems like you're mixing two different API
contracts here

[palmer, 2015/05/29 20:40:10]

Just trying to be maximally defensive, again.

When given "https://173.194.65.103:000" (test case "Invalid port 1"), the port
is in fact 0. That test case is set to have the expected return value be
L"https://173.194.65.103"; should I change it to L"https://173.194.65.103:0"?

[Ryan Sleevi, 2015/05/29 21:13:50]

Yeah, I think :0 is correct, because that's actually a different origin than
"https://173.194.65.103" [0 is an invalid, out of range port - there's a whole
lot of URL spec angst going around how to handle that, but effectively, the URL
is invalid at that point, because :0 isn't a valid port. As such, it's good to
preserve the :0]

+    result += base::ASCIIToUTF16(":") + base::UTF8ToUTF16(origin.port());
+
+  return result;
+}