Update BuiltinProvider to use chrome:// URLs.

Update BuiltinProvider to provide chrome:// URLs.
Provide common URLs as users start typing "about://" or "chrome://".
Highlight matching input (including "chrome://" for "about:" input).
Support settings sub-pages/paths, e.g. "chrome://settings/foo".
Add BuiltinProviderTest unit test.

Additional hosts will be added when I fix crbug.com/73926.

BUG=55771
TEST=Get chrome:// AutocompleteProvider URLs in the omnibox dropdown.

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

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

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

[msw, 2011-06-08 23:54:55]

PTAL; thanks.

[Peter Kasting, 2011-06-09 00:45:59]

Hmmm.  So, the problem with this is that it will help users who type
"chrome://foo" but not users who type "about:foo".  Seems like we should
probably try to assist both forms?

[msw, 2011-06-10 02:27:37]

Done; PTAL. Also, I'd appreciate feedback on optional polish opportunities:
-Highlight "foo" in "chrome://foobar" result if user types "about:foo".
-Highlight "chrome://foo" if the user types extra (slash, path, query, ref,
etc.).
-Support settings sub-pages/paths "chrome://settings/foo".

[msw, 2011-06-10 10:48:51]

I improved parsing and highlighting, added settings sub-page support, and
refactored a bit for cleanup. PTAL, thanks!

[Peter Kasting, 2011-06-10 18:50:35]

http://codereview.chromium.org/6995096/diff/2002/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/builtin_provider.cc (right):

http://codereview.chromium.org/6995096/diff/2002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:15: const string16 kAbout =
ASCIIToUTF16(chrome::kAboutScheme) +
Declaring global instances of strings violates the style guide.  These have to
be char* or non-global-scope.

I'd probably just create them within the functions that use them, there won't be
a discernable speed hit.

http://codereview.chromium.org/6995096/diff/2002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:22: const int kMatch =
ACMatchClassification::URL | ACMatchClassification::MATCH;
Nit: I'd probably declare these in the function where they're used, too, to
scope them as narrowly as possible.

http://codereview.chromium.org/6995096/diff/2002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:24: // This list should be kept
in sync with chrome/common/url_constnats.h.
Nit: constnats -> constants.  Would it make sense to have this list over in that
header instead?

