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/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 160 matching lines @@
   // Calculate the WindowOpenDisposition if necessary.
   if (params->browser->tabstrip_model()->empty() &&
       (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;
+    params->show_window = true;
   }
 
   // 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;
+  else if (params->disposition == NEW_FOREGROUND_TAB)

[Evan Stade, 2010/11/09 20:29:23]

maybe NEW_FOREGROUND_TAB can be added to the dispositions below on L194. Also,
can you update the comment in the header about this?

[sadrul, 2010/11/09 20:50:54]

I thought about doing a fallthrough switch-case on this:

switch (params->disposition) {
  case NEW_BACKGROUND_TAB:
    params->tabstrip_add_types &= ~TabStripModel::ADD_SELECTED;
    break;
  case NEW_WINDOW:
  case NEW_POPUP:
    params->show_window = true;
    // Fall-through
  case NEW_FOREGROUND_TAB:
    params->tabstrip_add_types |= TabStripModel::ADD_SELECTED;
    break;

  default:
    break;
}

How does that sound?

[Evan Stade, 2010/11/09 20:56:18]

what's wrong with showing the window for NEW_FOREGROUND? I think Show is
harmless if the window's already showing (actually, it probably Activates the
window, which seems desirable).

if there were a context menu item like "open in incognito" but instead it were
called "open in normal", and you didn't have any normal windows when you
selected it in an OTR window, you'd want to show the new window you had to
create.

[sadrul, 2010/11/09 21:06:39]

[snip]
That makes sense to me (and |show_window| does activate the window if already
exists). However, I do not know what 'activate the window' may mean for
different window-managers, and I am not familiar with this code enough to know
if this can cause problems for some. But if it sounds like an OK change to
you/Ben, I do not personally have a problem with it.

[sadrul, 2010/11/10 05:07:57]

I have gone ahead and made this change. This essentially makes a single change
fix both 62022 and 62137!

+    params->tabstrip_add_types |= TabStripModel::ADD_SELECTED;
 
   // 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;
   }
 }
 
 // This class makes sure the Browser object held in |params| is made visible
@@ skipping 175 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