Create net::FormatOriginForDisplay helper function

Create net::FormatOriginForDisplay helper function

net::FormatOriginForDisplay provides a canonical, security-friendly
display for origins. It's meant to be used for permission strings and
other security UI.

BUG=402698

[felt, 2014-09-01 18:20:55]

felt@chromium.org changed reviewers:
+ palmer@chromium.org, rsleevi@chromium.org

[Ryan Sleevi, 2014-09-01 18:22:41]

rsleevi@chromium.org changed reviewers:
- palmer@chromium.org

[Ryan Sleevi, 2014-09-01 18:22:41]

Why is the port stripping necessary? Previously it was established that GURL
does this when canonicalizing.

From a layering perspective, putting //ui: things in //net (like elision of
URLs) has been a recipe of layering violations and bad dependencies. Can we
establish it somewhere higher in the dependency graph, using primitives from
//net? Why or why not?

[felt, 2014-09-01 18:23:40]

On 2014/09/01 18:20:55, felt wrote:
> mailto:felt@chromium.org changed reviewers:
> + mailto:palmer@chromium.org, mailto:rsleevi@chromium.org

palmer, rsleevi: please review

I added new flags to FormatUrlWithAdjustments, so the heavy lifting can be done
as string manipulations there. That way FormatOriginForDisplay doesn't need to
do expensive GURL manipulations or do GURL->string->GURL.

Also: this should be extended in the future to take a suggested_max_length so
that it can do eliding for the caller. But I am not doing that yet.

[felt, 2014-09-01 18:30:30]

On 2014/09/01 18:22:41, Ryan Sleevi wrote:
> Why is the port stripping necessary? Previously it was established that GURL
> does this when canonicalizing.

If you initialize it simply as GURL("https://www.google.com:443/"), it doesn't
strip the port. (See the test case, which fails without the port stripping
logic.) I could add a call to url::Canonicalize if that's important.

> From a layering perspective, putting //ui: things in //net (like elision of
> URLs) has been a recipe of layering violations and bad dependencies. Can we
> establish it somewhere higher in the dependency graph, using primitives from
> //net? Why or why not?

I put it in net so that it can be right next to the very similar FormatUrl,
which also notes in its comment that it's meant for displaying URLs to the user.

[felt, 2014-09-01 18:32:15]

On 2014/09/01 18:30:30, felt wrote:
> On 2014/09/01 18:22:41, Ryan Sleevi wrote:
> > Why is the port stripping necessary? Previously it was established that GURL
> > does this when canonicalizing.
> 
> If you initialize it simply as GURL("https://www.google.com:443/"), it doesn't
> strip the port. (See the test case, which fails without the port stripping
> logic.) I could add a call to url::Canonicalize if that's important.

JK the GURL constructor is already canonicalizing it, so it is not removing the
port.

> 
> > From a layering perspective, putting //ui: things in //net (like elision of
> > URLs) has been a recipe of layering violations and bad dependencies. Can we
> > establish it somewhere higher in the dependency graph, using primitives from
> > //net? Why or why not?
> 
> I put it in net so that it can be right next to the very similar FormatUrl,
> which also notes in its comment that it's meant for displaying URLs to the
user.

[Ryan Sleevi, 2014-09-01 18:35:35]

On 2014/09/01 18:30:30, felt wrote:
> On 2014/09/01 18:22:41, Ryan Sleevi wrote:
> > Why is the port stripping necessary? Previously it was established that GURL
> > does this when canonicalizing.
> 
> If you initialize it simply as GURL("https://www.google.com:443/"), it doesn't
> strip the port. (See the test case, which fails without the port stripping
> logic.) I could add a call to url::Canonicalize if that's important.
> 
> > From a layering perspective, putting //ui: things in //net (like elision of
> > URLs) has been a recipe of layering violations and bad dependencies. Can we
> > establish it somewhere higher in the dependency graph, using primitives from
> > //net? Why or why not?
> 
> I put it in net so that it can be right next to the very similar FormatUrl,
> which also notes in its comment that it's meant for displaying URLs to the
user.

FormatURL isn't exclusively limited to UX, and your comment made me concerned
(re: eliding).

My question would be why can't this be composed, in //content or, better, //ui,
over existing FormatURL primitives?

I'm not saying that there may not be good justification, but the initial
reaction towards anything UI specific in //net is strongly negative (e.g. we had
the bloody //GPU target requiring V8, due to a transitive dependency on //net.
That's crazy!)