http://codereview.chromium.org/6995096/diff/2002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:25: const char
*kChromeSettingsSubPages[] = {
Nit: * goes on type, not name

http://codereview.chromium.org/6995096/diff/2002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:58:
settings.append(ASCIIToUTF16("/"));
Nit: Or you could just do "... + '/'" in the previous initializer, I think that
ought to work.

http://codereview.chromium.org/6995096/diff/2002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:79: bool highlight =
starting_chrome || text.length() > kAboutLength;
How can text.length() be larger than |kAboutLength|, when |kAbout| starts with
|text|?

http://codereview.chromium.org/6995096/diff/2002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:93: // Include the path for
sub-pages (e.g. "chrome://settings/browser").
Shouldn't this whole |host_and_path| construction be hoisted outside this inner
loop?

http://codereview.chromium.org/6995096/diff/2002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:95:
host_and_path.append(UTF8ToUTF16(fixed_url.path()));
Nit: I'd just combine these two lines using the + operator

http://codereview.chromium.org/6995096/diff/2002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:98: host_and_path =
host_and_path.substr(0, host_and_path.length()-1);
Nit: Spaces around operator

[msw, 2011-06-11 21:49:48]

PTAL (addressed comments, added unit tests, supporting changes); thanks!

http://codereview.chromium.org/6995096/diff/2002/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/builtin_provider.cc (right):

http://codereview.chromium.org/6995096/diff/2002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:15: const string16 kAbout =
ASCIIToUTF16(chrome::kAboutScheme) +
On 2011/06/10 18:50:35, Peter Kasting wrote:
> Declaring global instances of strings violates the style guide.  These have to
> be char* or non-global-scope.
> 
> I'd probably just create them within the functions that use them, there won't
be
> a discernable speed hit.

Done.

http://codereview.chromium.org/6995096/diff/2002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:22: const int kMatch =
ACMatchClassification::URL | ACMatchClassification::MATCH;
On 2011/06/10 18:50:35, Peter Kasting wrote:
> Nit: I'd probably declare these in the function where they're used, too, to
> scope them as narrowly as possible.

Done.

http://codereview.chromium.org/6995096/diff/2002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:24: // This list should be kept
in sync with chrome/common/url_constnats.h.
On 2011/06/10 18:50:35, Peter Kasting wrote:
> Nit: constnats -> constants.  Would it make sense to have this list over in
that
> header instead?

Fixed constants, the list isn't used elsewhere, so I'll keep it here for now.

http://codereview.chromium.org/6995096/diff/2002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:25: const char
*kChromeSettingsSubPages[] = {
On 2011/06/10 18:50:35, Peter Kasting wrote:
> Nit: * goes on type, not name

Done. Also fixed browser_about_handler.cc.

http://codereview.chromium.org/6995096/diff/2002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:58:
settings.append(ASCIIToUTF16("/"));
On 2011/06/10 18:50:35, Peter Kasting wrote:
> Nit: Or you could just do "... + '/'" in the previous initializer, I think
that
> ought to work.

Done (sorta, can't add ASCII char/string to string16/char[] afaik).

http://codereview.chromium.org/6995096/diff/2002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:79: bool highlight =
starting_chrome || text.length() > kAboutLength;
On 2011/06/10 18:50:35, Peter Kasting wrote:
> How can text.length() be larger than |kAboutLength|, when |kAbout| starts with
> |text|?

If the user typed |text| "about:", "about:/", or "about://" then it's longer
than |chrome::kAboutScheme| "about", and I'll highlight the "chrome://". I
adjusted the comment for clarity.

http://codereview.chromium.org/6995096/diff/2002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:93: // Include the path for
sub-pages (e.g. "chrome://settings/browser").
On 2011/06/10 18:50:35, Peter Kasting wrote:
> Shouldn't this whole |host_and_path| construction be hoisted outside this
inner
> loop?

Done.

http://codereview.chromium.org/6995096/diff/2002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:95:
host_and_path.append(UTF8ToUTF16(fixed_url.path()));
On 2011/06/10 18:50:35, Peter Kasting wrote:
> Nit: I'd just combine these two lines using the + operator

Done.

http://codereview.chromium.org/6995096/diff/2002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:98: host_and_path =
host_and_path.substr(0, host_and_path.length()-1);
On 2011/06/10 18:50:35, Peter Kasting wrote:
> Nit: Spaces around operator

Done.

http://codereview.chromium.org/6995096/diff/6003/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:46: }  // namespace
Outdented one space.

[Peter Kasting, 2011-06-14 20:18:33]

LGTM

http://codereview.chromium.org/6995096/diff/6003/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/builtin_provider.cc (right):

http://codereview.chromium.org/6995096/diff/6003/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:79: static const size_t
kAboutLength = strlen(chrome::kAboutScheme);
Nit: Naming this |kAboutSchemeLength| might help convey that this is not the
same as the length of |kAbout|.

Another route might instead be to name |kAbout| to e.g. |kAboutWithSeparators|
and similarly adjust kChrome and all related variables, but that might get
really verbose.

http://codereview.chromium.org/6995096/diff/6003/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:83:
styles.push_back(ACMatchClassification(offset, kUrl));
Nit: Seems like you should only do this when |highlight| is true.

http://codereview.chromium.org/6995096/diff/6003/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:96: (i != builtins_.end()) &&
(matches_.size() < kMaxMatches); ++i) {
Nit: Indent 2 more

http://codereview.chromium.org/6995096/diff/6003/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:101: size_t match_length =
kChrome.length() + host_and_path.length();
Nit: This temp can be hoisted above the loop too, no?

http://codereview.chromium.org/6995096/diff/6003/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:114: void
BuiltinProvider::AddMatch(const string16& input,
Nit: This argument is never used.

Seems like |input| and |match_string| should be replaced with the text to put
into match.fill_into_edit, which the caller already sort of has to calculate
(kChrome + *i).

[msw, 2011-06-14 21:32:15]

Done, also included a minor correction in ui/gfx/canvas.h, sort of accidentally
:)

http://codereview.chromium.org/6995096/diff/6003/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/builtin_provider.cc (right):

http://codereview.chromium.org/6995096/diff/6003/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:79: static const size_t
kAboutLength = strlen(chrome::kAboutScheme);
On 2011/06/14 20:18:33, Peter Kasting wrote:
> Nit: Naming this |kAboutSchemeLength| might help convey that this is not the
> same as the length of |kAbout|.
> 
> Another route might instead be to name |kAbout| to e.g. |kAboutWithSeparators|
> and similarly adjust kChrome and all related variables, but that might get
> really verbose.

Done.

http://codereview.chromium.org/6995096/diff/6003/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:83:
styles.push_back(ACMatchClassification(offset, kUrl));
On 2011/06/14 20:18:33, Peter Kasting wrote:
> Nit: Seems like you should only do this when |highlight| is true.

Done.

http://codereview.chromium.org/6995096/diff/6003/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:96: (i != builtins_.end()) &&
(matches_.size() < kMaxMatches); ++i) {
On 2011/06/14 20:18:33, Peter Kasting wrote:
> Nit: Indent 2 more

Done.

http://codereview.chromium.org/6995096/diff/6003/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:101: size_t match_length =
kChrome.length() + host_and_path.length();
On 2011/06/14 20:18:33, Peter Kasting wrote:
> Nit: This temp can be hoisted above the loop too, no?

Done.

http://codereview.chromium.org/6995096/diff/6003/chrome/browser/autocomplete/...
chrome/browser/autocomplete/builtin_provider.cc:114: void
BuiltinProvider::AddMatch(const string16& input,
On 2011/06/14 20:18:33, Peter Kasting wrote:
> Nit: This argument is never used.
> 
> Seems like |input| and |match_string| should be replaced with the text to put
> into match.fill_into_edit, which the caller already sort of has to calculate
> (kChrome + *i).

Done.