[ios] Change SessionServiceIOS to always require a session id

To simplify the API of SessionServiceIOS in preparation of next
refactoring, change it to always require a session identifier.
Passing an empty string or nil will result in the same code path
as calling the methods that took no session identifier.

Bug: 1165798
Change-Id: I9739baf91cbcfcd21be61cd487c24b0d375289bb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2632760
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Stepan Khapugin <stkhapugin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845664}

ios/chrome/browser/sessions/session_restoration_browser_agent.mm

@@ -173,14 +173,17 @@ bool SessionRestorationBrowserAgent::RestoreSessionWindow(
 
 bool SessionRestorationBrowserAgent::RestoreSession() {
   PreviousSessionInfo* session_info = [PreviousSessionInfo sharedInstance];
-  BOOL is_previous_session_multi_window =
-      session_info.isMultiWindowEnabledSession;
-  BOOL force_single_window =
-      IsMultiwindowSupported() && !is_previous_session_multi_window;
-  NSString* path = base::SysUTF8ToNSString(
-      GetSessionStoragePath(force_single_window).AsUTF8Unsafe());
   auto scoped_restore = [session_info startSessionRestoration];
-  SessionIOS* session = [session_service_ loadSessionFromDirectory:path];
+
+  const base::FilePath& path = browser_state_->GetStatePath();
+  NSString* session_id =
+      (IsMultiwindowSupported() && session_info.isMultiWindowEnabledSession)
+          ? base::SysUTF8ToNSString(session_identifier_)
+          : nil;
+
+  SessionIOS* session = [session_service_
+      loadSessionWithSessionID:session_id
+                     directory:base::SysUTF8ToNSString(path.AsUTF8Unsafe())];
   SessionWindowIOS* session_window = nil;
 
   if (session) {
@@ -199,10 +202,10 @@ void SessionRestorationBrowserAgent::SaveSession(bool immediately) {
   if (!CanSaveSession())
     return;
 
-  NSString* path = base::SysUTF8ToNSString(
-      GetSessionStoragePath(/*force_single_window=*/false).AsUTF8Unsafe());
+  const base::FilePath& path = browser_state_->GetStatePath();
   [session_service_ saveSession:session_ios_factory_
-                      directory:path
+                      sessionID:base::SysUTF8ToNSString(session_identifier_)
+                      directory:base::SysUTF8ToNSString(path.AsUTF8Unsafe())
                     immediately:immediately];
 }
 

ios/chrome/browser/sessions/session_restoration_browser_agent_unittest.mm

@@ -226,7 +226,7 @@ TEST_F(SessionRestorationBrowserAgentTest, SaveAndRestoreEmptySession) {
   NSString* state_path = base::SysUTF8ToNSString(
       chrome_browser_state_->GetStatePath().AsUTF8Unsafe());
   SessionIOS* session =
-      [test_session_service_ loadSessionFromDirectory:state_path];
+      [test_session_service_ loadSessionWithSessionID:nil directory:state_path];
   ASSERT_EQ(1u, session.sessionWindows.count);
   SessionWindowIOS* session_window = session.sessionWindows[0];
   session_restoration_agent_->RestoreSessionWindow(session_window);
@@ -256,7 +256,7 @@ TEST_F(SessionRestorationBrowserAgentTest, SaveAndRestoreSession) {
   NSString* state_path = base::SysUTF8ToNSString(
       chrome_browser_state_->GetStatePath().AsUTF8Unsafe());
   SessionIOS* session =
-      [test_session_service_ loadSessionFromDirectory:state_path];
+      [test_session_service_ loadSessionWithSessionID:nil directory:state_path];
   ASSERT_EQ(1u, session.sessionWindows.count);
   SessionWindowIOS* session_window = session.sessionWindows[0];
 

ios/chrome/browser/sessions/session_service_ios.h

@@ -13,8 +13,8 @@
 @class SessionIOS;
 @class SessionIOSFactory;
 
-// A singleton service for saving the current session. Can either save on a
-// delay or immediately. Saving is always performed on a separate thread.
+// A singleton service for saving the sessions (list of tabs). Can either save
+// on a delay or immediately. Saving is always performed on a separate thread.
 @interface SessionServiceIOS : NSObject
 
 // Lazily creates a singleton instance with a default task runner.
@@ -26,18 +26,22 @@
     (const scoped_refptr<base::SequencedTaskRunner>&)taskRunner
     NS_DESIGNATED_INITIALIZER;
 
-// Saves the session returned by |factory| to |directory|. If |immediately|
-// is NO, the save is done after a delay. If another call is pending, this one
-// is ignored. If YES, the save is done now, cancelling any pending calls.
-// Either way, the save is done on a separate thread to avoid blocking the UI
-// thread.
+// Saves the session (list of tabs) returned by |factory|. The save location
+// is derived from the scene identifier |sessionID| and the ChromeBrowserState
+// |directory|. If |immediately| is NO, the save is done after a fixed delay,
+// or ignored if another delayed save for the same location is still pending.
+// If |immediately| is YES, then the save is done immediately and any pending
+// save is cancelled. Either way, the save is done on a separate thread to
+// avoid blocking the UI thread.
 - (void)saveSession:(__weak SessionIOSFactory*)factory
+          sessionID:(NSString*)sessionID
           directory:(NSString*)directory
         immediately:(BOOL)immediately;
 
-// Loads the session from default session file in |directory| on the main
-// thread. Returns nil in case of errors.
-- (SessionIOS*)loadSessionFromDirectory:(NSString*)directory;
+// Loads a session (list of tabs) from the save location derived from the scene
+// identifier |sessionID| and the ChromeBrowserState |directory|.
+- (SessionIOS*)loadSessionWithSessionID:(NSString*)sessionID
+                              directory:(NSString*)directory;
 
 // Loads the session from |sessionPath| on the main thread. Returns nil in case
 // of errors.
@@ -58,9 +62,6 @@
 + (NSString*)sessionPathForSessionID:(NSString*)sessionID
                            directory:(NSString*)directory;
 
-// Returns the path of the session file for |directory|.
-+ (NSString*)sessionPathForDirectory:(NSString*)directory;
-
 @end
 
 @interface SessionServiceIOS (SubClassing)

ios/chrome/browser/sessions/session_service_ios.mm

@@ -103,9 +103,11 @@ NSString* const kSessionFileName =
 }
 
 - (void)saveSession:(__weak SessionIOSFactory*)factory
+          sessionID:(NSString*)sessionID
           directory:(NSString*)directory
         immediately:(BOOL)immediately {
-  NSString* sessionPath = [[self class] sessionPathForDirectory:directory];
+  NSString* sessionPath = [[self class] sessionPathForSessionID:sessionID
+                                                      directory:directory];
   BOOL hadPendingSession = [_pendingSessions objectForKey:sessionPath] != nil;
   [_pendingSessions setObject:factory forKey:sessionPath];
   if (immediately) {
@@ -120,8 +122,10 @@ NSString* const kSessionFileName =
   }
 }
 
-- (SessionIOS*)loadSessionFromDirectory:(NSString*)directory {
-  NSString* sessionPath = [[self class] sessionPathForDirectory:directory];
+- (SessionIOS*)loadSessionWithSessionID:(NSString*)sessionID
+                              directory:(NSString*)directory {
+  NSString* sessionPath = [[self class] sessionPathForSessionID:sessionID
+                                                      directory:directory];
   base::TimeTicks start_time = base::TimeTicks::Now();
   SessionIOS* session = [self loadSessionFromPath:sessionPath];
   UmaHistogramTimes("Session.WebStates.ReadFromFileTime",
@@ -189,9 +193,11 @@ NSString* const kSessionFileName =
 
   // If there were no session ids, then scenes are not supported fall back to
   // the original location
-  if (sessionFilesPaths.count == 0)
+  if (sessionFilesPaths.count == 0) {
     [sessionFilesPaths
-        addObject:[[self class] sessionPathForDirectory:directory]];
+        addObject:[[self class] sessionPathForSessionID:nil
+                                              directory:directory]];
+  }
 
   [self deletePaths:sessionFilesPaths completion:std::move(callback)];
 }
@@ -209,16 +215,18 @@ NSString* const kSessionFileName =
   [self deletePaths:paths completion:base::DoNothing()];
 }
 
-+ (NSString*)sessionPathForDirectory:(NSString*)directory {
-  return [directory stringByAppendingPathComponent:kSessionFileName];
-}
-
 + (NSString*)sessionPathForSessionID:(NSString*)sessionID
                            directory:(NSString*)directory {
+  // TODO(crbug.com/1165798): remove when the sessionID is guaranteed to
+  // always be an non-empty string.
   if (!sessionID.length)
-    return [[self class] sessionPathForDirectory:directory];
+    return [directory stringByAppendingPathComponent:kSessionFileName];
+
   return [NSString pathWithComponents:@[
-    directory, kSessionDirectory, sessionID, kSessionFileName
+    directory,
+    kSessionDirectory,
+    sessionID,
+    kSessionFileName,
   ]];
 }
 

ios/chrome/browser/sessions/session_service_ios_unittest.mm

@@ -106,15 +106,27 @@ class SessionServiceTest : public PlatformTest {
 };
 
 TEST_F(SessionServiceTest, SessionPathForDirectory) {
-  EXPECT_NSEQ(@"session.plist",
-              [SessionServiceIOS sessionPathForDirectory:@""]);
+  EXPECT_NSEQ(@"root/session.plist",
+              [SessionServiceIOS sessionPathForSessionID:nil
+                                               directory:@"root"]);
+
+  EXPECT_NSEQ(@"root/session.plist",
+              [SessionServiceIOS sessionPathForSessionID:@""
+                                               directory:@"root"]);
+
+  EXPECT_NSEQ(@"root/Sessions/session-id/session.plist",
+              [SessionServiceIOS sessionPathForSessionID:@"session-id"
+                                               directory:@"root"]);
 }
 
 TEST_F(SessionServiceTest, SaveSessionWindowToPath) {
   std::unique_ptr<WebStateList> web_state_list = CreateWebStateList(0);
   SessionIOSFactory* factory =
       [[SessionIOSFactory alloc] initWithWebStateList:web_state_list.get()];
-  [session_service() saveSession:factory directory:directory() immediately:YES];
+  [session_service() saveSession:factory
+                       sessionID:nil
+                       directory:directory()
+                     immediately:YES];
 
   // Even if |immediately| is YES, the file is created by a task on the task
   // runner passed to SessionServiceIOS initializer (which is the current
@@ -134,7 +146,10 @@ TEST_F(SessionServiceTest, SaveSessionWindowToPathDirectoryExists) {
   SessionIOSFactory* factory =
       [[SessionIOSFactory alloc] initWithWebStateList:web_state_list.get()];
 
-  [session_service() saveSession:factory directory:directory() immediately:YES];
+  [session_service() saveSession:factory
+                       sessionID:nil
+                       directory:directory()
+                     immediately:YES];
 
   // Even if |immediately| is YES, the file is created by a task on the task
   // runner passed to SessionServiceIOS initializer (which is the current
@@ -147,7 +162,7 @@ TEST_F(SessionServiceTest, SaveSessionWindowToPathDirectoryExists) {
 
 TEST_F(SessionServiceTest, LoadSessionFromDirectoryNoFile) {
   SessionIOS* session =
-      [session_service() loadSessionFromDirectory:directory()];
+      [session_service() loadSessionWithSessionID:nil directory:directory()];
   EXPECT_TRUE(session == nil);
 }
 
@@ -158,7 +173,10 @@ TEST_F(SessionServiceTest, SaveExpiredSession) {
   SessionIOSFactory* factory =
       [[SessionIOSFactory alloc] initWithWebStateList:web_state_list.get()];
 
-  [session_service() saveSession:factory directory:directory() immediately:NO];
+  [session_service() saveSession:factory
+                       sessionID:nil
+                       directory:directory()
+                     immediately:NO];
   [factory disconnect];
   factory = nil;
   // Make sure that the delay for saving a session has passed (at least 2.5
@@ -167,7 +185,7 @@ TEST_F(SessionServiceTest, SaveExpiredSession) {
   base::RunLoop().RunUntilIdle();
 
   SessionIOS* session =
-      [session_service() loadSessionFromDirectory:directory()];
+      [session_service() loadSessionWithSessionID:nil directory:directory()];
   EXPECT_FALSE(session);
 }
 
@@ -176,7 +194,10 @@ TEST_F(SessionServiceTest, LoadSessionFromDirectory) {
   SessionIOSFactory* factory =
       [[SessionIOSFactory alloc] initWithWebStateList:web_state_list.get()];
 
-  [session_service() saveSession:factory directory:directory() immediately:YES];
+  [session_service() saveSession:factory
+                       sessionID:nil
+                       directory:directory()
+                     immediately:YES];
 
   // Even if |immediately| is YES, the file is created by a task on the task
   // runner passed to SessionServiceIOS initializer (which is the current
@@ -184,7 +205,7 @@ TEST_F(SessionServiceTest, LoadSessionFromDirectory) {
   base::RunLoop().RunUntilIdle();
 
   SessionIOS* session =
-      [session_service() loadSessionFromDirectory:directory()];
+      [session_service() loadSessionWithSessionID:nil directory:directory()];
   EXPECT_EQ(1u, session.sessionWindows.count);
   EXPECT_EQ(2u, session.sessionWindows[0].sessions.count);
   EXPECT_EQ(0u, session.sessionWindows[0].selectedIndex);
@@ -195,7 +216,10 @@ TEST_F(SessionServiceTest, LoadSessionFromPath) {
   SessionIOSFactory* factory =
       [[SessionIOSFactory alloc] initWithWebStateList:web_state_list.get()];
 
-  [session_service() saveSession:factory directory:directory() immediately:YES];
+  [session_service() saveSession:factory
+                       sessionID:nil
+                       directory:directory()
+                     immediately:YES];
 
   // Even if |immediately| is YES, the file is created by a task on the task
   // runner passed to SessionServiceIOS initializer (which is the current
@@ -203,7 +227,7 @@ TEST_F(SessionServiceTest, LoadSessionFromPath) {
   base::RunLoop().RunUntilIdle();
 
   NSString* session_path =
-      [SessionServiceIOS sessionPathForDirectory:directory()];
+      [SessionServiceIOS sessionPathForSessionID:nil directory:directory()];
   NSString* renamed_path = [session_path stringByAppendingPathExtension:@"bak"];
   ASSERT_NSNE(session_path, renamed_path);
 

ios/chrome/browser/sessions/test_session_service.mm

@@ -19,9 +19,11 @@
 }
 
 - (void)saveSession:(__weak SessionIOSFactory*)factory
+          sessionID:(NSString*)sessionID
           directory:(NSString*)directory
         immediately:(BOOL)immediately {
-  NSString* sessionPath = [[self class] sessionPathForDirectory:directory];
+  NSString* sessionPath = [[self class] sessionPathForSessionID:sessionID
+                                                      directory:directory];
   NSData* data =
       [NSKeyedArchiver archivedDataWithRootObject:[factory sessionForSaving]
                             requiringSecureCoding:NO

ios/chrome/browser/test/perf_test_with_bvc_ios.mm

@@ -99,7 +99,8 @@ void PerfTestWithBVC::SetUp() {
   NSString* state_path = base::SysUTF8ToNSString(
       chrome_browser_state_->GetStatePath().AsUTF8Unsafe());
   SessionIOS* session =
-      [[SessionServiceIOS sharedService] loadSessionFromDirectory:state_path];
+      [[SessionServiceIOS sharedService] loadSessionWithSessionID:nil
+                                                        directory:state_path];
   DCHECK_EQ(session.sessionWindows.count, 1u);
 
   browser_ = std::make_unique<TestBrowser>(chrome_browser_state_.get(),