[felt, 2014-09-01 18:40:13]

On 2014/09/01 18:35:35, Ryan Sleevi wrote:
> On 2014/09/01 18:30:30, felt wrote:
> > On 2014/09/01 18:22:41, Ryan Sleevi wrote:
> > > Why is the port stripping necessary? Previously it was established that
GURL
> > > does this when canonicalizing.
> > 
> > If you initialize it simply as GURL("https://www.google.com:443/"), it
doesn't
> > strip the port. (See the test case, which fails without the port stripping
> > logic.) I could add a call to url::Canonicalize if that's important.
> > 
> > > From a layering perspective, putting //ui: things in //net (like elision
of
> > > URLs) has been a recipe of layering violations and bad dependencies. Can
we
> > > establish it somewhere higher in the dependency graph, using primitives
from
> > > //net? Why or why not?
> > 
> > I put it in net so that it can be right next to the very similar FormatUrl,
> > which also notes in its comment that it's meant for displaying URLs to the
> user.
> 
> FormatURL isn't exclusively limited to UX, and your comment made me concerned
> (re: eliding).
> 
> My question would be why can't this be composed, in //content or, better,
//ui,
> over existing FormatURL primitives?
> 
> I'm not saying that there may not be good justification, but the initial
> reaction towards anything UI specific in //net is strongly negative (e.g. we
had
> the bloody //GPU target requiring V8, due to a transitive dependency on //net.
> That's crazy!)

FormatURL already has a variant that is intended solely for UX (it strips http
schemes and has a cautionary comment saying not to use it for anything else). My
new method is very similar to that, which is why this is a pretty small CL -- I
just had to tweak the support already in place for that existing method.

I could move both if you'd like, but it seems odd for FormatURL to have one
wrapper for display here and another elsewhere.

[felt, 2014-09-01 18:40:14]

On 2014/09/01 18:35:35, Ryan Sleevi wrote:
> On 2014/09/01 18:30:30, felt wrote:
> > On 2014/09/01 18:22:41, Ryan Sleevi wrote:
> > > Why is the port stripping necessary? Previously it was established that
GURL
> > > does this when canonicalizing.
> > 
> > If you initialize it simply as GURL("https://www.google.com:443/"), it
doesn't
> > strip the port. (See the test case, which fails without the port stripping
> > logic.) I could add a call to url::Canonicalize if that's important.
> > 
> > > From a layering perspective, putting //ui: things in //net (like elision
of
> > > URLs) has been a recipe of layering violations and bad dependencies. Can
we
> > > establish it somewhere higher in the dependency graph, using primitives
from
> > > //net? Why or why not?
> > 
> > I put it in net so that it can be right next to the very similar FormatUrl,
> > which also notes in its comment that it's meant for displaying URLs to the
> user.
> 
> FormatURL isn't exclusively limited to UX, and your comment made me concerned
> (re: eliding).
> 
> My question would be why can't this be composed, in //content or, better,
//ui,
> over existing FormatURL primitives?
> 
> I'm not saying that there may not be good justification, but the initial
> reaction towards anything UI specific in //net is strongly negative (e.g. we
had
> the bloody //GPU target requiring V8, due to a transitive dependency on //net.
> That's crazy!)

FormatURL already has a variant that is intended solely for UX (it strips http
schemes and has a cautionary comment saying not to use it for anything else). My
new method is very similar to that, which is why this is a pretty small CL -- I
just had to tweak the support already in place for that existing method.

I could move both if you'd like, but it seems odd for FormatURL to have one
wrapper for display here and another elsewhere.

[Michael van Ouwerkerk, 2014-09-02 13:10:47]

https://codereview.chromium.org/532523002/diff/110001/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/532523002/diff/110001/net/base/net_util.h#new...
net/base/net_util.h:355: // - By default, the scheme is only displayed for
non-secure origins. This can
Why is https omitted by default? Haven't we been teaching users for years to
specifically look for https to check it's safe?

[felt, 2014-09-02 14:01:24]

On 2014/09/02 13:10:47, Michael van Ouwerkerk wrote:
> https://codereview.chromium.org/532523002/diff/110001/net/base/net_util.h
> File net/base/net_util.h (right):
> 
>
https://codereview.chromium.org/532523002/diff/110001/net/base/net_util.h#new...
> net/base/net_util.h:355: // - By default, the scheme is only displayed for
> non-secure origins. This can
> Why is https omitted by default? Haven't we been teaching users for years to
> specifically look for https to check it's safe?

