Sessions API - previously Session Restore API. Supports restoring currently open foreign windows an…

chrome/browser/extensions/api/sessions/sessions_api.cc

-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2013 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/extensions/api/session_restore/session_restore_api.h"
+#include "chrome/browser/extensions/api/sessions/sessions_api.h"
 
 #include <vector>
 
 #include "base/i18n/rtl.h"
 #include "base/lazy_instance.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/extensions/extension_function_dispatcher.h"
 #include "chrome/browser/extensions/extension_function_registry.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
+#include "chrome/browser/extensions/window_controller.h"
+#include "chrome/browser/extensions/window_controller_list.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/session_restore.h"
 #include "chrome/browser/sessions/tab_restore_service_delegate.h"
 #include "chrome/browser/sessions/tab_restore_service_factory.h"
 #include "chrome/browser/sync/glue/session_model_associator.h"
 #include "chrome/browser/sync/glue/synced_session.h"
 #include "chrome/browser/sync/profile_sync_service.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/host_desktop.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "content/public/browser/web_contents.h"
+#include "extensions/common/error_utils.h"
 #include "ui/base/layout.h"
 
-
 namespace {
 
 const unsigned int kMaxRecentlyClosedSessionResults = 25;
-const char kRecentlyClosedListEmpty[] =
+const unsigned int kMaxSyncedSessionResults = 10;
+const char kRecentlyClosedListEmptyError[] =
     "There are no recently closed sessions.";
-const char kInvalidSessionId[] = "Invalid session id.";
+const char kInvalidSessionIdError[] = "Invalid session id.";
 const char kNoBrowserToRestoreSession[] =
     "There are no browser windows to restore the session.";
+const char kSessionsSyncNotEnabledError[] = "Syncing sessions is not enabled.";
+const char kSyncedSessionsListEmptyError[] = "There are no foreign sessions.";
+const char kSessionNotFoundError[] = "Session is not found.";
+const char kSessionSyncError[] = "Session sync error.";
+const char kForeignIdSeparator = '.';
+
+const char kReturnObjectError[] = "Error returning window or tab.";
+const char kWindowNotFoundError[] = "No window with id: *.";
+const char kNoCurrentWindowError[] = "No current window.";
+const char kWindowRestoreError[] = "Error restoring foreign window.";
+
+std::string CreateForeignId(const std::string& session_tag, int id) {
+  return (session_tag + kForeignIdSeparator + base::IntToString(id));
+}

[not at google - send to devlin, 2013/08/06 19:04:09]

Rather than sharing a magic variable (kForeignIdSeparator) and having util
classes (those 3), it would be nicer to encapsulate a foreign ID within a little
class, providing ToString() and FromString() methods.

[Kristen Dwan, 2013/08/12 15:11:30]

Done.

+
+// Comparator function for use with std::sort that will sort sessions by
+// descending modified_time (i.e., most recent first).
+bool SortSessionsByRecency(const browser_sync::SyncedSession* s1,
+                           const browser_sync::SyncedSession* s2) {
+  return s1->modified_time > s2->modified_time;
+}
+
+bool GetWindowFromWindowID(UIThreadExtensionFunction* function,
+                           int window_id,
+                           extensions::WindowController** controller) {

[not at google - send to devlin, 2013/08/06 19:04:09]

Please pull this into a common place and share it with the windows API code:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

[Kristen Dwan, 2013/08/12 15:11:30]

Done.

+  if (window_id == extension_misc::kCurrentWindowId) {
+    extensions::WindowController* extension_window_controller =
+        function->dispatcher()->delegate()->GetExtensionWindowController();
+    // If there is a window controller associated with this extension, use that.
+    if (extension_window_controller) {
+      *controller = extension_window_controller;
+    } else {
+      // Otherwise get the focused or most recently added window.
+      *controller = extensions::WindowControllerList::GetInstance()->
+          CurrentWindowForFunction(function);
+    }
+    if (!(*controller)) {
+      function->SetError(kNoCurrentWindowError);
+      return false;
+    }
+  } else {
+    *controller = extensions::WindowControllerList::GetInstance()->
+        FindWindowForFunctionById(function, window_id);
+    if (!(*controller)) {
+      function->SetError(extensions::ErrorUtils::FormatErrorMessage(
+          kWindowNotFoundError, base::IntToString(window_id)));
+      return false;
+    }
+  }
+  return true;
+}
+
+bool IsLocalSession(const std::string& id) {
+  return (id.find(kForeignIdSeparator) == std::string::npos);
+}
+
+bool SplitId(const std::string& sid, std::string* session_tag, int* id) {
+  std::size_t separator_index = sid.find(kForeignIdSeparator);
+  *session_tag = sid.substr(0, separator_index);
+  if (!base::StringToInt(sid.substr(separator_index + 1), id))
+    return false;
+  return true;
+}
 
 }  // namespace
 
 namespace extensions {
 
-namespace GetRecentlyClosed = api::session_restore::GetRecentlyClosed;
-namespace Restore = api::session_restore::Restore;
+namespace GetRecentlyClosed = api::sessions::GetRecentlyClosed;
+namespace GetDevices = api::sessions::GetDevices;
+namespace Restore = api::sessions::Restore;
 namespace tabs = api::tabs;
 namespace windows = api::windows;
-namespace session_restore = api::session_restore;
 
-scoped_ptr<tabs::Tab> SessionRestoreGetRecentlyClosedFunction::CreateTabModel(
-    const TabRestoreService::Tab& tab, int selected_index) {
+scoped_ptr<tabs::Tab> SessionsGetRecentlyClosedFunction::CreateTabModel(
+    const TabRestoreService::Tab& tab, const int* const session_id,

[not at google - send to devlin, 2013/08/06 19:04:09]

The second const isn't necessary.

That said I think it should be the job of the caller to decide what the session
ID is. Pass it in directly. Callers can pass in tab.id if they want.

[Kristen Dwan, 2013/08/12 15:11:30]

Done.

+    int selected_index) {
   scoped_ptr<tabs::Tab> tab_struct(new tabs::Tab);
   const sessions::SerializedNavigationEntry& current_navigation =
       tab.navigations[tab.current_navigation_index];
   GURL gurl = current_navigation.virtual_url();
   std::string title = UTF16ToUTF8(current_navigation.title());
 
+  if (session_id) {
+    tab_struct->session_id.reset(
+        new std::string(base::IntToString(*session_id)));
+  } else {
+    tab_struct->session_id.reset(
+        new std::string(base::IntToString(tab.id)));
+  }
   tab_struct->url.reset(new std::string(gurl.spec()));
   tab_struct->title.reset(new std::string(title.empty() ? gurl.spec() : title));
   tab_struct->index = tab.tabstrip_index;
   tab_struct->pinned = tab.pinned;
-  tab_struct->id = tab.id;
-  tab_struct->window_id = tab.browser_id;
   tab_struct->index = tab.tabstrip_index;
   tab_struct->pinned = tab.pinned;
   tab_struct->selected = tab.tabstrip_index == selected_index;
   tab_struct->active = false;
   tab_struct->highlighted = false;
   tab_struct->incognito = false;
   ExtensionTabUtil::ScrubTabForExtension(GetExtension(),
                                          tab_struct.get());
   return tab_struct.Pass();
 }
 
 scoped_ptr<windows::Window>
-    SessionRestoreGetRecentlyClosedFunction::CreateWindowModel(
-        const TabRestoreService::Window& window) {
+    SessionsGetRecentlyClosedFunction::CreateWindowModel(
+        const TabRestoreService::Window& window, int session_id) {
   scoped_ptr<windows::Window> window_struct(new windows::Window);
   DCHECK(!window.tabs.empty());
 
   scoped_ptr<std::vector<linked_ptr<tabs::Tab> > > tabs(
       new std::vector<linked_ptr<tabs::Tab> >);
   for (size_t i = 0; i < window.tabs.size(); ++i) {
-    tabs->push_back(make_linked_ptr(CreateTabModel(window.tabs[i],
+    tabs->push_back(make_linked_ptr(CreateTabModel(window.tabs[i], NULL,
         window.selected_tab_index).release()));
   }
+  window_struct->session_id.reset(
+      new std::string(base::IntToString(session_id)));
   window_struct->tabs.reset(tabs.release());
   window_struct->incognito = false;
   window_struct->always_on_top = false;
   window_struct->focused = false;
   window_struct->type = windows::Window::TYPE_NORMAL;
   window_struct->state = windows::Window::STATE_NORMAL;
   return window_struct.Pass();
 }
 
-scoped_ptr<session_restore::ClosedEntry>
-    SessionRestoreGetRecentlyClosedFunction::CreateEntryModel(
+scoped_ptr<api::sessions::Session>
+    SessionsGetRecentlyClosedFunction::CreateSessionModel(
         const TabRestoreService::Entry* entry) {
-  scoped_ptr<session_restore::ClosedEntry> entry_struct(
-      new session_restore::ClosedEntry);
+  scoped_ptr<api::sessions::Session> session_struct(new api::sessions::Session);
   switch (entry->type) {
     case TabRestoreService::TAB:
-      entry_struct->tab.reset(CreateTabModel(
-          *static_cast<const TabRestoreService::Tab*>(entry), -1).release());
+      session_struct->tab.reset(CreateTabModel(
+          *static_cast<const TabRestoreService::Tab*>(entry), &entry->id, -1)
+          .release());
       break;
     case TabRestoreService::WINDOW:
-      entry_struct->window.reset(CreateWindowModel(
-          *static_cast<const TabRestoreService::Window*>(entry)).release());
+      session_struct->window.reset(CreateWindowModel(
+          *static_cast<const TabRestoreService::Window*>(entry), entry->id)
+          .release());
       break;
     default:
       NOTREACHED();
   }
-  entry_struct->timestamp = entry->timestamp.ToTimeT();
-  entry_struct->id = entry->id;
-  return entry_struct.Pass();
+  session_struct->last_modified = entry->timestamp.ToTimeT();
+  return session_struct.Pass();
 }
 
-bool SessionRestoreGetRecentlyClosedFunction::RunImpl() {
+bool SessionsGetRecentlyClosedFunction::RunImpl() {
   scoped_ptr<GetRecentlyClosed::Params> params(
       GetRecentlyClosed::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
   unsigned int max_results = kMaxRecentlyClosedSessionResults;
   if (params->options && params->options->max_results)
     max_results = *params->options->max_results;
   EXTENSION_FUNCTION_VALIDATE(max_results >= 0 &&
                               max_results <= kMaxRecentlyClosedSessionResults);
 
-  std::vector<linked_ptr<session_restore::ClosedEntry> > result;
+  std::vector<linked_ptr<api::sessions::Session> > result;
   TabRestoreService* tab_restore_service =
       TabRestoreServiceFactory::GetForProfile(profile());
   DCHECK(tab_restore_service);
 
   // List of entries. They are ordered from most to least recent.
   // We prune the list to contain max 25 entries at any time and removes
   // uninteresting entries.
   TabRestoreService::Entries entries = tab_restore_service->entries();
   for (TabRestoreService::Entries::const_iterator it = entries.begin();
        it != entries.end() && result.size() < max_results; ++it) {
     TabRestoreService::Entry* entry = *it;
     if (!params->options || params->options->entry_type ==
         GetRecentlyClosed::Params::Options::ENTRY_TYPE_NONE) {
       // Include both tabs and windows if type is not defined.
-      result.push_back(make_linked_ptr(CreateEntryModel(entry).release()));
+      result.push_back(make_linked_ptr(CreateSessionModel(entry).release()));
     } else if (
         (params->options->entry_type ==
             GetRecentlyClosed::Params::Options::ENTRY_TYPE_TAB &&
                 entry->type == TabRestoreService::TAB) ||
         (params->options->entry_type ==
             GetRecentlyClosed::Params::Options::ENTRY_TYPE_WINDOW &&
                 entry->type == TabRestoreService::WINDOW)) {
-      result.push_back(make_linked_ptr(CreateEntryModel(entry).release()));
+      result.push_back(make_linked_ptr(CreateSessionModel(entry).release()));
     }
   }
 
   results_ = GetRecentlyClosed::Results::Create(result);
   return true;
 }
 
-bool SessionRestoreRestoreFunction::RunImpl() {
-  scoped_ptr<Restore::Params> params(Restore::Params::Create(*args_));
+scoped_ptr<tabs::Tab> SessionsGetDevicesFunction::CreateTabModel(

[not at google - send to devlin, 2013/08/06 19:04:09]

what's the difference between this and the other CreateTabModel?

[Kristen Dwan, 2013/08/12 15:11:30]

This takes a SessionTab and the other takes a TabRestoreService::Tab which don't
share a superclass. Made helpers that group similar behavior... should have done
that before and not just modified the old CLs code.. my bad!

+    const SessionTab& tab, int selected_index, int tab_index,
+    const std::string& session_tag) {
+  scoped_ptr<tabs::Tab> tab_struct(new tabs::Tab);
+  const sessions::SerializedNavigationEntry& current_navigation =
+      tab.navigations[tab.current_navigation_index];
+  const GURL& gurl = current_navigation.virtual_url();
+  std::string title = UTF16ToUTF8(current_navigation.title());
+
+  tab_struct->session_id.reset(
+      new std::string(CreateForeignId(session_tag, tab.tab_id.id())));
+  tab_struct->url.reset(new std::string(gurl.spec()));
+  tab_struct->title.reset(new std::string(title.empty() ? gurl.spec() : title));
+  tab_struct->pinned = tab.pinned;
+  tab_struct->index = tab_index;
+  tab_struct->pinned = tab.pinned;
+  tab_struct->selected = tab_index == selected_index;
+  tab_struct->active = false;
+  tab_struct->highlighted = false;
+  tab_struct->incognito = false;
+  ExtensionTabUtil::ScrubTabForExtension(GetExtension(), tab_struct.get());
+  return tab_struct.Pass();
+}
+
+scoped_ptr<windows::Window> SessionsGetDevicesFunction::CreateWindowModel(

[not at google - send to devlin, 2013/08/06 19:04:09]

ditto?

[Kristen Dwan, 2013/08/12 15:11:30]

SessionWindow VS TabRestoreService::Window.

Changed to helper. Need to dig a little deeper to find values for
top/right/left/bottom so that can be moved to the helper and the values won't be
optionals.

+        const SessionWindow& window, const std::string& session_tag) {
+  scoped_ptr<windows::Window> window_struct(new windows::Window);
+  DCHECK(!window.tabs.empty());
+
+  scoped_ptr<std::vector<linked_ptr<tabs::Tab> > > tabs(
+      new std::vector<linked_ptr<tabs::Tab> >);
+  for (size_t i = 0; i < window.tabs.size(); ++i) {
+    tabs->push_back(make_linked_ptr(
+        CreateTabModel(*window.tabs[i], window.selected_tab_index, i,
+                       session_tag).release()));
+  }
+  window_struct->tabs = tabs.Pass();
+
+  window_struct->session_id.reset(
+      new std::string(CreateForeignId(session_tag, window.window_id.id())));
+  window_struct->left.reset(new int(window.bounds.x()));
+  window_struct->top.reset(new int(window.bounds.y()));
+  window_struct->width.reset(new int(window.bounds.width()));
+  window_struct->height.reset(new int(window.bounds.height()));
+  window_struct->incognito = false;
+  window_struct->always_on_top = false;
+  window_struct->focused = false;
+
+  switch (window.type) {
+    case Browser::TYPE_TABBED:
+      window_struct->type = windows::Window::TYPE_NORMAL;
+      break;
+    case Browser::TYPE_POPUP:
+      window_struct->type = windows::Window::TYPE_POPUP;
+      break;
+    default:
+      window_struct->type = windows::Window::TYPE_NONE;
+  }
+
+  switch (window.show_state) {
+    case ui::SHOW_STATE_NORMAL:
+      window_struct->state = windows::Window::STATE_NORMAL;
+      break;
+    case ui::SHOW_STATE_MINIMIZED:
+      window_struct->state = windows::Window::STATE_MINIMIZED;
+      break;
+    case ui::SHOW_STATE_MAXIMIZED:
+      window_struct->state = windows::Window::STATE_MAXIMIZED;
+      break;
+    case ui::SHOW_STATE_FULLSCREEN:
+      window_struct->state = windows::Window::STATE_FULLSCREEN;
+      break;
+    default:
+      window_struct->state = windows::Window::STATE_NONE;
+  }
+  return window_struct.Pass();
+}
+
+scoped_ptr<api::sessions::Session>
+    SessionsGetDevicesFunction::CreateSessionModel(

[not at google - send to devlin, 2013/08/06 19:04:09]

ditto?

[Kristen Dwan, 2013/08/12 15:11:30]

SessionWindow vs. TabRestoreService::Entry*. put in helper.

+        const SessionWindow& window, const std::string& session_tag) {
+  scoped_ptr<api::sessions::Session> session_struct(new api::sessions::Session);
+  session_struct->last_modified = window.timestamp.ToTimeT();
+  session_struct->window.reset(
+      CreateWindowModel(window, session_tag).release());
+  return session_struct.Pass();
+}
+
+scoped_ptr<api::sessions::Device> SessionsGetDevicesFunction::CreateDeviceModel(
+    const browser_sync::SyncedSession* session) {
+  scoped_ptr<api::sessions::Device> device_struct(new api::sessions::Device);
+  device_struct->info = session->session_name;
+
+  for (browser_sync::SyncedSession::SyncedWindowMap::const_iterator it =
+       session->windows.begin(); it != session->windows.end(); ++it) {
+    device_struct->sessions.push_back(make_linked_ptr(CreateSessionModel(
+        *it->second, session->session_tag).release()));
+  }
+  return device_struct.Pass();
+}
+
+bool SessionsGetDevicesFunction::RunImpl() {
+  ProfileSyncService* service =
+      ProfileSyncServiceFactory::GetInstance()->GetForProfile(profile());
+  if (!(service && service->GetPreferredDataTypes().Has(syncer::SESSIONS))) {
+    SetError(kSessionsSyncNotEnabledError);

[not at google - send to devlin, 2013/08/06 19:04:09]

Not sure about this one. There should be a way for extensions to know whether
session sync is enabled, but can't be via an error message. That canonicalises
error messages, which is *not* what we want.

I.e. as an extension author, how am I supposed to tell? String-compare error
messages? Not good.

If only we had documented and strict error codes in APIs - but we don't. It's a
shame.

Here are some options:

1 - have another API call, chrome.sessions.canGetDevices(...). Ugh.

2 - make devices null if session sync isn't enabled. A bit bug-inducing since
extensions that go "if (result.devices)" wouldn't be able to tell between an
empty list and null. Obviously that's a bug in the extension, but it does imply
a confusing API.

3 - make results an object rather than an array. too much nesting, yuck.

Of those, (2) is by far the most preferable. Any other ideas?

[Kristen Dwan, 2013/08/12 15:11:30]

I can't think of anything that would be clean and allow you to distinguish
between the two in (2). I also don't think it would be a big deal for the
extension -- a message such as "no devices available, do you have Chrome sync
set up?" or what not.. would be acceptable. Either way, there is no information
available for the extension to display so it doesn't make too much of a
difference to them for what reason it is. The user could figure out between the
two.

[not at google - send to devlin, 2013/08/12 23:51:34]

This sounds good to me.

+    return false;
+  }
+  unsigned int max_results = kMaxSyncedSessionResults;

[not at google - send to devlin, 2013/08/06 19:04:09]

just "int".

[Kristen Dwan, 2013/08/12 15:11:30]

Done.

+  scoped_ptr<GetDevices::Params> params(
+      GetDevices::Params::Create(*args_));

[not at google - send to devlin, 2013/08/06 19:04:09]

1 line?

[Kristen Dwan, 2013/08/12 15:11:30]

Done.

   EXTENSION_FUNCTION_VALIDATE(params.get());
+  if (params->max_results.get())
+    max_results = *params->max_results.get();
+  EXTENSION_FUNCTION_VALIDATE(max_results >= 0 &&
+                              max_results <= kMaxSyncedSessionResults);
 
-  Browser* browser =
-      chrome::FindBrowserWithProfile(profile(),
-                                     chrome::HOST_DESKTOP_TYPE_NATIVE);
-  if (!browser) {
-    error_ = kNoBrowserToRestoreSession;
+  browser_sync::SessionModelAssociator* associator =
+      service->GetSessionModelAssociator();
+  std::vector<const browser_sync::SyncedSession*> sessions;
+  if (!associator) {
+    SetError(kSessionSyncError);
+    return false;
+  }
+  if (!associator->GetAllForeignSessions(&sessions)) {
+    SetError(kSyncedSessionsListEmptyError);

[not at google - send to devlin, 2013/08/06 19:04:09]

This isn't really an *error* case. Similar as before.

Speaking of which, all of these corner-ish cases, we should document in some
way.

[Kristen Dwan, 2013/08/12 15:11:30]

would you recommend just returning true here then w/ null results?

[not at google - send to devlin, 2013/08/12 23:51:34]

empty results seems consistent.

     return false;
   }
 
+  std::vector<linked_ptr<api::sessions::Device> > result;
+  // Sort sessions from most recent to least recent.
+  std::sort(sessions.begin(), sessions.end(), SortSessionsByRecency);
+  for (size_t i = 0; i < sessions.size() && result.size() < max_results; ++i) {
+    result.push_back(make_linked_ptr(CreateDeviceModel(sessions[i]).release()));
+  }
+
+  results_ = GetDevices::Results::Create(result);
+  return true;
+}
+
+bool SessionsRestoreFunction::RestoreMostRecentlyClosed(Browser* browser) {
   TabRestoreService* tab_restore_service =
       TabRestoreServiceFactory::GetForProfile(profile());
   TabRestoreServiceDelegate* delegate =
       TabRestoreServiceDelegate::FindDelegateForWebContents(
           browser->tab_strip_model()->GetActiveWebContents());
   DCHECK(delegate);
   chrome::HostDesktopType host_desktop_type = browser->host_desktop_type();
   TabRestoreService::Entries entries = tab_restore_service->entries();
 
   if (entries.empty()) {
-    error_ = kRecentlyClosedListEmpty;
+    SetError(kRecentlyClosedListEmptyError);

[not at google - send to devlin, 2013/08/06 19:04:09]

also not an error - and this function shouldn't be returning false (so -
existing bug).

hm, we should have a CHECK somewhere in the extension system that "return false"
also sets error_. Anyhow.

[Kristen Dwan, 2013/08/12 15:11:30]

this is how the current sessionRestore api implementation is. should it change
to just return null? thoughts?

the reason it seems like an error to me is if you're making a call
chrome.sessions.restore() when there is nothing to restore.. returning null /
empty would tell you that nothing was restored but idk if that is the behavior
we want? you shouldn't be able to successfully call restore when there is
nothing to be restored.

[not at google - send to devlin, 2013/08/12 23:51:34]

Yeah ok I might have gotten carried away with making this not errors. This
should probably be an error.

     return false;
   }
 
-  if (!params->id) {
-    tab_restore_service->RestoreMostRecentEntry(delegate, host_desktop_type);
-    return true;
+  bool is_window = entries.front()->type == TabRestoreService::WINDOW;

[not at google - send to devlin, 2013/08/06 19:04:09]

inline this?

[Kristen Dwan, 2013/08/12 15:11:30]

Done.

+  std::vector<content::WebContents*> contents =
+      tab_restore_service->RestoreMostRecentEntry(delegate, host_desktop_type);
+  if (!contents.size()) {
+    SetError(kReturnObjectError);

[not at google - send to devlin, 2013/08/06 19:04:09]

I think we can do a better job of an error message here.

[Kristen Dwan, 2013/08/12 15:11:30]

got rid of this. backend calls are always returning webcontents, so it should
never be the case that it's empty. put in dcheck.

+    return false;
+  }
+
+  if (is_window) {
+    WindowController* controller;

[not at google - send to devlin, 2013/08/06 19:04:09]

always initialise pointers to NULL in situations like these.

[Kristen Dwan, 2013/08/12 15:11:30]

Done.

+    if (!GetWindowFromWindowID(this,
+                               ExtensionTabUtil::GetWindowIdOfTab(contents[0]),
+                               &controller))
+      return false;

[not at google - send to devlin, 2013/08/06 19:04:09]

error message

[Kristen Dwan, 2013/08/12 15:11:30]

Done.

+    SetResult(controller->CreateWindowValueWithTabs(GetExtension()));
+  } else {
+    SetResult(ExtensionTabUtil::CreateTabValue(contents[0], GetExtension()));
+  }
+  return true;
+}
+
+bool SessionsRestoreFunction::RestoreLocalSession(int id, Browser* browser) {
+  TabRestoreService* tab_restore_service =
+      TabRestoreServiceFactory::GetForProfile(profile());
+  TabRestoreServiceDelegate* delegate =
+      TabRestoreServiceDelegate::FindDelegateForWebContents(
+          browser->tab_strip_model()->GetActiveWebContents());

[not at google - send to devlin, 2013/08/06 19:04:09]

delegate isn't used until way later, grab it then.

and is it really the case that it can't be NULL?

[Kristen Dwan, 2013/08/12 15:11:30]

"|delegate| will be NULL in cases where one isn't already available (eg, when
invoked on Mac OS X with no windows open)" from the deepest function call in the
stack that this gets passed to.. once again, my bad - shouldn't have relied on
the previous code. sorry :/

+  DCHECK(delegate);
+  chrome::HostDesktopType host_desktop_type = browser->host_desktop_type();
+  TabRestoreService::Entries entries = tab_restore_service->entries();
+
+  if (entries.empty()) {
+    SetError(kRecentlyClosedListEmptyError);
+    return false;
   }
 
   // Check if the recently closed list contains an entry with the provided id.
   bool is_valid_id = false;
+  bool is_window = false;

[not at google - send to devlin, 2013/08/06 19:04:09]

rather than hold onto these two values, perhaps find a pointer to the entry.
checks for is_valid_id would be replaced by entry != NULL. CHeck for is_window
would be directly inspecting the type.

[Kristen Dwan, 2013/08/12 15:11:30]

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ses...

when it's a window, the entry gets deleted :(

   for (TabRestoreService::Entries::iterator it = entries.begin();
        it != entries.end(); ++it) {
-    if ((*it)->id == *params->id) {
+    if ((*it)->id == id) {
       is_valid_id = true;
+      // The only time a full window is being restored is if the entry ID
+      // matches the provided ID and the entry type is Window.
+      is_window = (*it)->type == TabRestoreService::WINDOW;
       break;
     }
-
     // For Window entries, see if the ID matches a tab. If so, report true for
     // the window as the Entry.
     if ((*it)->type == TabRestoreService::WINDOW) {
       std::vector<TabRestoreService::Tab>& tabs =
           static_cast<TabRestoreService::Window*>(*it)->tabs;
       for (std::vector<TabRestoreService::Tab>::iterator tab_it = tabs.begin();
            tab_it != tabs.end(); ++tab_it) {
-        if ((*tab_it).id == *params->id) {
+        if ((*tab_it).id == id) {
           is_valid_id = true;
           break;
         }
       }
     }
   }
 
   if (!is_valid_id) {
-    error_ = kInvalidSessionId;
+    SetError(kInvalidSessionIdError);
     return false;
   }
 
-  tab_restore_service->RestoreEntryById(delegate, *params->id,
-      host_desktop_type, UNKNOWN);
+  std::vector<content::WebContents*> contents =
+      tab_restore_service->RestoreEntryById(delegate, id, host_desktop_type,
+      UNKNOWN);
+  if (!contents.size()) {
+    SetError(kReturnObjectError);

[not at google - send to devlin, 2013/08/06 19:04:09]

yeah so kReturnObject error - not a great error in general

[Kristen Dwan, 2013/08/12 15:11:30]

got rid of it.

+    return false;
+  }
+
+  if (is_window) {
+    WindowController* controller;
+    if (!GetWindowFromWindowID(this,
+                               ExtensionTabUtil::GetWindowIdOfTab(contents[0]),
+                               &controller))
+      return false;
+    SetResult(controller->CreateWindowValueWithTabs(GetExtension()));
+  } else {
+    SetResult(ExtensionTabUtil::CreateTabValue(contents[0], GetExtension()));
+  }
+
   return true;
 }
 
-SessionRestoreAPI::SessionRestoreAPI(Profile* profile) {
+bool SessionsRestoreFunction::RestoreForeignSession(
+    const std::string& session_tag, int id, Browser* browser) {

[not at google - send to devlin, 2013/08/06 19:04:09]

similar comments throughout here and the next function

[Kristen Dwan, 2013/08/12 15:11:30]

Done.

+  ProfileSyncService* service =
+      ProfileSyncServiceFactory::GetInstance()->GetForProfile(profile());
+  if (!(service && service->GetPreferredDataTypes().Has(syncer::SESSIONS))) {
+    SetError(kSessionsSyncNotEnabledError);

[Kristen Dwan, 2013/08/12 15:11:30]

Would you agree that these sync errors in restore should stay? I can understand
it not being an error in GetDevices but if you're trying to restore and
something goes wrong I'd assume that would be an error.. 

+    return false;
+  }
+  browser_sync::SessionModelAssociator* associator =
+      service->GetSessionModelAssociator();
+  if (!associator) {
+    SetError(kSessionSyncError);
+    return false;
+  }
+
+  const SessionTab* tab = NULL;
+  if (associator->GetForeignTab(session_tag, id, &tab)) {
+    TabStripModel* tab_strip = browser->tab_strip_model();
+    content::WebContents* contents = tab_strip->GetActiveWebContents();
+
+    content::WebContents* tab_content =
+        SessionRestore::RestoreForeignSessionTab(contents, *tab,
+            NEW_FOREGROUND_TAB);
+    SetResult(ExtensionTabUtil::CreateTabValue(tab_content, GetExtension()));
+    return true;
+  } else {

[not at google - send to devlin, 2013/08/06 19:04:09]

no else after return

[Kristen Dwan, 2013/08/12 15:11:30]

Done.

+    std::vector<const SessionWindow*> windows;
+    if (!associator->GetForeignSession(session_tag, &windows)) {
+      SetError(kSessionNotFoundError);
+      return false;
+    }
+
+    std::vector<const SessionWindow*>::const_iterator window = windows.begin();
+    while (window != windows.end() && (*window)->window_id.id() != id) {
+      ++window;
+    }
+    if (window == windows.end()) {
+      SetError(kSessionNotFoundError);
+      return false;
+    }
+
+    chrome::HostDesktopType host_desktop_type = browser->host_desktop_type();
+    // Only restore one window at a time.
+    std::vector<Browser*> browser =
+        SessionRestore::RestoreForeignSessionWindows(profile(),
+            host_desktop_type, window, (window + 1));
+    if (browser.size() != 1) {
+      SetError(kWindowRestoreError);
+      return false;
+    }
+
+    WindowController* controller;
+    if (!GetWindowFromWindowID(this,
+                               ExtensionTabUtil::GetWindowId(browser[0]),
+                               &controller))
+      return false;
+    SetResult(controller->CreateWindowValueWithTabs(GetExtension()));
+  }
+
+  return true;
 }
 
-SessionRestoreAPI::~SessionRestoreAPI() {
+bool SessionsRestoreFunction::RunImpl() {
+  scoped_ptr<Restore::Params> params(Restore::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+
+  Browser* browser =
+      chrome::FindBrowserWithProfile(profile(),
+                                     chrome::HOST_DESKTOP_TYPE_NATIVE);
+  if (!browser) {
+    SetError(kNoBrowserToRestoreSession);
+    return false;
+  }
+
+  if (!params->session_id)
+    return RestoreMostRecentlyClosed(browser);
+
+  if (IsLocalSession(*params->session_id)) {
+    int id;
+    if (!base::StringToInt(*params->session_id, &id)) {
+      SetError(kInvalidSessionIdError);
+      return false;
+    }
+    return RestoreLocalSession(id, browser);
+  } else {
+    std::string session_tag;
+    int id;
+    if (!SplitId(*params->session_id, &session_tag, &id)) {
+      SetError(kInvalidSessionIdError);
+      return false;
+    }
+    return RestoreForeignSession(session_tag, id, browser);
+  }
+
+  // Should never reach here.
+  return false;
 }
 
-static base::LazyInstance<ProfileKeyedAPIFactory<SessionRestoreAPI> >
+SessionsAPI::SessionsAPI(Profile* profile) {
+}
+
+SessionsAPI::~SessionsAPI() {
+}
+
+static base::LazyInstance<ProfileKeyedAPIFactory<SessionsAPI> >
     g_factory = LAZY_INSTANCE_INITIALIZER;
 
 // static
-ProfileKeyedAPIFactory<SessionRestoreAPI>*
-    SessionRestoreAPI::GetFactoryInstance() {
+ProfileKeyedAPIFactory<SessionsAPI>*
+    SessionsAPI::GetFactoryInstance() {
   return &g_factory.Get();
 }
 
 }  // namespace extensions