Normalize chrome://foo/ trailing slashes, ChromeURLHostEquals comparison, RIP dead consts.

Normalize chrome://foo/ trailing slashes (add a few).
Add web_ui_util::ChromeURLHostEquals and update some checks.
RIP kChromeUIAppLauncherURL (arv:r46266 - tfarina:r62866).
RIP kDefaultOptionsSubPage (zelidrag:r56816 - estade:r76329/r76490).
Remove unnecessary stdlib.h include and some extra spaces.

BUG=55771
TEST=None

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

[msw, 2011-05-19 18:15:07]

Please take a look or let me know who ought to. Thanks!

[Evan Stade, 2011-05-19 20:12:33]

Are you sure the StartsWithASCII checks aren't intentional? i.e.
chrome://newtab/foo might be a possible and valid URL. I know
chrome://settings/foo for various values of foo is valid.

[Erik does not do reviews, 2011-05-19 20:23:13]

+mpcomplete (aa is out)

On Thu, May 19, 2011 at 11:15 AM, <msw@chromium.org> wrote:

> Reviewers: Aaron Boodman, Evan Stade, tfarina,
>
> Message:
> Please take a look or let me know who ought to. Thanks!
>
> Description:
> Normalize chrome://foo trailing slashes (add a few).
> Use GURL comparison instead of url spec string comparison.
> RIP kChromeUIAppLauncherURL (arv:r46266 - tfarina:r62866).
> RIP kDefaultOptionsSubPage (zelidrag:r56816 - estade:r76329/r76490).
> Remove some extra spaces.
>
> BUG=55771
> TEST=none
>
> Please review this at http://codereview.chromium.org/7049004/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M chrome/browser/extensions/extension_install_ui.cc
>  M chrome/browser/tabs/tab_strip_model.cc
>  M chrome/browser/ui/browser.cc
>  M chrome/browser/ui/gtk/browser_toolbar_gtk.cc
>  M chrome/browser/ui/webui/BidiCheckerWebUITest.cc
>  M chrome/common/url_constants.h
>  M chrome/common/url_constants.cc
>
>
> Index: chrome/browser/extensions/extension_install_ui.cc
> diff --git a/chrome/browser/extensions/extension_install_ui.cc
> b/chrome/browser/extensions/extension_install_ui.cc
> index
>
783ccdc446a55f5f99aa0413da17876b4192a48a..86049a664adcdd128b5f9774c6550a955a980322
> 100644
> --- a/chrome/browser/extensions/extension_install_ui.cc
> +++ b/chrome/browser/extensions/extension_install_ui.cc
> @@ -70,8 +70,7 @@ void ShowAppInstalledAnimation(Browser* browser, const
> std::string& app_id) {
>   // automatically show the install animation for any new apps.
>   for (int i = 0; i < browser->tab_count(); ++i) {
>     TabContents* tab_contents = browser->GetTabContentsAt(i);
> -    GURL url = tab_contents->GetURL();
> -    if (StartsWithASCII(url.spec(), chrome::kChromeUINewTabURL, false)) {
> +    if (tab_contents->GetURL() == GURL(chrome::kChromeUINewTabURL)) {
>       browser->ActivateTabAt(i, false);
>       return;
>     }
> @@ -79,7 +78,7 @@ void ShowAppInstalledAnimation(Browser* browser, const
> std::string& app_id) {
>
>   // If there isn't an NTP, open one and pass it the ID of the installed
> app.
>   std::string url = base::StringPrintf(
> -      "%s/#app-id=%s", chrome::kChromeUINewTabURL, app_id.c_str());
> +      "%s#app-id=%s", chrome::kChromeUINewTabURL, app_id.c_str());
>   browser->AddSelectedTabWithURL(GURL(url), PageTransition::TYPED);
>  }
>
> Index: chrome/browser/tabs/tab_strip_model.cc
> diff --git a/chrome/browser/tabs/tab_strip_model.cc
> b/chrome/browser/tabs/tab_strip_model.cc
> index
>
d1ded2fb870b424509045ebb4b63ded573a3a98a..5a654416932bc8919f85538105193c94d9aee5e2
> 100644
> --- a/chrome/browser/tabs/tab_strip_model.cc
> +++ b/chrome/browser/tabs/tab_strip_model.cc
> @@ -1102,9 +1102,8 @@ std::vector<int>
> TabStripModel::GetIndicesForCommand(int index) const {
>
>  bool TabStripModel::IsNewTabAtEndOfTabStrip(
>     TabContentsWrapper* contents) const {
> -  return LowerCaseEqualsASCII(contents->tab_contents()->GetURL().spec(),
> -                              chrome::kChromeUINewTabURL) &&
> -      contents == GetContentsAt(count() - 1) &&
> +  return contents == GetContentsAt(count() - 1) &&
> +      contents->tab_contents()->GetURL() ==
> GURL(chrome::kChromeUINewTabURL) &&
>       contents->controller().entry_count() == 1;
>  }
>
> Index: chrome/browser/ui/browser.cc
> diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
> index
>
9501f5a70bb1969c3087da6f1a1160c15ed83c89..dd531ae3972fe2f3214448d0ba095121b221d963
> 100644
> --- a/chrome/browser/ui/browser.cc
> +++ b/chrome/browser/ui/browser.cc
> @@ -1585,8 +1585,7 @@ void Browser::ToggleFullscreenMode() {
>  #if defined(OS_CHROMEOS)
>  void Browser::Search() {
>   // If the NTP is showing, close it.
> -  if (StartsWithASCII(GetSelectedTabContents()->GetURL().spec(),
> -                       chrome::kChromeUINewTabURL, true)) {
> +  if (GetSelectedTabContents()->GetURL() ==
> GURL(chrome::kChromeUINewTabURL) {
>     CloseTab();
>     return;
>   }
> Index: chrome/browser/ui/gtk/browser_toolbar_gtk.cc
> diff --git a/chrome/browser/ui/gtk/browser_toolbar_gtk.cc
> b/chrome/browser/ui/gtk/browser_toolbar_gtk.cc
> index
>
863fe3d4c9643128626a139305852d1e9be4c526..746c4963047282aa467cdca681d47a968ef1e148
> 100644
> --- a/chrome/browser/ui/gtk/browser_toolbar_gtk.cc
> +++ b/chrome/browser/ui/gtk/browser_toolbar_gtk.cc
> @@ -606,7 +606,7 @@ void BrowserToolbarGtk::OnDragDataReceived(GtkWidget*
> widget,
>   if (!url.is_valid())
>     return;
>
> -  bool url_is_newtab = url.spec() == chrome::kChromeUINewTabURL;
> +  bool url_is_newtab = url == GURL(chrome::kChromeUINewTabURL);
>   home_page_is_new_tab_page_.SetValue(url_is_newtab);
>   if (!url_is_newtab)
>     home_page_.SetValue(url.spec());
> Index: chrome/browser/ui/webui/BidiCheckerWebUITest.cc
> diff --git a/chrome/browser/ui/webui/BidiCheckerWebUITest.cc
> b/chrome/browser/ui/webui/BidiCheckerWebUITest.cc
> index
ade6472ea6ecdb29b8d8564368608f34bcab34b6..29b430ae99c4f13148ef1598125576fc7ded5b84
> 100644
> --- a/chrome/browser/ui/webui/BidiCheckerWebUITest.cc
> +++ b/chrome/browser/ui/webui/BidiCheckerWebUITest.cc
> @@ -71,10 +71,6 @@ IN_PROC_BROWSER_TEST_F(WebUIBidiCheckerBrowserTest,
> TestAboutPageLTR) {\
>   RunBidiCheckerOnPage(chrome::kChromeUIAboutURL, false);\
>  }
>
> -IN_PROC_BROWSER_TEST_F(WebUIBidiCheckerBrowserTest,
> TestAppLauncherPageLTR) {\
> -  RunBidiCheckerOnPage(chrome::kChromeUIAppLauncherURL, false);\
> -}
> -
>  // Times out. http://code.google.com/p/chromium/issues/detail?id=82896
>  IN_PROC_BROWSER_TEST_F(WebUIBidiCheckerBrowserTest,
>                        DISABLED_TestBookmarksPageLTR) {\
> Index: chrome/common/url_constants.cc
> diff --git a/chrome/common/url_constants.cc
> b/chrome/common/url_constants.cc
> index
>
33fcb990b2ecd42303f70bd05f150da848f1a22f..332271ed0a67716135573e64fc30aa7c571f1735
> 100644
> --- a/chrome/common/url_constants.cc
> +++ b/chrome/common/url_constants.cc
> @@ -51,10 +51,9 @@ const char kAboutVersionURL[] = "about:version";
>  // to be used for testing.
>  const char kAboutBrowserCrash[] = "about:inducebrowsercrashforrealz";
>
> -const char kChromeUIAboutAboutURL[] = "chrome://about/about";
> -const char kChromeUIAboutCreditsURL[] = "chrome://about/credits";
> -const char kChromeUIAboutURL[] = "chrome://settings/about";
> -const char kChromeUIAppLauncherURL[] =
> "chrome://newtab/#mode=app-launcher";
> +const char kChromeUIAboutAboutURL[] = "chrome://about/about/";
> +const char kChromeUIAboutCreditsURL[] = "chrome://about/credits/";
> +const char kChromeUIAboutURL[] = "chrome://settings/about/";
>  const char kChromeUIBookmarksURL[] = "chrome://bookmarks/";
>  const char kChromeUIBugReportURL[] = "chrome://bugreport/";
>  const char kChromeUIConflictsURL[] = "chrome://conflicts/";
> @@ -68,16 +67,16 @@ const char kChromeUIFaviconURL[] = "chrome://favicon/";
>  const char kChromeUIFlagsURL[] = "chrome://flags/";
>  const char kChromeUIHistory2URL[] = "chrome://history2/";
>  const char kChromeUIHistoryURL[] = "chrome://history/";
> -const char kChromeUIIPCURL[] = "chrome://about/ipc";
> +const char kChromeUIIPCURL[] = "chrome://about/ipc/";
>  const char kChromeUIKeyboardURL[] = "chrome://keyboard/";
> -const char kChromeUINewTabURL[] = "chrome://newtab";
> +const char kChromeUINewTabURL[] = "chrome://newtab/";
>  const char kChromeUIPluginsURL[] = "chrome://plugins/";
>  const char kChromeUIPrintURL[] = "chrome://print/";
>  const char kChromeUISettingsURL[] = "chrome://settings/";
>  const char kChromeUITextfieldsURL[] = "chrome://textfields/";
>
>  #if defined(OS_CHROMEOS)
> -const char kChromeUIAboutOSCreditsURL[] = "chrome://about/os-credits";
> +const char kChromeUIAboutOSCreditsURL[] = "chrome://about/os-credits/";
>  const char kChromeUIActivationMessage[] = "chrome://activationmessage/";
>  const char kChromeUIActiveDownloadsURL[] = "chrome://active-downloads/";
>  const char kChromeUIChooseMobileNetworkURL[] =
> @@ -173,18 +172,17 @@ const char kSyncViewInternalsURL[] =
> "chrome://sync-internals/";
>  const char kGpuInternalsURL[] = "chrome://gpu-internals/";
>
>  // Option sub pages.
> -const char kAdvancedOptionsSubPage[] =  "advanced";
> +const char kAdvancedOptionsSubPage[] = "advanced";
>  const char kAutofillSubPage[] = "autofill";
> -const char kBrowserOptionsSubPage[] =  "browser";
> +const char kBrowserOptionsSubPage[] = "browser";
>  const char kClearBrowserDataSubPage[] = "clearBrowserData";
>  const char kContentSettingsSubPage[] = "content";
>  const char kContentSettingsExceptionsSubPage[] = "contentExceptions";
> -const char kDefaultOptionsSubPage[] =  "";
>  const char kImportDataSubPage[] = "importData";
>  const char kInstantConfirmPage[] = "instantConfirm";
>  const char kLanguageOptionsSubPage[] = "languages";
> -const char kPersonalOptionsSubPage[] =  "personal";
> -const char kPasswordManagerSubPage[] =  "passwords";
> +const char kPersonalOptionsSubPage[] = "personal";
> +const char kPasswordManagerSubPage[] = "passwords";
>  const char kSearchEnginesSubPage[] = "searchEngines";
>  const char kSyncSetupSubPage[] = "syncSetup";
>  #if defined(OS_CHROMEOS)
> Index: chrome/common/url_constants.h
> diff --git a/chrome/common/url_constants.h b/chrome/common/url_constants.h
> index
>
8d030d22afe9c0694db54d2d235b552dd073e3fe..f4a84ddb119eea50e2b21fdb90f6fdd7e3a6930b
> 100644
> --- a/chrome/common/url_constants.h
> +++ b/chrome/common/url_constants.h
> @@ -45,7 +45,6 @@ extern const char kAboutVersionURL[];
>  extern const char kChromeUIAboutAboutURL[];
>  extern const char kChromeUIAboutCreditsURL[];
>  extern const char kChromeUIAboutURL[];
> -extern const char kChromeUIAppLauncherURL[];
>  extern const char kChromeUIBookmarksURL[];
>  extern const char kChromeUIBugReportURL[];
>  extern const char kChromeUIConflictsURL[];
>
>
>

[msw, 2011-05-19 20:36:57]

Good point, Evan; reverted.
+mpcomplete.
Please take a look.

[Evan Stade, 2011-05-19 20:47:44]

I think the way to check if a tab is newtab would be

url.SchemeIs(chrome::kChromeUIScheme) && url.host() ==
chrome::kChromeUINewTabHost

[Matt Perry, 2011-05-19 22:35:26]

LGTM

[msw, 2011-05-19 23:13:11]

On 2011/05/19 20:47:44, Evan Stade wrote:
> I think the way to check if a tab is newtab would be
> 
> url.SchemeIs(chrome::kChromeUIScheme) && url.host() ==
> chrome::kChromeUINewTabHost

I filed http://crbug.com/83314 for the general case, and fixed up the two
StartsWithASCII instances encountered here with scheme&host comparison, since
that seems to be the logical intent of their checks.

PTAL

[tfarina, 2011-05-19 23:19:10]

http://codereview.chromium.org/7049004/diff/3011/chrome/common/url_constants.cc
File chrome/common/url_constants.cc (right):

http://codereview.chromium.org/7049004/diff/3011/chrome/common/url_constants....
chrome/common/url_constants.cc:5: #include "chrome/common/url_constants.h"
thanks.

http://codereview.chromium.org/7049004/diff/3011/chrome/common/url_constants....
chrome/common/url_constants.cc:7: #include <stdlib.h>
Is this include really needed?

[msw, 2011-05-19 23:55:46]

PTAL; thanks.

http://codereview.chromium.org/7049004/diff/3011/chrome/common/url_constants.cc
File chrome/common/url_constants.cc (right):

http://codereview.chromium.org/7049004/diff/3011/chrome/common/url_constants....
chrome/common/url_constants.cc:5: #include "chrome/common/url_constants.h"
On 2011/05/19 23:19:10, tfarina wrote:
> thanks.

:)

http://codereview.chromium.org/7049004/diff/3011/chrome/common/url_constants....
chrome/common/url_constants.cc:7: #include <stdlib.h>
On 2011/05/19 23:19:10, tfarina wrote:
> Is this include really needed?

Apparently not; removed!

[Evan Stade, 2011-05-20 05:18:42]

http://codereview.chromium.org/7049004/diff/1009/chrome/browser/ui/gtk/browse...
File chrome/browser/ui/gtk/browser_toolbar_gtk.cc (right):

http://codereview.chromium.org/7049004/diff/1009/chrome/browser/ui/gtk/browse...
chrome/browser/ui/gtk/browser_toolbar_gtk.cc:609: bool url_is_newtab = url ==
GURL(chrome::kChromeUINewTabURL);
sorry, I guess I was unclear during our chat. I count this as one of the places
you are "touching", hence it should be changed to the other matching style.

[msw, 2011-05-22 01:12:12]

PTAL.

http://codereview.chromium.org/7049004/diff/1009/chrome/browser/ui/gtk/browse...
File chrome/browser/ui/gtk/browser_toolbar_gtk.cc (right):

http://codereview.chromium.org/7049004/diff/1009/chrome/browser/ui/gtk/browse...
chrome/browser/ui/gtk/browser_toolbar_gtk.cc:609: bool url_is_newtab = url ==
GURL(chrome::kChromeUINewTabURL);
On 2011/05/20 05:18:43, Evan Stade wrote:
> sorry, I guess I was unclear during our chat. I count this as one of the
places
> you are "touching", hence it should be changed to the other matching style.

Done.

[Evan Stade, 2011-05-23 20:41:17]

LGTM with optional nit

http://codereview.chromium.org/7049004/diff/9003/chrome/common/url_constants.h
File chrome/common/url_constants.h (right):

http://codereview.chromium.org/7049004/diff/9003/chrome/common/url_constants....
chrome/common/url_constants.h:225: bool IsChromeURL(const GURL& url, const char*
host);
can we call this ChromeURLEquals or something? IsChromeURL sounds like "is this
any chrome url" not "is this a specific chrome url"

[msw, 2011-05-24 03:27:39]

PTAL.

http://codereview.chromium.org/7049004/diff/9003/chrome/common/url_constants.h
File chrome/common/url_constants.h (right):

http://codereview.chromium.org/7049004/diff/9003/chrome/common/url_constants....
chrome/common/url_constants.h:225: bool IsChromeURL(const GURL& url, const char*
host);
On 2011/05/23 20:41:17, Evan Stade wrote:
> can we call this ChromeURLEquals or something? IsChromeURL sounds like "is
this
> any chrome url" not "is this a specific chrome url"

Done.

[msw, 2011-05-24 19:22:19]

Evan: Ping! I moved ChromeURLHostEquals to
chrome/browser/ui/webui/web_ui_util.h, as you requested offline.

[Evan Stade, 2011-05-25 16:54:58]

lgtm