That's a good question. I thought this might be controversial, and I debated it
a bit with myself.

Here's my logic for leaving off the scheme by default:

* The assumption is that this is a string on a page with a visible URL bar and
security indicator.
* If the feature is available only on secure origins (as new features with
security ramifications should be), then it isn't as important to show the
scheme.

But I could flip the meaning of the boolean to be hide_secure_scheme instead if
the above is crazy talk.

[Michael van Ouwerkerk, 2014-09-02 14:09:38]

On 2014/09/02 14:01:24, felt wrote:
> On 2014/09/02 13:10:47, Michael van Ouwerkerk wrote:
> > https://codereview.chromium.org/532523002/diff/110001/net/base/net_util.h
> > File net/base/net_util.h (right):
> > 
> >
>
https://codereview.chromium.org/532523002/diff/110001/net/base/net_util.h#new...
> > net/base/net_util.h:355: // - By default, the scheme is only displayed for
> > non-secure origins. This can
> > Why is https omitted by default? Haven't we been teaching users for years to
> > specifically look for https to check it's safe?
> 
> That's a good question. I thought this might be controversial, and I debated
it
> a bit with myself.
> 
> Here's my logic for leaving off the scheme by default:
> 
> * The assumption is that this is a string on a page with a visible URL bar and
> security indicator.
> * If the feature is available only on secure origins (as new features with
> security ramifications should be), then it isn't as important to show the
> scheme.
> 
> But I could flip the meaning of the boolean to be hide_secure_scheme instead
if
> the above is crazy talk.

I can't really tell whether it makes sense to omit when in context. I'm asking
just to make sure we document the reasoning behind it. What you wrote might be
sufficient documentation for now, but it kind of sounds like it needs to fit in
a larger policy/guideline.

[felt, 2014-09-02 14:14:18]

On 2014/09/02 14:09:38, Michael van Ouwerkerk wrote:
> On 2014/09/02 14:01:24, felt wrote:
> > On 2014/09/02 13:10:47, Michael van Ouwerkerk wrote:
> > > https://codereview.chromium.org/532523002/diff/110001/net/base/net_util.h
> > > File net/base/net_util.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/532523002/diff/110001/net/base/net_util.h#new...
> > > net/base/net_util.h:355: // - By default, the scheme is only displayed for
> > > non-secure origins. This can
> > > Why is https omitted by default? Haven't we been teaching users for years
to
> > > specifically look for https to check it's safe?
> > 
> > That's a good question. I thought this might be controversial, and I debated
> it
> > a bit with myself.
> > 
> > Here's my logic for leaving off the scheme by default:
> > 
> > * The assumption is that this is a string on a page with a visible URL bar
and
> > security indicator.
> > * If the feature is available only on secure origins (as new features with
> > security ramifications should be), then it isn't as important to show the
> > scheme.
> > 
> > But I could flip the meaning of the boolean to be hide_secure_scheme instead
> if
> > the above is crazy talk.
> 
> I can't really tell whether it makes sense to omit when in context. I'm asking
> just to make sure we document the reasoning behind it. What you wrote might be
> sufficient documentation for now, but it kind of sounds like it needs to fit
in
> a larger policy/guideline.

palmer, what do you think the default should be?

[palmer, 2014-09-02 19:11:49]

I think we should show not just text, but some kind of View object (call it an
OriginView) that shows a status icon (Good, Dubious, Bad — possibly no icon for
Good), the hostname and port of the origin, and a green, yellow, or red border
(for Good, Dubious, Bad, respectively) around the hostname and port. Then we
should use this OriginView object everywhere.

You can see me attempting to toy around here:
https://codereview.chromium.org/424383002/

If there are contexts where we can't show an OriginView for whatever reason —
logs, some graphical UI context that is too limited for some reason — then we
should use a plain unicode text representation that shows the scheme and host
(and, if non-standard for the scheme, the port).

I think we should wait on letting the scheme for HTTPS show as blank, until
we've achieved more of our long-term mission of normalizing HTTPS.

