NTP5: Starting implementation of a native menu for showing other device sessions.

chrome/browser/ui/webui/ntp/foreign_session_handler.cc

Index: chrome/browser/ui/webui/ntp/foreign_session_handler.cc
diff --git a/chrome/browser/ui/webui/ntp/foreign_session_handler.cc b/chrome/browser/ui/webui/ntp/foreign_session_handler.cc
index 3d114c4f6983a1bb82e5dda14851339119d9c302..c6d0ad61de40c830ffebe807e7ab6cca7a8e52d8 100644
--- a/chrome/browser/ui/webui/ntp/foreign_session_handler.cc
+++ b/chrome/browser/ui/webui/ntp/foreign_session_handler.cc
@@ -20,6 +20,7 @@
 #include "chrome/browser/sessions/session_restore.h"
 #include "chrome/browser/sync/profile_sync_service.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
+#include "chrome/browser/ui/search/other_device_menu.h"
 #include "chrome/browser/ui/webui/ntp/new_tab_ui.h"
 #include "chrome/browser/ui/webui/session_favicon_source.h"
 #include "chrome/browser/ui/webui/web_ui_util.h"
@@ -29,18 +30,14 @@
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
-#include "content/public/browser/web_ui.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 
 namespace browser_sync {
 
-// Maximum number of session we're going to display on the NTP
+// Maximum number of sessions we're going to display on the NTP
 static const size_t kMaxSessionsToShow = 10;
 
-// Invalid value, used to note that we don't have a tab or window number.
-static const int kInvalidId = -1;
-
 namespace {
 
 // Comparator function for use with std::sort that will sort sessions by
@@ -60,6 +57,92 @@ void ForeignSessionHandler::RegisterUserPrefs(PrefService* prefs) {
                                 PrefService::UNSYNCABLE_PREF);
 }
 
+// static
+void ForeignSessionHandler::OpenForeignSession(
+    content::WebUI* web_ui,
+    const std::string& session_string_value,
+    SessionID::id_type window_num,
+    SessionID::id_type tab_id,
+    const WindowOpenDisposition& disposition) {
+
+  SessionModelAssociator* associator = GetModelAssociator(web_ui);
+  if (!associator)
+    return;
+
+  if (tab_id != kInvalidId) {
+    // We don't actually care about |window_num|, this is just a sanity check.
+    DCHECK_LT(kInvalidId, window_num);
+    const SessionTab* tab;
+    if (!associator->GetForeignTab(session_string_value, tab_id, &tab)) {
+      LOG(ERROR) << "Failed to load foreign tab.";

[Evan Stade, 2012/10/17 21:41:40]

aren't we supposed to use VLOG(1) rather than LOG(ERROR)

[jeremycho, 2012/10/20 23:38:15]

Not sure, it was here already and I see it throughout this file.  Happy to
switch though.

[Evan Stade, 2012/10/22 19:06:42]

nevermind. VLOG(1) replaces LOG(INFO) not LOG(ERROR)

+      return;
+    }
+    if (tab->navigations.size() == 0) {

[Dan Beam, 2012/10/19 04:18:47]

nit: .empty()

[jeremycho, 2012/10/20 23:38:15]

Done.

+      LOG(ERROR) << "Foreign tab no longer has valid navigations.";
+      return;
+    }
+    SessionRestore::RestoreForeignSessionTab(
+        web_ui->GetWebContents(), *tab, disposition);

[Dan Beam, 2012/10/19 04:18:47]

nit: return and no else, IMO

[jeremycho, 2012/10/20 23:38:15]

Done.

+  } else {
+    std::vector<const SessionWindow*> windows;
+    // Note: we don't own the ForeignSessions themselves.
+    if (!associator->GetForeignSession(session_string_value, &windows)) {
+      LOG(ERROR) << "ForeignSessionHandler failed to get session data from"
+          "SessionModelAssociator.";
+      return;
+    }
+    std::vector<const SessionWindow*>::const_iterator iter_begin =
+        windows.begin() + ((window_num == kInvalidId) ? 0 : window_num);

[Evan Stade, 2012/10/17 21:41:40]

remove excess parens

[jeremycho, 2012/10/20 23:38:15]

Done.

+    std::vector<const SessionWindow*>::const_iterator iter_end =
+        ((window_num == kInvalidId) ?

[Evan Stade, 2012/10/17 21:41:40]

here you have 2 sets of excess parens

[jeremycho, 2012/10/20 23:38:15]

Done.

+        std::vector<const SessionWindow*>::const_iterator(windows.end()) :
+        iter_begin + 1);
+    SessionRestore::RestoreForeignSessionWindows(
+        Profile::FromWebUI(web_ui), iter_begin, iter_end);
+  }
+}
+
+// static
+bool ForeignSessionHandler::SessionTabToValue(
+    const SessionTab& tab,
+    DictionaryValue* dictionary) {
+  if (tab.navigations.empty())
+    return false;

[Dan Beam, 2012/10/19 04:18:47]

nit: \n IMO

[jeremycho, 2012/10/20 23:38:15]

Done.

+  int selected_index = std::max(
+      0,

[Dan Beam, 2012/10/19 04:18:47]

nit: you don't really need the std::max() if you've verified that
navigations.size() > 0 (with the .empty()) and assuming
tab.current_navigation_index can't be < 0

[jeremycho, 2012/10/20 23:38:15]

Done.

+      std::min(tab.current_navigation_index,
+               static_cast<int>(tab.navigations.size() - 1)));
+  const TabNavigation& current_navigation =
+      tab.navigations.at(selected_index);
+  GURL tab_url = current_navigation.virtual_url();
+  if (tab_url == GURL(chrome::kChromeUINewTabURL))
+    return false;

[Dan Beam, 2012/10/19 04:18:47]

nit: \n IMO

[jeremycho, 2012/10/20 23:38:15]

Done.

+  NewTabUI::SetUrlTitleAndDirection(dictionary, current_navigation.title(),
+                                    tab_url);
+  dictionary->SetString("type", "tab");
+  dictionary->SetDouble("timestamp",
+                        static_cast<double>(tab.timestamp.ToInternalValue()));
+  // TODO(jeremycho): This should probably be renamed to tabId to avoid
+  // confusion with the ID corresponding to a session.  Investigate all the
+  // places (C++ and JS) where this is being used.  (http://crbug.com/154865).
+  dictionary->SetInteger("sessionId", tab.tab_id.id());
+  return true;
+}
+
+// static
+SessionModelAssociator* ForeignSessionHandler::GetModelAssociator(
+    content::WebUI* web_ui) {
+  Profile* profile = Profile::FromWebUI(web_ui);
+  ProfileSyncService* service =
+      ProfileSyncServiceFactory::GetInstance()->GetForProfile(profile);
+
+  // Only return the associator if it exists and it is done syncing sessions.
+  if (service && service->ShouldPushChanges())
+    return service->GetSessionModelAssociator();
+
+  return NULL;
+}
+
 void ForeignSessionHandler::RegisterMessages() {
   Init();
   web_ui()->RegisterMessageCallback("getForeignSessions",
@@ -113,17 +196,6 @@ void ForeignSessionHandler::Observe(
   }
 }
 
-SessionModelAssociator* ForeignSessionHandler::GetModelAssociator() {
-  Profile* profile = Profile::FromWebUI(web_ui());
-  ProfileSyncService* service =
-      ProfileSyncServiceFactory::GetInstance()->GetForProfile(profile);
-
-  // Only return the associator if it exists and it is done syncing sessions.
-  if (service && service->ShouldPushChanges())
-    return service->GetSessionModelAssociator();
-
-  return NULL;
-}
 
 bool ForeignSessionHandler::IsTabSyncEnabled() {
   Profile* profile = Profile::FromWebUI(web_ui());
@@ -138,7 +210,7 @@ string16 ForeignSessionHandler::FormatSessionTime(const base::Time& time) {
 }
 
 void ForeignSessionHandler::HandleGetForeignSessions(const ListValue* args) {
-  SessionModelAssociator* associator = GetModelAssociator();
+  SessionModelAssociator* associator = GetModelAssociator(web_ui());
   std::vector<const SyncedSession*> sessions;
 
   ListValue session_list;
@@ -229,43 +301,11 @@ void ForeignSessionHandler::HandleOpenForeignSession(const ListValue* args) {
     return;
   }
 
-  SessionModelAssociator* associator = GetModelAssociator();
-  if (!associator)
-    return;
+  WindowOpenDisposition disposition =
+      web_ui_util::GetDispositionFromClick(args, 3);
 
-  if (tab_id != kInvalidId) {
-    // We don't actually care about |window_num|, this is just a sanity check.
-    DCHECK_LT(kInvalidId, window_num);
-    const SessionTab* tab;
-    if (!associator->GetForeignTab(session_string_value, tab_id, &tab)) {
-      LOG(ERROR) << "Failed to load foreign tab.";
-      return;
-    }
-    if (tab->navigations.size() == 0) {
-      LOG(ERROR) << "Foreign tab no longer has valid navigations.";
-      return;
-    }
-    WindowOpenDisposition disposition =
-        web_ui_util::GetDispositionFromClick(args, 3);
-    SessionRestore::RestoreForeignSessionTab(
-        web_ui()->GetWebContents(), *tab, disposition);
-  } else {
-    std::vector<const SessionWindow*> windows;
-    // Note: we don't own the ForeignSessions themselves.
-    if (!associator->GetForeignSession(session_string_value, &windows)) {
-      LOG(ERROR) << "ForeignSessionHandler failed to get session data from"
-          "SessionModelAssociator.";
-      return;
-    }
-    std::vector<const SessionWindow*>::const_iterator iter_begin =
-        windows.begin() + ((window_num == kInvalidId) ? 0 : window_num);
-    std::vector<const SessionWindow*>::const_iterator iter_end =
-        ((window_num == kInvalidId) ?
-        std::vector<const SessionWindow*>::const_iterator(windows.end()) :
-        iter_begin + 1);
-    SessionRestore::RestoreForeignSessionWindows(
-        Profile::FromWebUI(web_ui()), iter_begin, iter_end);
-  }
+  OpenForeignSession(
+      web_ui(), session_string_value, window_num, tab_id, disposition);
 }
 
 void ForeignSessionHandler::HandleSetForeignSessionCollapsed(
@@ -306,41 +346,20 @@ void ForeignSessionHandler::HandleShowOtherDeviceSessionPopup(
   std::string session_string_value;
   CHECK(args->GetString(0, &session_string_value));
 
-  // Extract horizontal coordinate of the click within the application's client
-  // area.
-  double client_x;
-  CHECK(args->GetDouble(1, &client_x));
-
-  // Extract vertical coordinate of the click within the application's client
-  // area.
-  double client_y;
-  CHECK(args->GetDouble(2, &client_y));
+  // Extract horizontal coordinate of the click relative to the origin of the
+  // screen coordinate system.
+  double screen_x;
+  CHECK(args->GetDouble(1, &screen_x));
 
-  // TODO(vadimt): implement this method.
-}
+  // Extract vertical coordinate of the click relative to the origin of the
+  // screen coordinate system.
+  double screen_y;
+  CHECK(args->GetDouble(2, &screen_y));
 
-bool ForeignSessionHandler::SessionTabToValue(
-    const SessionTab& tab,
-    DictionaryValue* dictionary) {
-  if (tab.navigations.empty())
-    return false;
-  int selected_index = tab.current_navigation_index;
-  selected_index = std::max(
-      0,
-      std::min(selected_index,
-               static_cast<int>(tab.navigations.size() - 1)));
-  const TabNavigation& current_navigation =
-      tab.navigations.at(selected_index);
-  GURL tab_url = current_navigation.virtual_url();
-  if (tab_url == GURL(chrome::kChromeUINewTabURL))
-    return false;
-  NewTabUI::SetUrlTitleAndDirection(dictionary, current_navigation.title(),
-                                    tab_url);
-  dictionary->SetString("type", "tab");
-  dictionary->SetDouble("timestamp",
-                        static_cast<double>(tab.timestamp.ToInternalValue()));
-  dictionary->SetInteger("sessionId", tab.tab_id.id());
-  return true;
+    OtherDeviceMenu* menu = new OtherDeviceMenu(
+        web_ui(),
+        session_string_value, gfx::Point(screen_x, screen_y));
+    menu->ShowMenu();
 }
 
 bool ForeignSessionHandler::SessionWindowToValue(