Fixing activation problem: Pages which refresh themselves should not get focus

chrome/browser/ui/browser.cc

Index: chrome/browser/ui/browser.cc
diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index 987af6c2271b886ff7cef3465471b2785fb1e1a5..ad319298cebb406c825f68dfc04a66c00a805481 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -2697,6 +2697,14 @@ void Browser::UpdateUIForNavigationInTab(TabContentsWrapper* contents,
   // navigating away from the new tab page.
   ScheduleUIUpdate(contents->web_contents(), content::INVALIDATE_TYPE_URL);
 
+  // In case of multiple windows we do not want to switch windows when this is

[sky, 2012/05/21 17:29:15]

This comment is confusing.

+  // not user initiated (e.g. automatic page refresh) and the current window
+  // is not the window we are targeting.
+  if (!user_initiated &&

[sky, 2012/05/21 17:29:15]

I think you missed a comment from last round: "Combine this with the Focus()
request since that's what it is intended for." Basicaly, I'm saying combine this
with the if on 2708:

if (user_initiated && contents_is_selected && window()->IsActive()

[Mr4D (OOO till 08-26), 2012/05/21 18:18:37]

Ahh. In that case I have misunderstood. Done.

+      (GetSelectedWebContents() != contents->web_contents() ||
+       !window()->IsActive()))
+    return;
+
   if (contents_is_selected)
     contents->web_contents()->Focus();
 }
@@ -3409,7 +3417,13 @@ WebContents* Browser::OpenURLFromTab(WebContents* source,
   nav_params.referrer = params.referrer;
   nav_params.disposition = params.disposition;
   nav_params.tabstrip_add_types = TabStripModel::ADD_NONE;
-  nav_params.window_action = browser::NavigateParams::SHOW_WINDOW;
+
+  // If the current tab is initiating this, we don't want to update.

[sky, 2012/05/21 17:29:15]

You comment doesn't match what you're doing.

[Mr4D (OOO till 08-26), 2012/05/21 18:18:37]

Done.

+  if (nav_params.window_action != browser::NavigateParams::NO_ACTION ||

[sky, 2012/05/21 17:29:15]

I don't understand this first check. The default is NO_ACTION, so this should
never be true.

[Mr4D (OOO till 08-26), 2012/05/21 18:18:37]

Done. Yes, that is correct (forgotten to remove that).

+      params.disposition != CURRENT_TAB ||

[sky, 2012/05/21 17:29:15]

Why are you checking this?

[Mr4D (OOO till 08-26), 2012/05/21 18:18:37]

Because when a new thing (Popup / Window / Tab) gets opened we want to show it -
even if the (what ever) was not yet active. In case of an automated update a
"CURRENT_TAB" gets passed down which matches our requirement. I added a comment.
But please let me know if you feel that that might be too limiting.

[sky, 2012/05/21 20:42:52]

I believe Navigate takes care of that for you. See the comment and
implementation where it sets the window_action to SHOW.

[Mr4D (OOO till 08-26), 2012/05/21 21:27:12]

Okay, I removed the special check for the current_tab since the latter function
will set the window_action as described.

+      (GetSelectedWebContents() == source && window()->IsActive()))
+    nav_params.window_action = browser::NavigateParams::SHOW_WINDOW;
+
   nav_params.user_gesture = true;
   nav_params.override_encoding = params.override_encoding;
   nav_params.is_renderer_initiated = params.is_renderer_initiated;