Random idea, take it with a grain of salt: We could also consider showing
textual origins (i.e. when we can't use OriginView) like this:

    (secure, mail.google.com)
    (secure, noodles.org, 8443)
    (not secure, pumpkins.co.jp)
    (not secure, lasers.int, 8080)

I.e. as tuples instead of serialized URLs, and saying "secure" and "not secure"
instead of the names of protocols that only developers know about.

[Ryan Sleevi, 2014-09-02 19:26:17]

Not trying to be a pain in the butt, still just pushing back to make sure this
is the right layer, conceptually, before digging into the low-level details.

If it seems like it's a huge pain to get something added to //net, well... yeah.
We've got a lot of technical debt, so I'm trying to make sure we're not
introducing more, by making sure we've got the right layering considered :)

https://codereview.chromium.org/532523002/diff/110001/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/532523002/diff/110001/net/base/net_util.h#new...
net/base/net_util.h:82: // after verifying that it is secure to do so. Not part
of kFormatUrlOmitAll.
This is... vaguely worded.

When is it ever secure to omit the port? Only when the port is a default port,
correct?

https://codereview.chromium.org/532523002/diff/110001/net/base/net_util.h#new...
net/base/net_util.h:276: // takes the same accept languages component as
ElideURL().
So, this is an example of the layering.

ElideURL was actually moved up to //chrome/browser/ui as the canonical place for
it (it used to be here in //net)

https://codereview.chromium.org/532523002/diff/110001/net/base/net_util.h#new...
net/base/net_util.h:350: // in security UI: infobars, permission strings, etc.
Use this instead of
So, to re-iterate the layering, //net has no idea about (infobars, permission
strings, etc). It's a conceptual separation in //net (that occasionally gets
punctured, and then I whine about it until someone fixes it or tries to do more
of the same, and then I whine until either they fix it or they call me on my
whining and I fix it)

Strictly from a code layout and layering point, what code potentially calls into
this? Is it //content? //chrome? //components? I would rather explore that, and
understand how the high level code would/is going to use this, and then figure
out the right layer.

Ideal world, this would live in //chrome/browser/ui, and just call FormatUrl
with some set of flags.

https://codereview.chromium.org/532523002/diff/110001/net/base/net_util_icu.cc
File net/base/net_util_icu.cc (right):

https://codereview.chromium.org/532523002/diff/110001/net/base/net_util_icu.c...
net/base/net_util_icu.cc:844: (display_origin.SchemeIs("https") &&
display_origin.port() == "443"))
We already have a //net concept of "default port for scheme", which is what I
raised in the previous review - see https://codereview.chromium.org/493253003/

[felt, 2014-09-15 05:42:30]

https://codereview.chromium.org/532523002/diff/110001/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/532523002/diff/110001/net/base/net_util.h#new...
net/base/net_util.h:350: // in security UI: infobars, permission strings, etc.
Use this instead of
On 2014/09/02 19:26:17, Ryan Sleevi wrote:
> So, to re-iterate the layering, //net has no idea about (infobars, permission
> strings, etc). It's a conceptual separation in //net (that occasionally gets
> punctured, and then I whine about it until someone fixes it or tries to do
more
> of the same, and then I whine until either they fix it or they call me on my
> whining and I fix it)
> 
> Strictly from a code layout and layering point, what code potentially calls
into
> this? Is it //content? //chrome? //components? I would rather explore that,
and
> understand how the high level code would/is going to use this, and then figure
> out the right layer.
> 
> Ideal world, this would live in //chrome/browser/ui, and just call FormatUrl
> with some set of flags.

Here are some files that should be refactored to call this proposed new method:
//chrome/browser/pepper_broker_infobar_delegate.cc
//chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc
//chrome/browser/ui/views/website_settings/permissions_bubble_view.cc
//chrome/browser/content_settings/permission_infobar_delegate.cc

And possibly also these:
//components/nacl/browser/nacl_process_host.cc
//content/browser/frame_host/navigation_entry_impl.cc

For the first few, c/b/ui seems appropriate, but that wouldn't work for the
//components and //content code. WDYT?

[felt, 2014-09-15 22:47:55]

