DOMUI: Use ShowSingletonTab to open the settings tab.

DOMUI: Add |ignore_path| to NavigateParams and use this to implement opening a singleton settings window.

BUG=59889
TEST=none

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

[James Hawkins, 2010-10-29 23:18:54]

+ben

[csilv, 2010-10-29 23:45:54]

The change LGTM, but let me document why we decided to not use ShowSingletonTab
(we used to.)  The options url can specify a subpage.  Example:

chrome://settings/advanced

If the user goes to the above URL, and then chooses 'Settings' from the wrench
menu, we want to re-use an existing options tab if one exists.  ShowSingletonTab
will only do that if the URL's match exactly so we would end up with multiple
Settings tabs.

A related problem is this.  If the user chooses 'Clear Browsing Data', it will
open a settings window with the Clear Browsing Data sub-dialog visible.  The
user can then close the sub-dialog.  If the user then chooses the 'Clear
Browsing Data' command again, it will fail... it will focus the existing
settings tab, but it won't show the sub-dialog because it thinks that it's
already visible.

We could perhaps move the logic used in ShowOptionsTab to ShowSingletonTab, but
I'm not sure if that's the correct logic to use in all cases... it could break
other usage scenarios.

[Ben Goodger (Google), 2010-11-01 18:33:38]

I don't think you should degrade functionality... since you're doing all of this
in terms of ref params... I think you should build this functionality into
browser_navigator.cc and expose it as an option on NavigateParams.

[csilv, 2010-11-01 18:41:11]

http://codereview.chromium.org/4145013/diff/3001/4001
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/4145013/diff/3001/4001#newcode1094
chrome/browser/browser.cc:1094: // Check the scheme,host combination to find a
previously opened settings
Suggest removing reference to 'settings tab' in the comment as ShowSingletonTab
is not settings specific.

http://codereview.chromium.org/4145013/diff/3001/4001#newcode1099
chrome/browser/browser.cc:1099: (tab_url.scheme() == url.scheme() &&
tab_url.host() == url.host())) {
If either CompareURLsIgnoreRef is true, simply calling SelectTabContentsAt is
fine.  But if both CompareURLsIgnoreRef are false, but scheme & host match, then
we also need to call OpenURLAtIndex as previously found in ShowOptionsTab. 
Otherwise, we will simply select an existing tab, but fail to load the correct
page.

Also, when doing the scheme/host comparison, we probably need to compare tab_url
to url and rewritten_url for completeness.

[Ben Goodger (Google), 2010-11-01 19:18:45]

http://codereview.chromium.org/4145013/diff/3001/4001
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/4145013/diff/3001/4001#newcode1099
chrome/browser/browser.cc:1099: (tab_url.scheme() == url.scheme() &&
tab_url.host() == url.host())) {
Take a look at src/chrome/browser/browser_navigator.cc... it has a lot of code
that can handle this sort of thing. I am going to land a change today that
converts Browser to use it fully (including ShowSingletonTab).

[James Hawkins, 2010-11-17 00:02:01]

New implementation uploaded.  Please take a look.

[Ben Goodger (Google), 2010-11-17 00:20:45]

http://codereview.chromium.org/4145013/diff/15001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/4145013/diff/15001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1768: browser::Navigate(&params);
How about changing Browser::ShowSingletonTab() to take ignore_path as a
parameter and just calling that with |url|?

[James Hawkins, 2010-11-17 00:31:16]

http://codereview.chromium.org/4145013/diff/15001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/4145013/diff/15001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1768: browser::Navigate(&params);
On 2010/11/17 00:20:45, Ben Goodger wrote:
> How about changing Browser::ShowSingletonTab() to take ignore_path as a
> parameter and just calling that with |url|?

Done.

[csilv, 2010-11-17 01:31:56]

LGTM

http://codereview.chromium.org/4145013/diff/24001/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator_browsertest.cc (right):

http://codereview.chromium.org/4145013/diff/24001/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest.cc:474: // and |ignore_tabs| =
true opens a new tab navigated to the specified URL if no
I think you mean "ignore_path".  Same on lines 503 and 534.

http://codereview.chromium.org/4145013/diff/24001/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest.cc:481: // We should have one
browser with 2 tabs, the 2nd selected.
minor nit, the comment states that we should have 2 tabs, but it doesn't
actually test that in this block.  Same in next two tests.

[James Hawkins, 2010-11-17 01:48:21]

http://codereview.chromium.org/4145013/diff/24001/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator_browsertest.cc (right):

http://codereview.chromium.org/4145013/diff/24001/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest.cc:474: // and |ignore_tabs| =
true opens a new tab navigated to the specified URL if no
On 2010/11/17 01:31:57, csilv wrote:
> I think you mean "ignore_path".  Same on lines 503 and 534.

Done.

http://codereview.chromium.org/4145013/diff/24001/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator_browsertest.cc:481: // We should have one
browser with 2 tabs, the 2nd selected.
On 2010/11/17 01:31:57, csilv wrote:
> minor nit, the comment states that we should have 2 tabs, but it doesn't
> actually test that in this block.  Same in next two tests.

Done.

[Ben Goodger (Google), 2010-11-17 15:45:49]

LGTM.