Make SessionID tolerant of non-tabs.

This reverts r156144.

BUG=107201
TEST=no crashes


Review URL: https://chromiumcodereview.appspot.com/10915246

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@156570 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 25 insertions, 37 deletions

diff --git a/chrome/browser/extensions/extension_tab_util.cc b/chrome/browser/extensions/extension_tab_util.cc
index 1421368..09d20e0 100644
--- a/chrome/browser/extensions/extension_tab_util.cc
+++ b/chrome/browser/extensions/extension_tab_util.cc
@@ -10,7 +10,6 @@
 #include "chrome/browser/net/url_fixer_upper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/session_id.h"
-#include "chrome/browser/sessions/session_tab_helper.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_list.h"
@@ -50,9 +49,6 @@ int ExtensionTabUtil::GetWindowIdOfTabStripModel(
 }
 
 int ExtensionTabUtil::GetTabId(const WebContents* web_contents) {
-  // TODO(avi): Make IdForTab return -1 for non-tabs.
-  if (!SessionTabHelper::FromWebContents(web_contents))
-    return -1;
   return SessionID::IdForTab(web_contents);
 }
 
@@ -61,9 +57,6 @@ std::string ExtensionTabUtil::GetTabStatusText(bool is_loading) {
 }
 
 int ExtensionTabUtil::GetWindowIdOfTab(const WebContents* web_contents) {
-  // TODO(avi): Make IdForWindowContainingTab return -1 for non-tabs.
-  if (!SessionTabHelper::FromWebContents(web_contents))
-    return -1;
   return SessionID::IdForWindowContainingTab(web_contents);
 }
 
diff --git a/chrome/browser/extensions/page_action_controller.cc b/chrome/browser/extensions/page_action_controller.cc
index 6723578..162840f 100644
--- a/chrome/browser/extensions/page_action_controller.cc
+++ b/chrome/browser/extensions/page_action_controller.cc
@@ -10,7 +10,6 @@
 #include "chrome/browser/extensions/tab_helper.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
 #include "chrome/browser/sessions/session_id.h"
-#include "chrome/browser/sessions/session_tab_helper.h"
 #include "chrome/browser/ui/tab_contents/tab_contents.h"
 #include "chrome/common/extensions/extension_set.h"
 #include "chrome/common/chrome_notification_types.h"
@@ -95,12 +94,10 @@ void PageActionController::DidNavigateMainFrame(
   if (current_actions.empty())
     return;
 
-  // TODO(avi): Make IdForTab return -1 for non-tabs.
-  if (SessionTabHelper::FromWebContents(web_contents()))
-    for (size_t i = 0; i < current_actions.size(); ++i) {
-      current_actions[i]->ClearAllValuesForTab(
-          SessionID::IdForTab(web_contents()));
-    }
+  for (size_t i = 0; i < current_actions.size(); ++i) {
+    current_actions[i]->ClearAllValuesForTab(
+        SessionID::IdForTab(web_contents()));
+  }
 
   NotifyChange();
 }
diff --git a/chrome/browser/extensions/script_badge_controller.cc b/chrome/browser/extensions/script_badge_controller.cc
index 7c1ad86..111384a 100644
--- a/chrome/browser/extensions/script_badge_controller.cc
+++ b/chrome/browser/extensions/script_badge_controller.cc
@@ -12,7 +12,6 @@
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/extensions/tab_helper.h"
 #include "chrome/browser/sessions/session_id.h"
-#include "chrome/browser/sessions/session_tab_helper.h"
 #include "chrome/browser/ui/tab_contents/tab_contents.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_action.h"
@@ -56,11 +55,8 @@ void ScriptBadgeController::GetAttentionFor(
   // TODO(jyasskin): Modify the icon's appearance to indicate that the
   // extension is merely asking for permission to run:
   // http://crbug.com/133142
-  // TODO(avi): Make IdForTab return -1 for non-tabs.
-  if (SessionTabHelper::FromWebContents(web_contents())) {
-    script_badge->SetAppearance(SessionID::IdForTab(web_contents()),
-                                ExtensionAction::WANTS_ATTENTION);
-  }
+  script_badge->SetAppearance(SessionID::IdForTab(web_contents()),
+                              ExtensionAction::WANTS_ATTENTION);
 
   NotifyChange();
 }
