Add SessionWindow::WindowType APP and DEVTOOLS to match new Browser::Types

This CL changes SessionService to no longer restore DEVTOOLS on chromeos.
I suspect it was never intentional that DEVTOOLS was being included
to be restored.

Devtools restore is messy.  It doesn't connect to the previous
page, and will also create an extra tab in the NORMAL browser
with a devtools:// url which shows nothing.

See screenshots at crbug.com/990158#c6

Bug: 990158
Change-Id: I4861785da13ab9500418119f7fb76e40b9217312
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1753998Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688781}

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

@@ -281,6 +281,12 @@ SessionsGetDevicesFunction::CreateWindowModel(
     case sessions::SessionWindow::TYPE_POPUP:
       type = api::windows::WINDOW_TYPE_POPUP;
       break;
+    case sessions::SessionWindow::TYPE_APP:
+      type = api::windows::WINDOW_TYPE_APP;
+      break;
+    case sessions::SessionWindow::TYPE_DEVTOOLS:
+      type = api::windows::WINDOW_TYPE_DEVTOOLS;
+      break;
   }
 
   api::windows::WindowState state = api::windows::WINDOW_STATE_NONE;

chrome/browser/sessions/session_restore.cc

@@ -624,13 +624,22 @@ class SessionRestoreImpl : public BrowserListObserver {
                                  ui::WindowShowState show_state,
                                  const std::string& app_name) {
     Browser::CreateParams params(type, profile_, false);
-    if (!app_name.empty()) {
+    params.initial_bounds = bounds;
+
+    // Only browsers of TYPE_NORMAL, and Chrome OS TYPE_APP are saved or
+    // restored.  See SessionService::ShouldRestoreWindowOfType.
+#if defined(OS_CHROMEOS)
+    DCHECK(type == Browser::Type::TYPE_NORMAL ||
+           type == Browser::Type::TYPE_APP);
+    if (type == Browser::Type::TYPE_APP) {
       const bool trusted_source = true;  // We only store trusted app windows.
       params = Browser::CreateParams::CreateForApp(app_name, trusted_source,
                                                    bounds, profile_, false);
-    } else {
-      params.initial_bounds = bounds;
     }
+#else
+    DCHECK(type == Browser::Type::TYPE_NORMAL);
+#endif
+
     params.initial_show_state = show_state;
     params.initial_workspace = workspace;
     params.is_session_restore = true;

chrome/browser/sessions/session_restore_browsertest_chromeos.cc

@@ -139,6 +139,25 @@ IN_PROC_BROWSER_TEST_F(SessionRestoreTestChromeOS, RestoreAppsV1) {
   EXPECT_EQ(4u, total_count);  // Default browser() + 3 app windows
 }
 
+IN_PROC_BROWSER_TEST_F(SessionRestoreTestChromeOS, PRE_RestoreNoDevtools) {
+  // Create devtools.
+  CreateBrowserWithParams(Browser::CreateParams::CreateForDevTools(profile()));
+
+  TurnOnSessionRestore();
+}
+
+IN_PROC_BROWSER_TEST_F(SessionRestoreTestChromeOS, RestoreNoDevtools) {
+  size_t total_count = 0;
+  size_t devtools_count = 0;
+  for (auto* browser : *BrowserList::GetInstance()) {
+    ++total_count;
+    if (browser->is_type_devtools())
+      ++devtools_count;
+  }
+  EXPECT_EQ(1u, total_count);
+  EXPECT_EQ(0u, devtools_count);
+}
+
 IN_PROC_BROWSER_TEST_F(SessionRestoreTestChromeOS, PRE_RestoreMaximized) {
   // One browser window is always created by default.
   ASSERT_TRUE(browser());

chrome/browser/sessions/session_service.cc

@@ -266,8 +266,7 @@ void SessionService::WindowOpened(Browser* browser) {
     return;
 
   RestoreIfNecessary(std::vector<GURL>(), browser);
-  SetWindowType(browser->session_id(), browser->type(),
-                browser->deprecated_is_app() ? TYPE_APP : TYPE_NORMAL);
+  SetWindowType(browser->session_id(), browser->type());
   SetWindowAppName(browser->session_id(), browser->app_name());
 }
 
@@ -384,11 +383,10 @@ void SessionService::TabClosing(WebContents* contents) {
 }
 
 void SessionService::SetWindowType(const SessionID& window_id,
-                                   Browser::Type type,
-                                   AppType app_type) {
+                                   Browser::Type type) {
   sessions::SessionWindow::WindowType window_type =
       WindowTypeForBrowserType(type);
-  if (!ShouldRestoreWindowOfType(window_type, app_type))
+  if (!ShouldRestoreWindowOfType(window_type))
     return;
 
   windows_tracking_.insert(window_id);
@@ -583,15 +581,11 @@ void SessionService::Init() {
   BrowserList::AddObserver(this);
 }
 
-// TODO(crbug.com/990158): Remove AppType.  WindowType will be sufficient once
-// it matches Browser::Type with APP and DEVTOOLS.
 bool SessionService::ShouldRestoreWindowOfType(
-    sessions::SessionWindow::WindowType window_type,
-    AppType app_type) const {
+    sessions::SessionWindow::WindowType window_type) const {
 #if defined(OS_CHROMEOS)
   // Restore app popups for ChromeOS alone.
-  if (window_type == sessions::SessionWindow::TYPE_POPUP &&
-      app_type == TYPE_APP)
+  if (window_type == sessions::SessionWindow::TYPE_APP)
     return true;
 #endif
 
@@ -603,9 +597,7 @@ void SessionService::RemoveUnusedRestoreWindows(
   auto i = window_list->begin();
   while (i != window_list->end()) {
     sessions::SessionWindow* window = i->get();
-    if (!ShouldRestoreWindowOfType(window->type,
-                                   window->app_name.empty() ? TYPE_NORMAL :
-                                                              TYPE_APP)) {
+    if (!ShouldRestoreWindowOfType(window->type)) {
       i = window_list->erase(i);
     } else {
       ++i;
@@ -926,9 +918,7 @@ bool SessionService::ShouldTrackBrowser(Browser* browser) const {
   if (browser->deprecated_is_app() && !browser->is_trusted_source()) {
     return false;
   }
-  return ShouldRestoreWindowOfType(
-      WindowTypeForBrowserType(browser->type()),
-      browser->deprecated_is_app() ? TYPE_APP : TYPE_NORMAL);
+  return ShouldRestoreWindowOfType(WindowTypeForBrowserType(browser->type()));
 }
 
 void SessionService::MaybeDeleteSessionOnlyData() {

chrome/browser/sessions/session_service.h

@@ -68,12 +68,6 @@ class SessionService : public sessions::BaseSessionServiceDelegate,
                        public BrowserListObserver {
   friend class SessionServiceTestHelper;
  public:
-  // Used to distinguish an application from a ordinary content window.
-  enum AppType {
-    TYPE_APP,
-    TYPE_NORMAL
-  };
-
   // Creates a SessionService for the specified profile.
   explicit SessionService(Profile* profile);
   // For testing.
@@ -173,9 +167,7 @@ class SessionService : public sessions::BaseSessionServiceDelegate,
   // Sets the type of window. In order for the contents of a window to be
   // tracked SetWindowType must be invoked with a type we track
   // (ShouldRestoreOfWindowType returns true).
-  void SetWindowType(const SessionID& window_id,
-                     Browser::Type type,
-                     AppType app_type);
+  void SetWindowType(const SessionID& window_id, Browser::Type type);
 
   // Sets the application name of the specified window.
   void SetWindowAppName(const SessionID& window_id,
@@ -251,10 +243,10 @@ class SessionService : public sessions::BaseSessionServiceDelegate,
 
   void Init();
 
-  // Returns true if a window of given |window_type| and |app_type| should get
+  // Returns true if a window of given |window_type| should get
   // restored upon session restore.
-  bool ShouldRestoreWindowOfType(sessions::SessionWindow::WindowType type,
-                                 AppType app_type) const;
+  bool ShouldRestoreWindowOfType(
+      sessions::SessionWindow::WindowType type) const;
 
   // Removes unrestorable windows from the previous windows list.
   void RemoveUnusedRestoreWindows(

chrome/browser/sessions/session_service_unittest.cc

@@ -64,8 +64,7 @@ class SessionServiceTest : public BrowserWithTestWindowTest {
 
     helper_.SetService(session_service);
 
-    service()->SetWindowType(window_id, Browser::TYPE_NORMAL,
-                             SessionService::TYPE_NORMAL);
+    service()->SetWindowType(window_id, Browser::TYPE_NORMAL);
     service()->SetWindowBounds(window_id,
                                window_bounds,
                                ui::SHOW_STATE_NORMAL);
@@ -162,8 +161,7 @@ class SessionServiceTest : public BrowserWithTestWindowTest {
     UpdateNavigation(window_id, tab1_id, *nav1, true);
 
     const gfx::Rect window2_bounds(3, 4, 5, 6);
-    service()->SetWindowType(window2_id, Browser::TYPE_NORMAL,
-                             SessionService::TYPE_NORMAL);
+    service()->SetWindowType(window2_id, Browser::TYPE_NORMAL);
     service()->SetWindowBounds(window2_id,
                                window2_bounds,
                                ui::SHOW_STATE_MAXIMIZED);
@@ -371,8 +369,7 @@ TEST_F(SessionServiceTest, WindowWithNoTabsGetsPruned) {
   UpdateNavigation(window_id, tab1_id, nav1, true);
 
   const gfx::Rect window2_bounds(3, 4, 5, 6);
-  service()->SetWindowType(window2_id, Browser::TYPE_NORMAL,
-                           SessionService::TYPE_NORMAL);
+  service()->SetWindowType(window2_id, Browser::TYPE_NORMAL);
   service()->SetWindowBounds(window2_id,
                              window2_bounds,
                              ui::SHOW_STATE_NORMAL);
@@ -465,8 +462,7 @@ TEST_F(SessionServiceTest, WindowCloseCommittedAfterNavigate) {
   SessionID tab2_id = SessionID::NewUnique();
   ASSERT_NE(window2_id, window_id);
 
-  service()->SetWindowType(window2_id, Browser::TYPE_NORMAL,
-                           SessionService::TYPE_NORMAL);
+  service()->SetWindowType(window2_id, Browser::TYPE_NORMAL);
   service()->SetWindowBounds(window2_id,
                              window_bounds,
                              ui::SHOW_STATE_NORMAL);
@@ -508,9 +504,7 @@ TEST_F(SessionServiceTest, IgnorePopups) {
   SessionID tab2_id = SessionID::NewUnique();
   ASSERT_NE(window2_id, window_id);
 
-  service()->SetWindowType(window2_id,
-                           Browser::TYPE_POPUP,
-                           SessionService::TYPE_NORMAL);
+  service()->SetWindowType(window2_id, Browser::TYPE_POPUP);
   service()->SetWindowBounds(window2_id,
                              window_bounds,
                              ui::SHOW_STATE_NORMAL);
@@ -562,8 +556,7 @@ TEST_F(SessionServiceTest, RestoreApp) {
   SessionID tab2_id = SessionID::NewUnique();
   ASSERT_NE(window2_id, window_id);
 
-  service()->SetWindowType(window2_id, Browser::TYPE_APP,
-                           SessionService::TYPE_APP);
+  service()->SetWindowType(window2_id, Browser::TYPE_APP);
   service()->SetWindowBounds(window2_id,
                              window_bounds,
                              ui::SHOW_STATE_NORMAL);
@@ -600,7 +593,7 @@ TEST_F(SessionServiceTest, RestoreApp) {
   ASSERT_EQ(0, windows[app_index]->selected_tab_index);
   ASSERT_EQ(window2_id, windows[app_index]->window_id);
   ASSERT_EQ(1U, windows[app_index]->tabs.size());
-  ASSERT_TRUE(windows[app_index]->type == sessions::SessionWindow::TYPE_POPUP);
+  ASSERT_TRUE(windows[app_index]->type == sessions::SessionWindow::TYPE_APP);
   ASSERT_EQ("TestApp", windows[app_index]->app_name);
 
   tab = windows[app_index]->tabs[0].get();
@@ -613,8 +606,7 @@ TEST_F(SessionServiceTest, IgnoreCrostiniApps) {
   SessionID window2_id = SessionID::NewUnique();
   ASSERT_NE(window2_id, window_id);
 
-  service()->SetWindowType(window2_id, Browser::TYPE_APP,
-                           SessionService::TYPE_NORMAL);
+  service()->SetWindowType(window2_id, Browser::TYPE_APP);
   service()->SetWindowBounds(window2_id, window_bounds, ui::SHOW_STATE_NORMAL);
   service()->SetWindowAppName(window2_id, "_crostini_fakeappid");
 

chrome/browser/sessions/session_service_utils.cc

@@ -4,16 +4,17 @@
 
 #include "chrome/browser/sessions/session_service_utils.h"
 
-// TODO(crbug.com/990158): Add mappings for Browser::Type APP and DEVTOOLS.
 sessions::SessionWindow::WindowType WindowTypeForBrowserType(
     Browser::Type type) {
   switch (type) {
     case Browser::TYPE_NORMAL:
       return sessions::SessionWindow::TYPE_NORMAL;
     case Browser::TYPE_POPUP:
+      return sessions::SessionWindow::TYPE_POPUP;
     case Browser::TYPE_APP:
+      return sessions::SessionWindow::TYPE_APP;
     case Browser::TYPE_DEVTOOLS:
-      return sessions::SessionWindow::TYPE_POPUP;
+      return sessions::SessionWindow::TYPE_DEVTOOLS;
   }
   NOTREACHED();
   return sessions::SessionWindow::TYPE_NORMAL;
@@ -26,6 +27,10 @@ Browser::Type BrowserTypeForWindowType(
       return Browser::TYPE_NORMAL;
     case sessions::SessionWindow::TYPE_POPUP:
       return Browser::TYPE_POPUP;
+    case sessions::SessionWindow::TYPE_APP:
+      return Browser::TYPE_APP;
+    case sessions::SessionWindow::TYPE_DEVTOOLS:
+      return Browser::TYPE_DEVTOOLS;
   }
   NOTREACHED();
   return Browser::TYPE_NORMAL;

chrome/browser/sessions/tab_restore_service_unittest.cc

@@ -148,8 +148,7 @@ class TabRestoreServiceImplTest : public ChromeRenderViewHostTestHarness {
 
     SessionService* session_service =
         SessionServiceFactory::GetForProfile(profile());
-    session_service->SetWindowType(window_id(), Browser::TYPE_NORMAL,
-                                   SessionService::TYPE_NORMAL);
+    session_service->SetWindowType(window_id(), Browser::TYPE_NORMAL);
     session_service->SetTabWindow(window_id(), tab_id());
     session_service->SetTabIndexInWindow(window_id(), tab_id(), 0);
     session_service->SetSelectedTabInWindow(window_id(), 0);

chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc

@@ -285,8 +285,7 @@ TEST_F(RecentTabsSubMenuModelTest,
                                            base::WrapUnique(session_service));
   SessionID tab_id = SessionID::FromSerializedValue(1);
   SessionID window_id = SessionID::FromSerializedValue(2);
-  session_service->SetWindowType(window_id, Browser::TYPE_NORMAL,
-                                 SessionService::TYPE_NORMAL);
+  session_service->SetWindowType(window_id, Browser::TYPE_NORMAL);
   session_service->SetTabWindow(window_id, tab_id);
   session_service->SetTabIndexInWindow(window_id, tab_id, 0);
   session_service->SetSelectedTabInWindow(window_id, 0);

components/sessions/core/session_types.h

@@ -131,9 +131,12 @@ struct SESSIONS_EXPORT SessionWindow {
 
   // Possible window types which can be stored here. Note that these values will
   // be written out to disc via session commands.
-  // TODO(crbug.com/990158): Add types for TYPE_APP and TYPE_DEVTOOLS to match
-  // updated Browser::Type.
-  enum WindowType { TYPE_NORMAL = 0, TYPE_POPUP = 1 };
+  enum WindowType {
+    TYPE_NORMAL = 0,
+    TYPE_POPUP = 1,
+    TYPE_APP = 2,
+    TYPE_DEVTOOLS = 3
+  };
 
   // Identifier of the window.
   SessionID window_id;