Make all browser code use browser::Navigate to open tabs....

Make all browser code use browser::Navigate to open tabs.

BUG=none
TEST=existing unittests. Also, test all places where new tabs and windows are opened from UI, e.g. links in options, new tab button, Ctrl+T, popup windows, etc.


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

[Ben Goodger (Google), 2010-10-20 16:54:20]

I have some issues to work out on tests but this is the gist.

[sky, 2010-10-20 20:07:15]

http://codereview.chromium.org/3834002/diff/13002/51002
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/3834002/diff/13002/51002#newcode2319
chrome/browser/browser.cc:2319: TabContents* Browser::AddBlankTabAt(int index,
bool foreground) {
Can we nuke the index param as it doesn't seem to be used?

http://codereview.chromium.org/3834002/diff/13002/51004
File chrome/browser/browser_browsertest.cc (right):

http://codereview.chromium.org/3834002/diff/13002/51004#newcode211
chrome/browser/browser_browsertest.cc:211: params.tabstrip_index = 0;
Does this need to set disposition to NEW_TAB?

http://codereview.chromium.org/3834002/diff/13002/51004#newcode237
chrome/browser/browser_browsertest.cc:237: browser()->AddSelectedTabWithURL(url,
PageTransition::TYPED);
This inserts int the opposite order of the old code. Does it matter?

http://codereview.chromium.org/3834002/diff/13002/51006
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/3834002/diff/13002/51006#newcode773
chrome/browser/browser_init.cc:773: browser::Navigate(&params);
Set disposition to NEW_TAB?

http://codereview.chromium.org/3834002/diff/13002/51009
File chrome/browser/chromeos/tab_closeable_state_watcher_browsertest.cc (right):

http://codereview.chromium.org/3834002/diff/13002/51009#newcode43
chrome/browser/chromeos/tab_closeable_state_watcher_browsertest.cc:43:
browser::Navigate(&params);
NEW_TAB?

http://codereview.chromium.org/3834002/diff/13002/51010
File chrome/browser/cocoa/applescript/window_applescript.mm (right):

http://codereview.chromium.org/3834002/diff/13002/51010#newcode189
chrome/browser/cocoa/applescript/window_applescript.mm:189:
browser::NavigateParams params(browser_, GURL(chrome::kChromeUINewTabURL),
NEW_FOREGROUND_TAB?

http://codereview.chromium.org/3834002/diff/13002/51012
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/3834002/diff/13002/51012#newcode1701
chrome/browser/cocoa/tab_strip_controller.mm:1701: browser::NavigateParams
params(browser_, url, PageTransition::TYPED);
NEW_FOREGROUND_TAB?

[Ben Goodger (Google), 2010-10-20 20:49:35]

http://codereview.chromium.org/3834002/diff/13002/51004
File chrome/browser/browser_browsertest.cc (right):

http://codereview.chromium.org/3834002/diff/13002/51004#newcode237
chrome/browser/browser_browsertest.cc:237: browser()->AddSelectedTabWithURL(url,
PageTransition::TYPED);
On 2010/10/20 20:07:15, sky wrote:
> This inserts int the opposite order of the old code. Does it matter?

There was no reason for the ordering.

[sky, 2010-10-21 17:06:58]

http://codereview.chromium.org/3834002/diff/86001/87002
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/3834002/diff/86001/87002#newcode2320
chrome/browser/browser.cc:2320: TabContents* Browser::AddBlankTabAt(int index,
bool foreground) {
Could you remove index since it isn't used?

http://codereview.chromium.org/3834002/diff/86001/87006
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/3834002/diff/86001/87006#newcode770
chrome/browser/browser_init.cc:770: params.disposition = NEW_FOREGROUND_TAB;
Should this be NEW_BACKGROUND_TAB if !first_tab?

http://codereview.chromium.org/3834002/diff/86001/87013
File chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc (right):

http://codereview.chromium.org/3834002/diff/86001/87013#newcode65
chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc:65:
params.disposition = NEW_FOREGROUND_TAB;
You don't set this above, and it looks like browser_navigation infers
NEW_FOREGROUND_TAB in this case. Does it need to be set?

http://codereview.chromium.org/3834002/diff/86001/87014
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/3834002/diff/86001/87014#newcode411
chrome/browser/extensions/extension_tabs_module.cc:411:
new_window->AddSelectedTabWithURL(*i, PageTransition::LINK);
This is going to add each tab and selected it, right? Don't we only want to
select the first tab?

http://codereview.chromium.org/3834002/diff/86001/87018
File chrome/browser/sessions/session_restore.cc (right):

http://codereview.chromium.org/3834002/diff/86001/87018#newcode557
chrome/browser/sessions/session_restore.cc:557: params.disposition =
NEW_FOREGROUND_TAB;
Only i==0 should be selected.

http://codereview.chromium.org/3834002/diff/86001/87025
File chrome/test/in_process_browser_test.h (right):

http://codereview.chromium.org/3834002/diff/86001/87025#newcode72
chrome/test/in_process_browser_test.h:72: void AddTabAtIndexToBrowser(Browser*
browser, int index, const GURL& url,
nit: each param on its own line.

http://codereview.chromium.org/3834002/diff/86001/87025#newcode78
chrome/test/in_process_browser_test.h:78: void AddTabAt(int index, const GURL&
url, PageTransition::Type transition);
It doesn't look like there is an implementation of this. Can it be nuked?

[Ben Goodger (Google), 2010-10-21 20:05:03]

Updated with comments.

http://codereview.chromium.org/3834002/diff/86001/87013
File chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc (right):

http://codereview.chromium.org/3834002/diff/86001/87013#newcode65
chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc:65:
params.disposition = NEW_FOREGROUND_TAB;
On 2010/10/21 17:06:58, sky wrote:
> You don't set this above, and it looks like browser_navigation infers
> NEW_FOREGROUND_TAB in this case. Does it need to be set?

I believe it to be necessary and the code you refer to above to be in error, so
I fixed it.

http://codereview.chromium.org/3834002/diff/86001/87014
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/3834002/diff/86001/87014#newcode411
chrome/browser/extensions/extension_tabs_module.cc:411:
new_window->AddSelectedTabWithURL(*i, PageTransition::LINK);
On 2010/10/21 17:06:58, sky wrote:
> This is going to add each tab and selected it, right? Don't we only want to
> select the first tab?

I believe this to be compatible with the old code - it called AddTabWithURL
which has a default value of ADD_SELECTED for add types.

[sky, 2010-10-21 20:23:22]

LGTM