@@ -251,9 +247,6 @@ bool ScriptBadgeController::MarkExtensionExecuting(
   if (!script_badge)
     return false;
 
-  // TODO(avi): Make IdForTab return -1 for non-tabs.
-  if (!SessionTabHelper::FromWebContents(web_contents()))
-    return true;
   script_badge->SetAppearance(SessionID::IdForTab(web_contents()),
                               ExtensionAction::ACTIVE);
   return true;
diff --git a/chrome/browser/sessions/session_id.cc b/chrome/browser/sessions/session_id.cc
index 0b234c6..5d3c075 100644
--- a/chrome/browser/sessions/session_id.cc
+++ b/chrome/browser/sessions/session_id.cc
@@ -13,14 +13,14 @@ SessionID::SessionID() {
 }
 
 SessionID::id_type SessionID::IdForTab(const content::WebContents* tab) {
-  // Explicitly crash if this isn't a tab. This is temporary during refactoring
-  // and will go away soon.
-  return tab ? SessionTabHelper::FromWebContents(tab)->session_id().id() : -1;
+  const SessionTabHelper* session_tab_helper =
+      tab ? SessionTabHelper::FromWebContents(tab) : NULL;
+  return session_tab_helper ? session_tab_helper->session_id().id() : -1;
 }
 
 SessionID::id_type SessionID::IdForWindowContainingTab(
     const content::WebContents* tab) {
-  // Explicitly crash if this isn't a tab. This is temporary during refactoring
-  // and will go away soon.
-  return tab ? SessionTabHelper::FromWebContents(tab)->window_id().id() : -1;
+  const SessionTabHelper* session_tab_helper =
+      tab ? SessionTabHelper::FromWebContents(tab) : NULL;
+  return session_tab_helper ? session_tab_helper->window_id().id() : -1;
 }
diff --git a/chrome/browser/sessions/session_id.h b/chrome/browser/sessions/session_id.h
index 54f43a7..f4d103b 100644
--- a/chrome/browser/sessions/session_id.h
+++ b/chrome/browser/sessions/session_id.h
@@ -21,16 +21,21 @@ class SessionID {
   SessionID();
   ~SessionID() {}
 
-  // This value is immutable for a given tab. It will be unique across Chrome
-  // within the current session, but may be re-used across sessions. Returns -1
-  // for NULL, and will never return -1 for a valid tab.
+  // If the specified WebContents has a SessionTabHelper (probably because it
+  // was used as the contents of a tab), returns a tab id. This value is
+  // immutable for a given tab. It will be unique across Chrome within the
+  // current session, but may be re-used across sessions. Returns -1
+  // for a NULL WebContents or if the WebContents has no SessionTabHelper.
   static id_type IdForTab(const content::WebContents* tab);
 
-  // If the tab has ever been attached to a window, this is the value of
-  // Browser::session_id().id() for some Browser object. Returns -1 for NULL, or
-  // another value that's not equal to any Browser's id for a tab that has never
-  // been attached to a window. IdForWindowContainingTab() returns the old
-  // window for a tab that's currently being dragged between windows.
+  // If the specified WebContents has a SessionTabHelper (probably because it
+  // was used as the contents of a tab), and has ever been attached to a Browser
+  // window, returns Browser::session_id().id() for that Browser. If the tab is
+  // being dragged between Browser windows, returns the old window's id value.
+  // If the WebContents has a SessionTabHelper but has never been attached to a
+  // Browser window, returns an id value that is different from that of any
+  // Browser. Returns -1 for a NULL WebContents or if the WebContents has no
+  // SessionTabHelper.
   static id_type IdForWindowContainingTab(const content::WebContents* tab);
 
   // Returns the underlying id.