Revise about: and chrome: url handling.

*Fixup about and chrome scheme URLs in URLFixerUpper::FixupURL.
*Update AboutSource to use the source_name of each about/chrome page.
*Make WillHandleBrowserAboutURL fix up schemes and handle chrome://foo/
-Redirect memory to memory-redirect (wasn't handling chrome://memory/).
-Catalog all kChromeUI*Hosts in url_constants.cc
-Simplify paths (credits, os-credits, ipc, settings, about/version).
-Nix web_ui_util::ChromeURLHostEquals (use scheme & host comparison).
-Favor GURL::SchemeIs and url_constants, update & expand tests.
-Add and fixup chrome://chrome-urls (the about:about page).
-Update special_tabs.py from Nirnimesh's codereview.chromium.org/6995057/.

This change was reverted with r88166 for sync_integration_tests failures.
These failures are a tangential issue, crbug.com/85294; I'll re-land this soon.

BUG=55771
TEST=Access about:, about://, chrome:, chrome:// pages.

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

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

[msw, 2011-05-27 16:35:32]

This change still needs some work, but please review or include others. Thanks!
Jeff is pushing hard to get this in for M13. Some review suggestions:
estade: mostly url constants, webui.
sky: mostly browser_about_handler / AboutSource.
abarth: mostly web_ui_utils.

[sky, 2011-05-27 16:54:40]

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/browser_about...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/browser_about...
chrome/browser/browser_about_handler.cc:113: #if defined(OS_WIN)
nit: All the ifdefs in here make this a bit hard to read. Maybe it would be more
readable if you grouped the conditional ifdefs at the end.

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/browser_about...
chrome/browser/browser_about_handler.cc:156: // AboutSource handles these about:
and chrome: paths.
nit: match indentation of 146-154, and same comment about grouping #ifdefs at
end.

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/browser_about...
chrome/browser/browser_about_handler.cc:188: explicit AboutSource(const
std::string& source_name, Profile* profile);
nit: no explicit

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/browser_about...
chrome/browser/browser_about_handler.cc:216: for (size_t i = 0; i <
arraysize(kAboutSourceNames); i++) {
Do you really need this for loop? Seems like it should be in a #if
!defined(NDEBUG)

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/browser_about...
chrome/browser/browser_about_handler.cc:246: : source_(source),
request_id_(request_id) {}
nit: if you can't fit the whole constructor on a single line, each member goes
on its own line.

[abarth-chromium, 2011-05-27 17:19:24]

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/ui/webui/web_...
File chrome/browser/ui/webui/web_ui_util.cc (right):

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_util.cc:58: return path.length() > begin ?
path.substr(begin, length) : std::string();
These functions are way too low-level for chrome/browser/ui.

I'm sympathetic if you're rushing to meet M13, but we should at least have TODO
comments and bugs filed about doing this the right way.  (Of course, doing it
the right way first is preferable.)

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/ui/webui/web_...
File chrome/browser/ui/webui/web_ui_util.h (right):

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_util.h:38: std::string GetChromeURLPath(const
GURL& url);
Should we fix GURL to get these cases right instead of re-inventing URL parsing
in chrome/browser/ui ?

[msw, 2011-05-31 13:34:29]

Please take another look; thanks!

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/browser_about...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/browser_about...
chrome/browser/browser_about_handler.cc:113: #if defined(OS_WIN)
On 2011/05/27 16:54:40, sky wrote:
> nit: All the ifdefs in here make this a bit hard to read. Maybe it would be
more
> readable if you grouped the conditional ifdefs at the end.

I was shooting for combined alphabetical order, also avoiding the trailing comma
confusion of #ifdefs at the end :] Would you prefer a platform specific section
at the end of the about:about pages list?

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/browser_about...
chrome/browser/browser_about_handler.cc:156: // AboutSource handles these about:
and chrome: paths.
On 2011/05/27 16:54:40, sky wrote:
> nit: match indentation of 146-154, and same comment about grouping #ifdefs at
> end.

indentation fixed, see comment above regarding order.

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/browser_about...
chrome/browser/browser_about_handler.cc:188: explicit AboutSource(const
std::string& source_name, Profile* profile);
On 2011/05/27 16:54:40, sky wrote:
> nit: no explicit

Done.

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/browser_about...
chrome/browser/browser_about_handler.cc:216: for (size_t i = 0; i <
arraysize(kAboutSourceNames); i++) {
On 2011/05/27 16:54:40, sky wrote:
> Do you really need this for loop? Seems like it should be in a #if
> !defined(NDEBUG)

This is done to avoid allocating an AboutSource for
chrome://LongUnknownChromeURLThatTheUserEnteredForWhoKnowsWhatReason :)

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/browser_about...
chrome/browser/browser_about_handler.cc:246: : source_(source),
request_id_(request_id) {}
On 2011/05/27 16:54:40, sky wrote:
> nit: if you can't fit the whole constructor on a single line, each member goes
> on its own line.

Done.

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/ui/webui/web_...
File chrome/browser/ui/webui/web_ui_util.cc (right):

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_util.cc:58: return path.length() > begin ?
path.substr(begin, length) : std::string();
On 2011/05/27 17:19:25, abarth wrote:
> These functions are way too low-level for chrome/browser/ui.
> 
> I'm sympathetic if you're rushing to meet M13, but we should at least have
TODO
> comments and bugs filed about doing this the right way.  (Of course, doing it
> the right way first is preferable.)

I rewrote this as URLFixerUpper::FixupChromeURL.

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/ui/webui/web_...
File chrome/browser/ui/webui/web_ui_util.h (right):

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_util.h:38: std::string GetChromeURLPath(const
GURL& url);
On 2011/05/27 17:19:25, abarth wrote:
> Should we fix GURL to get these cases right instead of re-inventing URL
parsing
> in chrome/browser/ui ?

I rewrote this as URLFixerUpper::FixupChromeURL. I'm waiting to hear from
mmentovai if such a change could be up-streamed to GURL.

[sky, 2011-05-31 17:15:36]

LGTM

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/browser_about...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/browser_about...
chrome/browser/browser_about_handler.cc:113: #if defined(OS_WIN)
On 2011/05/27 16:54:40, sky wrote:
> nit: All the ifdefs in here make this a bit hard to read. Maybe it would be
more
> readable if you grouped the conditional ifdefs at the end.

I vote for single #ifdef section at the end.

http://codereview.chromium.org/7068007/diff/6002/chrome/browser/browser_about...
chrome/browser/browser_about_handler.cc:216: for (size_t i = 0; i <
arraysize(kAboutSourceNames); i++) {
On 2011/05/31 13:34:29, msw wrote:
> On 2011/05/27 16:54:40, sky wrote:
> > Do you really need this for loop? Seems like it should be in a #if
> > !defined(NDEBUG)
> 
> This is done to avoid allocating an AboutSource for
> chrome://LongUnknownChromeURLThatTheUserEnteredForWhoKnowsWhatReason :)

Ah, I didn't realize this is invoked for any random URL. Can you update the
comment to indicate 'name' may not be known.

[msw, 2011-05-31 18:43:59]

Done! Evan and Adam, please take a look; thanks!

[abarth-chromium, 2011-05-31 19:27:54]

Some minor comments below.  Feel free to land without syncing up with me again.

http://codereview.chromium.org/7068007/diff/24001/chrome/browser/browser_abou...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/7068007/diff/24001/chrome/browser/browser_abou...
chrome/browser/browser_about_handler.cc:366: html += "<li><a href='chrome://" +
*i + "/'>about:" + *i + "</a></li>\n";
It's strange that the link says "about:i" but the link actually points to
"chrome://i"

http://codereview.chromium.org/7068007/diff/24001/chrome/browser/browser_abou...
chrome/browser/browser_about_handler.cc:1304: *url =
url->ReplaceComponents(replacements);
Do we need to do all the replacements?  It would be nice to have a 1-to-1
mapping, but maybe that's an issue for a later patch.

http://codereview.chromium.org/7068007/diff/24001/chrome/browser/browser_abou...
chrome/browser/browser_about_handler.cc:1309: int* bad_pointer = NULL;
This needs to be "volatile" right?

http://codereview.chromium.org/7068007/diff/24001/chrome/browser/net/url_fixe...
File chrome/browser/net/url_fixer_upper.h (right):

http://codereview.chromium.org/7068007/diff/24001/chrome/browser/net/url_fixe...
chrome/browser/net/url_fixer_upper.h:55: GURL FixupChromeURL(const GURL& url);
This description doesn't seem quite right.  This function always returns a
"chrome" URL, right?

[Evan Stade, 2011-05-31 20:26:43]

my parts LGTM

[msw, 2011-05-31 20:58:25]

Thanks! FYI on updates. Also, I updated
chrome/browser/extensions/extension_tabs_module.cc to handle new chrome:,
chrome://, and about:// crashing URLs, thankfully caught by this extra
investigation.

Will land soon with no objections.

http://codereview.chromium.org/7068007/diff/24001/chrome/browser/browser_abou...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/7068007/diff/24001/chrome/browser/browser_abou...
chrome/browser/browser_about_handler.cc:366: html += "<li><a href='chrome://" +
*i + "/'>about:" + *i + "</a></li>\n";
On 2011/05/31 19:27:54, abarth wrote:
> It's strange that the link says "about:i" but the link actually points to
> "chrome://i"

Done. Further cleanup is slated for crbug.com/73926.

http://codereview.chromium.org/7068007/diff/24001/chrome/browser/browser_abou...
chrome/browser/browser_about_handler.cc:1304: *url =
url->ReplaceComponents(replacements);
On 2011/05/31 19:27:54, abarth wrote:
> Do we need to do all the replacements?  It would be nice to have a 1-to-1
> mapping, but maybe that's an issue for a later patch.

Only the host may need replacement here. The scheme is 'normalized' to chrome://
in URLFixerUpper::FixupChromeURL() above (so GURL::host() will actually parse
the host) and the remaining path components are untouched (since individual
chrome:// pages may elect to use them).

http://codereview.chromium.org/7068007/diff/24001/chrome/browser/browser_abou...
chrome/browser/browser_about_handler.cc:1309: int* bad_pointer = NULL;
On 2011/05/31 19:27:54, abarth wrote:
> This needs to be "volatile" right?

I'm following pkasting's offline suggestion to change this to "CHECK(false);" as
that's simpler and we don't appear to use any  specifics of this seg fault for
logging/crash analysis.

http://codereview.chromium.org/7068007/diff/24001/chrome/browser/net/url_fixe...
File chrome/browser/net/url_fixer_upper.h (right):

http://codereview.chromium.org/7068007/diff/24001/chrome/browser/net/url_fixe...
chrome/browser/net/url_fixer_upper.h:55: GURL FixupChromeURL(const GURL& url);
On 2011/05/31 19:27:54, abarth wrote:
> This description doesn't seem quite right.  This function always returns a
> "chrome" URL, right?

Done.

http://codereview.chromium.org/7068007/diff/29001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/7068007/diff/29001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_module.cc:417: // Don't let the
extension crash the browser or renderers.
comment outdented...

[msw, 2011-06-01 00:09:07]

Hey Peter, can you review URLFixerUpper? Eric said he could rubber stamp as
owner once I have your approval.

[Peter Kasting, 2011-06-01 00:45:19]

Added comments on bug summarizing our discussion in person.

[msw, 2011-06-06 19:15:04]

Please consider this revised CL; high level changes:
*Fixup about and chrome URLs in URLFixerUpper::FixupURL. 
-Add and fixup chrome://chrome-urls (the about:about page).
-Handle some code/test fallout.

Planned follow-up: Mostly eliminate "about:*" constants and literals from code
and tests, then hopefully remove the forced fixup in WillHandleBrowserAboutURL.

If interested, see my interim work between patch 7&8:
codereview.chromium.org/7104022/

[Peter Kasting, 2011-06-06 22:30:21]

LGTM

[msw, 2011-06-06 23:32:43]

jam - please review content/browser/*
willchan - please reivew chrome/browser/net/*
Thanks!

[eroman, 2011-06-07 00:24:04]

lgtm on chrome/browser/net/*

http://codereview.chromium.org/7068007/diff/38002/chrome/browser/net/url_fixe...
File chrome/browser/net/url_fixer_upper.h (right):

http://codereview.chromium.org/7068007/diff/38002/chrome/browser/net/url_fixe...
chrome/browser/net/url_fixer_upper.h:44: // Schemes "about" and "chrome" are
'normalized' to "chrome://", with slashes.
nit: please remove single quotes from normalized. Or change it to double quotes.

[msw, 2011-06-07 00:55:31]

http://codereview.chromium.org/7068007/diff/38002/chrome/browser/net/url_fixe...
File chrome/browser/net/url_fixer_upper.h (right):

http://codereview.chromium.org/7068007/diff/38002/chrome/browser/net/url_fixe...
chrome/browser/net/url_fixer_upper.h:44: // Schemes "about" and "chrome" are
'normalized' to "chrome://", with slashes.
On 2011/06/07 00:24:05, eroman wrote:
> nit: please remove single quotes from normalized. Or change it to double
quotes.

Done.

[jam, 2011-06-07 16:48:53]

content lgtm

[msw, 2011-06-08 04:12:52]

Nirnimesh: please review chrome/test/functional/special_tabs.py.
I applied your codereview.chromium.org/6995057 as we discussed; thanks.

[Nirnimesh, 2011-06-08 18:04:03]

On 2011/06/08 04:12:52, msw wrote:
> Nirnimesh: please review chrome/test/functional/special_tabs.py.
> I applied your codereview.chromium.org/6995057 as we discussed; thanks.

special_tabs.py LGTM