Strips http from omnibox (and a couple of other places)

Strips http from the omnibox

BUG=none
TEST=type in urls and make sure once loaded we don't show http. Make sure we do show https (and other schemes) though.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=44140

[Peter Kasting, 2010-04-07 22:33:55]

Several of the places you used FORMAT_URL_NONE seem to me like maybe they should
be stripping schemes.  It seems like if we're going to strip things in the
address bar, why wouldn't we also want to do something similar in much of the
rest of our UI, and in general never show http:// except in rare circumstances?

(This would match what we're doing in the geolocation exceptions dialog, for
example, where we'll omit http:// but show other schemes.)

Otherwise most of the below are nits.

http://codereview.chromium.org/1513023/diff/1/2
File app/text_elider.cc (right):

http://codereview.chromium.org/1513023/diff/1/2#newcode1
app/text_elider.cc:1: // Copyright (c) 2009 The Chromium Authors. All rights
reserved.
Nit: 2010

http://codereview.chromium.org/1513023/diff/1/2#newcode366
app/text_elider.cc:366: net::FORMAT_URL_OMIT_USERNAME_PASSWORD,
UnescapeRule::SPACES, &parsed,
Shouldn't this omit the scheme?

http://codereview.chromium.org/1513023/diff/1/6
File chrome/browser/autocomplete/autocomplete.cc (right):

http://codereview.chromium.org/1513023/diff/1/6#newcode724
chrome/browser/autocomplete/autocomplete.cc:724: //  providers_.push_back(new
SearchProvider(this, profile));
Um... why are these commented out?

http://codereview.chromium.org/1513023/diff/1/7
File chrome/browser/autocomplete/autocomplete.h (right):

http://codereview.chromium.org/1513023/diff/1/7#newcode575
chrome/browser/autocomplete/autocomplete.h:575: int string_for_url_types) const;
It seems overly complex to use a bitmask for what in this case is just two
bools, given that we have to declare the values above, write longer checks in
the function itself, etc., and you're not really passing on this value to
further child functions.  I'd just add a second bool argument here instead.

http://codereview.chromium.org/1513023/diff/1/9
File chrome/browser/autocomplete/history_contents_provider.cc (right):

http://codereview.chromium.org/1513023/diff/1/9#newcode1
chrome/browser/autocomplete/history_contents_provider.cc:1: // Copyright (c)
2009 The Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/1513023/diff/1/5
File chrome/browser/autocomplete/history_url_provider.cc (right):

http://codereview.chromium.org/1513023/diff/1/5#newcode1
chrome/browser/autocomplete/history_url_provider.cc:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/1513023/diff/1/18
File chrome/browser/bookmarks/bookmark_table_model.cc (right):

http://codereview.chromium.org/1513023/diff/1/18#newcode1
chrome/browser/bookmarks/bookmark_table_model.cc:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/1513023/diff/1/14
File chrome/browser/gtk/options/passwords_page_gtk.cc (right):

http://codereview.chromium.org/1513023/diff/1/14#newcode1
chrome/browser/gtk/options/passwords_page_gtk.cc:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/1513023/diff/1/12
File chrome/browser/net/browser_url_util.cc (right):

http://codereview.chromium.org/1513023/diff/1/12#newcode1
chrome/browser/net/browser_url_util.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/1513023/diff/1/3
File net/base/net_util.cc (right):

http://codereview.chromium.org/1513023/diff/1/3#newcode1
net/base/net_util.cc:1: // Copyright (c) 2009 The Chromium Authors. All rights
reserved.
Nit: 2010

http://codereview.chromium.org/1513023/diff/1/3#newcode713
net/base/net_util.cc:713: bool omit_username_password,
Nit: Just have the caller pass the enum value (since it already has it), that
simplifies the code below.

http://codereview.chromium.org/1513023/diff/1/3#newcode1375
net/base/net_util.cc:1375: int format_types,
Nit: Take a net::FormatUrlTypes instead of an int.

