Fix navigation bugs, patch the right file, nuke an old file.

chrome/browser/ui/browser_navigator.cc

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/browser_navigator.h"
 
 #include "base/command_line.h"
 #include "chrome/browser/browser.h"
 #include "chrome/browser/browser_list.h"
 #include "chrome/browser/browser_url_handler.h"
@@ skipping 162 matching lines @@
       (params->disposition == NEW_BACKGROUND_TAB ||
        params->disposition == CURRENT_TAB ||
        params->disposition == SINGLETON_TAB)) {
     params->disposition = NEW_FOREGROUND_TAB;
   }
   if (params->browser->profile()->IsOffTheRecord() &&
       params->disposition == OFF_THE_RECORD) {
     params->disposition = NEW_FOREGROUND_TAB;
   }
 
-  // Disposition trumps add types. ADD_SELECTED is a default, so we need to
-  // remove it if disposition implies the tab is going to open in the
-  // background.
-  if (params->disposition == NEW_BACKGROUND_TAB)
-    params->tabstrip_add_types &= ~TabStripModel::ADD_SELECTED;
+  switch (params->disposition) {
+    case NEW_BACKGROUND_TAB:
+      // Disposition trumps add types. ADD_SELECTED is a default, so we need to
+      // remove it if disposition implies the tab is going to open in the
+      // background.
+      params->tabstrip_add_types &= ~TabStripModel::ADD_SELECTED;
+      break;
 
-  // Code that wants to open a new window typically expects it to be shown
-  // automatically.
-  if (params->disposition == NEW_WINDOW || params->disposition == NEW_POPUP) {
-    params->show_window = true;
-    params->tabstrip_add_types |= TabStripModel::ADD_SELECTED;
+    case NEW_WINDOW:
+    case NEW_POPUP:
+      // Code that wants to open a new window typically expects it to be shown
+      // automatically.
+      params->show_window = true;
+      // Fall-through.
+    case NEW_FOREGROUND_TAB:
+    case SINGLETON_TAB:
+      params->tabstrip_add_types |= TabStripModel::ADD_SELECTED;
+      break;
+
+    default:
+      break;
   }
 }
 
 // This class makes sure the Browser object held in |params| is made visible
 // by the time it goes out of scope, provided |params| wants it to be shown.
 class ScopedBrowserDisplayer {
  public:
   explicit ScopedBrowserDisplayer(browser::NavigateParams* params)
       : params_(params) {
   }
@@ skipping 70 matching lines @@
       tabstrip_add_types(TabStripModel::ADD_SELECTED),
       show_window(false),
       browser(a_browser),
       profile(NULL) {
 }
 
 NavigateParams::~NavigateParams() {
 }
 
 void Navigate(NavigateParams* params) {
+  Browser* browser = params->browser;
   params->browser = GetBrowserForDisposition(params);
   if (!params->browser)
     return;
   // Navigate() must not return early after this point.
 
+  if (browser != params->browser &&
+      params->browser->tabstrip_model()->empty()) {
+    // A new window has been created. So it needs to be displayed.
+    params->show_window = true;

[Ben Goodger (Google), 2010/11/17 15:57:50]

Did you evaluate all cases relating to startup (in browser.cc, browser_init.cc
etc) and verify that no one did any work post-Navigate() but before calling
browser->window()->Show()?

[sadrul, 2010/11/17 17:02:23]

I cannot honestly claim I covered all the places, but I tried my best. It looks
like window()->Show() is called (if at all) immediately after Navigate() (or
after AddBlankTab() etc. which essentially ends with Navigate()).

BrowserInit::LaunchWithProfile::OpenTabsInBrowser does a number of Navigate()
calls in a loop, and then calls Show(). But this change shouldn't affect that,
and |show_window| shouldn't be set since params->browser wouldn't be changed
within GetBrowserForDisposition or AdjustNavigateParamsForURL.

+  }
+
   // Make sure the Browser is shown if params call for it.
   ScopedBrowserDisplayer displayer(params);
 
   // Makes sure any TabContents created by this function is destroyed if
   // not properly added to a tab strip.
   ScopedTargetContentsOwner target_contents_owner(params);
 
   // Some dispositions need coercion to base types.
   NormalizeDisposition(params);
 
@@ skipping 74 matching lines @@
           params->transition,
           params->tabstrip_add_types);
       // Now that the |params->target_contents| is safely owned by the target
       // Browser's TabStripModel, we can release ownership.
       target_contents_owner.ReleaseOwnership();
     }
   }
 }
 
 }  // namespace browser