If the URL text has no scheme, and starts with ftp., then ...

If the URL text has no scheme, and starts with ftp., then 
default the scheme to ftp: instead of http:. Fixes issue 565.

BUG=565

[Dean McNamee, 2009-01-04 00:39:18]

http://codereview.chromium.org/16511/diff/1/3
File chrome/browser/url_fixer_upper.cc (right):

http://codereview.chromium.org/16511/diff/1/3#newcode297
Line 297: // scheme (e.g. ftp)?
We should explain what other browser do (I was personally surprised by this, I
just ran some tests, I thought maybe they tried ftp:// after trying http://, but
both Firefox and IE go to ftp:// without every trying http://).  The comment
about "known scheme" is deceiving, we're only handling a single case, that of
FTP.  And the "eg ftp" makes it sound like it's a single example of more, which
it's not.  I think it would be better as:

When we couldn't find a scheme from the input, we need to pick a scheme.  If the
URL starts with ftp., other major browsers (Firefox, IE) will assume the
protocol is ftp://.  Otherwise we assume the protocol is http://.

http://codereview.chromium.org/16511/diff/1/3#newcode298
Line 298: if (text.find(L"ftp.") == 0) {
string_util.h StartsWith will be more efficient, there is no point in searching
the entire string.

http://codereview.chromium.org/16511/diff/1/2
File chrome/browser/url_fixer_upper_unittest.cc (right):

http://codereview.chromium.org/16511/diff/1/2#newcode176
Line 176: {L"ftp.google.com", L"", L"ftp://ftp.google.com/"},
Maybe you should also add some negative cases, like ftpblah.google.com -> http,
ftp -> http://ftp, google.ftp.com -> http://, etc.

[Dean McNamee, 2009-01-04 00:41:23]

Also, I'm not familiar with this code, and I don't know what state the text
variable will be in.  If it's not canonicalized to some point, you need to make
sure FTP. would also be matched, so you probably want to test that and maybe do
a case insensitive prefix match.  I also don't know about leading whitespace,
etc...

On 2009/01/04 00:39:18, Dean McNamee wrote:
> http://codereview.chromium.org/16511/diff/1/3
> File chrome/browser/url_fixer_upper.cc (right):
> 
> http://codereview.chromium.org/16511/diff/1/3#newcode297
> Line 297: // scheme (e.g. ftp)?
> We should explain what other browser do (I was personally surprised by this, I
> just ran some tests, I thought maybe they tried ftp:// after trying http://,
but
> both Firefox and IE go to ftp:// without every trying http://).  The comment
> about "known scheme" is deceiving, we're only handling a single case, that of
> FTP.  And the "eg ftp" makes it sound like it's a single example of more,
which
> it's not.  I think it would be better as:
> 
> When we couldn't find a scheme from the input, we need to pick a scheme.  If
the
> URL starts with ftp., other major browsers (Firefox, IE) will assume the
> protocol is ftp://.  Otherwise we assume the protocol is http://.
> 
> http://codereview.chromium.org/16511/diff/1/3#newcode298
> Line 298: if (text.find(L"ftp.") == 0) {
> string_util.h StartsWith will be more efficient, there is no point in
searching
> the entire string.
> 
> http://codereview.chromium.org/16511/diff/1/2
> File chrome/browser/url_fixer_upper_unittest.cc (right):
> 
> http://codereview.chromium.org/16511/diff/1/2#newcode176
> Line 176: {L"ftp.google.com", L"", L"ftp://ftp.google.com/"},
> Maybe you should also add some negative cases, like ftpblah.google.com ->
http,
> ftp -> http://ftp, google.ftp.com -> http://, etc.

[Brian Duff, 2009-01-05 00:48:09]

For the canonicalization issue: the routine already strips trailing and leading
whitespace. I've added a test that covers this case. For capitalization, I've
used the case insensitive version of StartsWith, and added a unit test for this
case too. I verified that Firefox also uses the ftp:// protocol for ambiguous
URLs that start with "FTP."

http://codereview.chromium.org/16511/diff/1/3
File chrome/browser/url_fixer_upper.cc (right):

http://codereview.chromium.org/16511/diff/1/3#newcode297
Line 297: // scheme (e.g. ftp)?
On 2009/01/04 00:39:18, Dean McNamee wrote:
> We should explain what other browser do (I was personally surprised by this, I
> just ran some tests, I thought maybe they tried ftp:// after trying http://,
but
> both Firefox and IE go to ftp:// without every trying http://).  The comment
> about "known scheme" is deceiving, we're only handling a single case, that of
> FTP.  And the "eg ftp" makes it sound like it's a single example of more,
which
> it's not.  I think it would be better as:
> 
> When we couldn't find a scheme from the input, we need to pick a scheme.  If
the
> URL starts with ftp., other major browsers (Firefox, IE) will assume the
> protocol is ftp://.  Otherwise we assume the protocol is http://.

Done.

http://codereview.chromium.org/16511/diff/1/3#newcode298
Line 298: if (text.find(L"ftp.") == 0) {
On 2009/01/04 00:39:18, Dean McNamee wrote:
> string_util.h StartsWith will be more efficient, there is no point in
searching
> the entire string.

Done.

http://codereview.chromium.org/16511/diff/1/2
File chrome/browser/url_fixer_upper_unittest.cc (right):

http://codereview.chromium.org/16511/diff/1/2#newcode176
Line 176: {L"ftp.google.com", L"", L"ftp://ftp.google.com/"},
On 2009/01/04 00:39:18, Dean McNamee wrote:
> Maybe you should also add some negative cases, like ftpblah.google.com ->
http,
> ftp -> http://ftp, google.ftp.com -> http://, etc.

Done.

[brettw, 2009-01-05 02:11:05]

LGTM with formatting nit.

http://codereview.chromium.org/16511/diff/206/10
File chrome/browser/url_fixer_upper.cc (right):

http://codereview.chromium.org/16511/diff/206/10#newcode300
Line 300: if (StartsWith(text, L"ftp.", false)) {
The rest of this file doesn't use {} for single-line conditions, so I would be
consistent and not use them here either.

[Peter Kasting, 2009-01-05 19:24:54]

LGTM with very minor nits

http://codereview.chromium.org/16511/diff/206/10
File chrome/browser/url_fixer_upper.cc (right):

http://codereview.chromium.org/16511/diff/206/10#newcode295
Line 295: } else {
Nit: Roll the below "if" into this one (i.e. "} else if (StartsWith(.."). 
Comments will need a minor update.

http://codereview.chromium.org/16511/diff/206/10#newcode298
Line 298: // (Firefox, IE) will assume the scheme is ftp://. In all other
Tiny nit: Just say "scheme is ftp" and "scheme is http" instead of appending
"://" (since that's not actually part of the scheme, and your comment could
confuse someone later).

[Brian Duff, 2009-01-06 05:25:36]

http://codereview.chromium.org/16511/diff/206/10
File chrome/browser/url_fixer_upper.cc (right):

http://codereview.chromium.org/16511/diff/206/10#newcode295
Line 295: } else {
On 2009/01/05 19:24:55, pkasting wrote:
> Nit: Roll the below "if" into this one (i.e. "} else if (StartsWith(.."). 
> Comments will need a minor update.

Ah. Actually, scheme_end is a dead store, and so the if (scheme.is_valid())
branch is not needed. I've rolled this into a single if / else if, although it's
a bit clumsy because of the need to double check is_valid in the else clause.
Let me know what you think.

http://codereview.chromium.org/16511/diff/206/10#newcode298
Line 298: // (Firefox, IE) will assume the scheme is ftp://. In all other
On 2009/01/05 19:24:55, pkasting wrote:
> Tiny nit: Just say "scheme is ftp" and "scheme is http" instead of appending
> "://" (since that's not actually part of the scheme, and your comment could
> confuse someone later).

Done.

http://codereview.chromium.org/16511/diff/206/10#newcode300
Line 300: if (StartsWith(text, L"ftp.", false)) {
On 2009/01/05 02:11:05, brettw wrote:
> The rest of this file doesn't use {} for single-line conditions, so I would be
> consistent and not use them here either.

Done.

[Peter Kasting, 2009-01-06 18:29:49]

http://codereview.chromium.org/16511/diff/14/210
File chrome/browser/url_fixer_upper.cc (right):

http://codereview.chromium.org/16511/diff/14/210#newcode290
Line 290: if (!parts->scheme.is_valid() && StartsWith(text, L"ftp.", false))
I would just do it this way:

// When we couldn't find a scheme in the input, we need to pick one.  Normally
we choose http, but if the URL starts with "ftp.", we match other browsers and
choose ftp.
if (!parts->scheme.is_valid())
  scheme.assign(StartsWith(text, L"ftp.", false) ? L"ftp" : L"http");

[Brian Duff, 2009-01-07 05:49:03]

http://codereview.chromium.org/16511/diff/14/210
File chrome/browser/url_fixer_upper.cc (right):

http://codereview.chromium.org/16511/diff/14/210#newcode290
Line 290: if (!parts->scheme.is_valid() && StartsWith(text, L"ftp.", false))
On 2009/01/06 18:29:49, pkasting wrote:
> I would just do it this way:
> 
> // When we couldn't find a scheme in the input, we need to pick one.  Normally
> we choose http, but if the URL starts with "ftp.", we match other browsers and
> choose ftp.
> if (!parts->scheme.is_valid())
>   scheme.assign(StartsWith(text, L"ftp.", false) ? L"ftp" : L"http");

Done.

[Peter Kasting, 2009-01-07 18:04:33]

LGTM.