http://codereview.chromium.org/1513023/diff/1/3#newcode1380
net/base/net_util.cc:1380: bool omit_username_password =
Nit: If you pass the enum value to FormatViewSourceUrl(), you can inline this
condition in the one other place it's used below.

http://codereview.chromium.org/1513023/diff/1/3#newcode1425
net/base/net_util.cc:1425: if ((format_types & FORMAT_URL_OMIT_HTTP) != 0 &&
url_string == L"http://") {
Nit: It seems like you could just omit the copy above instead of clearing the
string:

if ((format_types & FORMAT_URL_OMIT_HTTP) &&
    url.SchemeIs(chrome::kHttpScheme)) {
  // Update *offset_for_adjustment
} else {
  std::copy(...);
}

http://codereview.chromium.org/1513023/diff/1/4
File net/base/net_util.h (right):

http://codereview.chromium.org/1513023/diff/1/4#newcode1
net/base/net_util.h:1: // Copyright (c) 2009 The Chromium Authors. All rights
reserved.
Nit: 2010

http://codereview.chromium.org/1513023/diff/1/4#newcode49
net/base/net_util.h:49: FORMAT_URL_ALL = FORMAT_URL_OMIT_HTTP |
FORMAT_URL_OMIT_USERNAME_PASSWORD
Nit: The constant names "NONE" and "ALL" here are kinda weird... they sound
backwards from what they mean.  Maybe FORMAT_URL_NONE could be
FORMAT_URL_OMIT_NOTHING or FORMAT_URL_INCLUDE_ALL, and FORMAT_URL_ALL could be
FORMAT_URL_OMIT_UNNECESSARY or FORMAT_URL_OMIT_HTTP_USERNAME_PASSWORD or
something?

http://codereview.chromium.org/1513023/diff/1/4#newcode291
net/base/net_util.h:291: // new_parsed=NULL, and prefix_end=NULL.
Nit: This comment is redundant with the code, maybe we could trim it some.

[sky, 2010-04-08 23:07:49]

New patch uploaded.

[Peter Kasting, 2010-04-08 23:30:30]

LGTM if you fix the below

http://codereview.chromium.org/1513023/diff/32001/17006
File chrome/browser/autocomplete/autocomplete.cc (right):

http://codereview.chromium.org/1513023/diff/32001/17006#newcode602
chrome/browser/autocomplete/autocomplete.cc:602: const net::FormatUrlTypes
format_types = net::kFormatUrlOmitUsernamePassword |
Nit: Clearer and simpler:

const net::FormatUrlTypes format_types = trim_http ?
    net::kFormatUrlOmitAll : net::kFormatUrlOmitUsernamePassword;

http://codereview.chromium.org/1513023/diff/32001/17005
File chrome/browser/autocomplete/history_url_provider.cc (right):

http://codereview.chromium.org/1513023/diff/32001/17005#newcode829
chrome/browser/autocomplete/history_url_provider.cc:829: const
net::FormatUrlTypes format_types = net::kFormatUrlOmitUsernamePassword |
Nit: Clearer and simpler:

const net::FormatUrlTypes format_types =
    (params->trim_http && !history_match.match_in_scheme) ?
    net::kFormatUrlOmitAll : net::kFormatUrlOmitUsernamePassword;

http://codereview.chromium.org/1513023/diff/32001/17008
File chrome/browser/autocomplete/search_provider.cc (right):

http://codereview.chromium.org/1513023/diff/32001/17008#newcode740
chrome/browser/autocomplete/search_provider.cc:740: match.contents =
StringForURLDisplay(navigation.url, true, true);
We should only be trimming http here if the user didn't type it (so you should
preserve the condition the old code used).

http://codereview.chromium.org/1513023/diff/32001/17003
File net/base/net_util.cc (right):

http://codereview.chromium.org/1513023/diff/32001/17003#newcode1427
net/base/net_util.cc:1427: const wchar_t* const kHTTP = L"http://";
Nit: Should we be using chrome::kHttpScheme in here somehow?