https://codereview.chromium.org/532523002/diff/110001/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/532523002/diff/110001/net/base/net_util.h#new...
net/base/net_util.h:350: // in security UI: infobars, permission strings, etc.
Use this instead of
On 2014/09/15 05:42:30, felt wrote:
> On 2014/09/02 19:26:17, Ryan Sleevi wrote:
> > So, to re-iterate the layering, //net has no idea about (infobars,
permission
> > strings, etc). It's a conceptual separation in //net (that occasionally gets
> > punctured, and then I whine about it until someone fixes it or tries to do
> more
> > of the same, and then I whine until either they fix it or they call me on my
> > whining and I fix it)
> > 
> > Strictly from a code layout and layering point, what code potentially calls
> into
> > this? Is it //content? //chrome? //components? I would rather explore that,
> and
> > understand how the high level code would/is going to use this, and then
figure
> > out the right layer.
> > 
> > Ideal world, this would live in //chrome/browser/ui, and just call FormatUrl
> > with some set of flags.
> 
> Here are some files that should be refactored to call this proposed new
method:
> //chrome/browser/pepper_broker_infobar_delegate.cc
> //chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc
> //chrome/browser/ui/views/website_settings/permissions_bubble_view.cc
> //chrome/browser/content_settings/permission_infobar_delegate.cc
> 
> And possibly also these:
> //components/nacl/browser/nacl_process_host.cc
> //content/browser/frame_host/navigation_entry_impl.cc
> 
> For the first few, c/b/ui seems appropriate, but that wouldn't work for the
> //components and //content code. WDYT?

Sleevi: what about putting this on gurl.h?

[Ryan Sleevi, 2014-09-15 23:33:08]

On 2014/09/15 22:47:55, felt wrote:
> https://codereview.chromium.org/532523002/diff/110001/net/base/net_util.h
> File net/base/net_util.h (right):
> 
>
https://codereview.chromium.org/532523002/diff/110001/net/base/net_util.h#new...
> net/base/net_util.h:350: // in security UI: infobars, permission strings, etc.
> Use this instead of
> On 2014/09/15 05:42:30, felt wrote:
> > On 2014/09/02 19:26:17, Ryan Sleevi wrote:
> > > So, to re-iterate the layering, //net has no idea about (infobars,
> permission
> > > strings, etc). It's a conceptual separation in //net (that occasionally
gets
> > > punctured, and then I whine about it until someone fixes it or tries to do
> > more
> > > of the same, and then I whine until either they fix it or they call me on
my
> > > whining and I fix it)
> > > 
> > > Strictly from a code layout and layering point, what code potentially
calls
> > into
> > > this? Is it //content? //chrome? //components? I would rather explore
that,
> > and
> > > understand how the high level code would/is going to use this, and then
> figure
> > > out the right layer.
> > > 
> > > Ideal world, this would live in //chrome/browser/ui, and just call
FormatUrl
> > > with some set of flags.
> > 
> > Here are some files that should be refactored to call this proposed new
> method:
> > //chrome/browser/pepper_broker_infobar_delegate.cc
> > //chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc
> > //chrome/browser/ui/views/website_settings/permissions_bubble_view.cc
> > //chrome/browser/content_settings/permission_infobar_delegate.cc
> > 
> > And possibly also these:
> > //components/nacl/browser/nacl_process_host.cc
> > //content/browser/frame_host/navigation_entry_impl.cc
> > 
> > For the first few, c/b/ui seems appropriate, but that wouldn't work for the
> > //components and //content code. WDYT?
> 
> Sleevi: what about putting this on gurl.h?

Doesn't belong there - GURL is UI/policy independent.

I don't want the low-level policy decision in //net. If something is related to
UI display, it really doesn't belong here (PR below). Either //content or
//components if necessary, //chrome preferably.

[Ryan Sleevi, 2015-05-07 01:11:40]

Is this a dead CL? Did you want to still poke on this? Shall we figure out where
it goes? Just trying to clean out my CL list (feel free to close it if dead)

[felt, 2015-05-07 01:16:08]

On 2015/05/07 01:11:40, Ryan Sleevi wrote:
> Is this a dead CL? Did you want to still poke on this? Shall we figure out
where
> it goes? Just trying to clean out my CL list (feel free to close it if dead)

palmer -- am I correct in thinking that I should close this, and you're taking
over it?

[Ryan Sleevi, 2015-05-08 00:15:45]

I'm assuming you meant to copy palmer@ before asking him a question, since uh...
he wasn't on this review ;)

[felt, 2015-05-08 00:16:35]

On 2015/05/08 00:15:45, Ryan Sleevi wrote:
> I'm assuming you meant to copy palmer@ before asking him a question, since
uh...
> he wasn't on this review ;)

palmer is all-seeing and all-knowing. he doesn't need to be cc'ed.

(also ha yes whoooops)

[palmer, 2015-05-08 17:39:02]

> palmer -- am I correct in thinking that I should close this, and you're taking
> over it?

If you like, go ahead and re-assign the bug to me. (Please don't delete this CL;
I'd likely use it for reference.)