http://codereview.chromium.org/1513023/diff/32001/17003#newcode1429
net/base/net_util.cc:1429: bool omit_http = ((format_types & kFormatUrlOmitHTTP)
!= 0 &&
Nit: It seems like normally we don't append "!= 0" to bitmask comparisons?

http://codereview.chromium.org/1513023/diff/32001/17003#newcode1536
net/base/net_util.cc:1536: if (omit_http && !url_string.compare(0, kHTTPSize,
kHTTP)) {
Do you need to do this comparison again?  How can it fail when |omit_http| is
true?

http://codereview.chromium.org/1513023/diff/32001/17004
File net/base/net_util.h (right):

http://codereview.chromium.org/1513023/diff/32001/17004#newcode40
net/base/net_util.h:40: typedef int FormatUrlType;
For bitmasks, google3 says to use uint32, and I agree (using a signed type can
occasionally cause strange consequences).

http://codereview.chromium.org/1513023/diff/32001/17004#newcode43
net/base/net_util.h:43: // If set, any username and password are removed.
Nit: Put these four in the same order as they're defined.

http://codereview.chromium.org/1513023/diff/32001/17020
File net/base/net_util_unittest.cc (right):

http://codereview.chromium.org/1513023/diff/32001/17020#newcode1
net/base/net_util_unittest.cc:1: // Copyright (c) 2009 The Chromium Authors. All
rights reserved.
Nit: 2010

http://codereview.chromium.org/1513023/diff/32001/17020#newcode1361
net/base/net_util_unittest.cc:1361:
"http://user%3A:%40passwd@example.com/foo%3Fbar?q=b%26z", L"", false, true,
Looks like you missed converting a "false" here.

http://codereview.chromium.org/1513023/diff/32001/17020#newcode1391
net/base/net_util_unittest.cc:1391: 
Nit: No blank line

[Peter Kasting, 2010-04-08 23:31:23]

Oh yeah, and fix the change description

[sky, 2010-04-09 03:38:55]

I think I got them. I also realized I missed updating the new_parsed argument,
so I did that and added a test case for it.

http://codereview.chromium.org/1513023/diff/32001/17003
File net/base/net_util.cc (right):

http://codereview.chromium.org/1513023/diff/32001/17003#newcode1427
net/base/net_util.cc:1427: const wchar_t* const kHTTP = L"http://";
On 2010/04/08 23:30:30, Peter Kasting wrote:
> Nit: Should we be using chrome::kHttpScheme in here somehow?

net doesn't depend upon chrome.

http://codereview.chromium.org/1513023/diff/32001/17003#newcode1536
net/base/net_util.cc:1536: if (omit_http && !url_string.compare(0, kHTTPSize,
kHTTP)) {
On 2010/04/08 23:30:30, Peter Kasting wrote:
> Do you need to do this comparison again?  How can it fail when |omit_http| is
> true?

Sanity check.

http://codereview.chromium.org/1513023/diff/32001/17020
File net/base/net_util_unittest.cc (right):

http://codereview.chromium.org/1513023/diff/32001/17020#newcode1361
net/base/net_util_unittest.cc:1361:
"http://user%3A:%40passwd@example.com/foo%3Fbar?q=b%26z", L"", false, true,
On 2010/04/08 23:30:30, Peter Kasting wrote:
> Looks like you missed converting a "false" here.

This code is wrong and I didn't want to touch it, but ok.

[sky, 2010-04-09 03:41:22]

Brett,

Peter reviewed most of this, but he's out tomorrow. Could you take a look at the
changes to net_util. The one area I'm not sure on is where I adjust the
url_parse::Components. FormatViewSourceUrl checks is_nonempty rather that
is_valid. Seems like I want is_valid here and so does FormatViewSourceUrl, but
then you wrote the code:)

Thanks,

  -Scott

[brettw, 2010-04-09 18:24:13]

I think we do want is_valid rather than is_nonempty in this case.

[sky, 2010-04-09 19:32:56]

Updated. Could you take another look?

Thanks,

  -Scott

[brettw, 2010-04-09 21:05:35]

LGTM, I only checked net_util.