[ios] Use base::FilePath for SessionServiceIOS/CrashRestoreHelper APIs

Requiring the BrowserState's path to be passed as a NSString* forces
all callers to convert the base::FilePath to NSString* making the API
harder to use (as the conversion is non trivial).

Instead change the API of both classes to use base::FilePath directly
and to do the conversion internally where it can be centralized in a
few location makes the API easier to use.

Bug: 1165798
Change-Id: Ida8294210f83875fcbeffa88a3e7f5ae78a2316f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2635668
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#846070}

ios/chrome/browser/browsing_data/browsing_data_remover_impl.mm

@@ -276,10 +276,11 @@ void BrowsingDataRemoverImpl::RemoveImpl(base::Time delete_begin,
 
   if (IsRemoveDataMaskSet(mask, BrowsingDataRemoveMask::REMOVE_HISTORY)) {
     if (session_service_) {
-      NSString* state_path = base::SysUTF8ToNSString(
-          browser_state_->GetStatePath().AsUTF8Unsafe());
-      [session_service_ deleteAllSessionFilesInBrowserStateDirectory:state_path
-          completion:CreatePendingTaskCompletionClosure()];
+      const base::FilePath& state_path = browser_state_->GetStatePath();
+      [session_service_
+          deleteAllSessionFilesInDirectory:state_path
+                                completion:
+                                    CreatePendingTaskCompletionClosure()];
     }
 
     // Remove the screenshots taken by the system when backgrounding the
@@ -558,10 +559,9 @@ void BrowsingDataRemoverImpl::RemoveImpl(base::Time delete_begin,
 void BrowsingDataRemoverImpl::RemoveSessionsData(
     NSArray<NSString*>* session_ids) {
   [[SessionServiceIOS sharedService]
-                 deleteSessions:session_ids
-      fromBrowserStateDirectory:base::SysUTF8ToNSString(
-                                    browser_state_->GetStatePath()
-                                        .AsUTF8Unsafe())];
+      deleteSessions:session_ids
+           directory:browser_state_->GetStatePath()
+          completion:base::DoNothing()];
 }
 
 // TODO(crbug.com/619783): removing data from WkWebsiteDataStore should be

ios/chrome/browser/crash_report/crash_restore_helper.h

@@ -7,6 +7,8 @@
 
 #import <Foundation/Foundation.h>
 
+#include "base/files/file_path.h"
+
 class Browser;
 class ChromeBrowserState;
 
@@ -40,7 +42,7 @@ class ChromeBrowserState;
 
 // Returns the path for back of session |sessionID| relative in |directory|.
 + (NSString*)backupPathForSessionID:(NSString*)sessionID
-                          directory:(NSString*)directory;
+                          directory:(const base::FilePath&)directory;
 
 @end
 

ios/chrome/browser/crash_report/crash_restore_helper.mm

@@ -71,12 +71,19 @@
 
 namespace {
 
-NSString* const kSessionBackupDirectory =
-    @"Backups";  // The name for directory which contains all session backup
-                 // subdirectories for multiple sessions.
-
-NSString* const kSessionBackupFileName =
-    @"session.backup.plist";  // The session file name on disk.
+// The name for directory which contains all session backup subdirectories for
+// multiple sessions.
+const base::FilePath::CharType kSessionBackupDirectory[] =
+    FILE_PATH_LITERAL("Backups");
+
+// The session file name on disk.
+const base::FilePath::CharType kSessionBackupFileName[] =
+    FILE_PATH_LITERAL("session.backup.plist");
+
+// Convert |path| to NSString.
+NSString* PathAsNSString(const base::FilePath& path) {
+  return base::SysUTF8ToNSString(path.AsUTF8Unsafe());
+}
 
 class InfoBarManagerObserverBridge : infobars::InfoBarManager::Observer {
  public:
@@ -261,8 +268,7 @@ int SessionCrashedInfoBarDelegate::GetIconId() const {
        forBrowserState:(ChromeBrowserState*)browserState
           shouldBackup:(BOOL)shouldBackup {
   BOOL partialSuccess = NO;
-  NSString* stashPath =
-      base::SysUTF8ToNSString(browserState->GetStatePath().value());
+  const base::FilePath& stashPath = browserState->GetStatePath();
 
   for (NSString* sessionID in sessionIDs) {
     NSString* sessionPath =
@@ -325,27 +331,25 @@ int SessionCrashedInfoBarDelegate::GetIconId() const {
 }
 
 + (NSString*)backupPathForSessionID:(NSString*)sessionID
-                          directory:(NSString*)directory {
+                          directory:(const base::FilePath&)directory {
+  // TODO(crbug.com/1165798): remove when the sessionID is guaranteed to
+  // always be an non-empty string.
   if (!sessionID.length)
-    return [directory stringByAppendingPathComponent:kSessionBackupFileName];
-
-  return [NSString pathWithComponents:@[
-    directory,
-    kSessionBackupDirectory,
-    sessionID,
-    kSessionBackupFileName,
-  ]];
+    return PathAsNSString(directory.Append(kSessionBackupFileName));
+
+  return PathAsNSString(directory.Append(kSessionBackupDirectory)
+                            .Append(base::SysNSStringToUTF8(sessionID))
+                            .Append(kSessionBackupFileName));
 }
 
 + (NSArray<NSString*>*)backedupSessionIDsForBrowserState:
     (ChromeBrowserState*)browserState {
   if (!IsMultiwindowSupported())
     return @[ @"" ];
-  NSString* stashPath =
-      base::SysUTF8ToNSString(browserState->GetStatePath().AsUTF8Unsafe());
+  const base::FilePath backupDirectory =
+      browserState->GetStatePath().Append(kSessionBackupDirectory);
   return [[NSFileManager defaultManager]
-      contentsOfDirectoryAtPath:
-          [stashPath stringByAppendingPathComponent:kSessionBackupDirectory]
+      contentsOfDirectoryAtPath:PathAsNSString(backupDirectory)
                           error:nil];
 }
 
@@ -389,8 +393,7 @@ int SessionCrashedInfoBarDelegate::GetIconId() const {
 
 + (BOOL)restoreSessionsAfterCrashForBrowserState:
     (ChromeBrowserState*)browserState {
-  NSString* stashPath =
-      base::SysUTF8ToNSString(browserState->GetStatePath().AsUTF8Unsafe());
+  const base::FilePath& stashPath = browserState->GetStatePath();
 
   BrowserList* browserList =
       BrowserListFactory::GetForBrowserState(browserState);
@@ -472,8 +475,7 @@ int SessionCrashedInfoBarDelegate::GetIconId() const {
   _sessionRestored = YES;
 
   ChromeBrowserState* browserState = _browser->GetBrowserState();
-  NSString* stashPath =
-      base::SysUTF8ToNSString(browserState->GetStatePath().AsUTF8Unsafe());
+  const base::FilePath& stashPath = browserState->GetStatePath();
 
   NSArray<NSString*>* sessionsIDs =
       [CrashRestoreHelper backedupSessionIDsForBrowserState:browserState];

ios/chrome/browser/crash_report/crash_restore_helper_unittest.mm

@@ -53,8 +53,7 @@ class CrashRestoreHelperTest : public PlatformTest {
     };
     NSData* data = [NSData dataWithBytes:"hello" length:5];
     for (size_t index = 0; index < base::size(browser_states); ++index) {
-      NSString* state_path = base::SysUTF8ToNSString(
-          browser_states[index]->GetStatePath().value());
+      const base::FilePath& state_path = browser_states[index]->GetStatePath();
       NSString* backup_path =
           [CrashRestoreHelper backupPathForSessionID:session_id
                                            directory:state_path];
@@ -86,8 +85,7 @@ class CrashRestoreHelperTest : public PlatformTest {
     };
 
     for (size_t index = 0; index < base::size(browser_states); ++index) {
-      NSString* state_path = base::SysUTF8ToNSString(
-          browser_states[index]->GetStatePath().value());
+      const base::FilePath& state_path = browser_states[index]->GetStatePath();
       NSString* session_path =
           [SessionServiceIOS sessionPathForSessionID:session_id
                                            directory:state_path];
@@ -105,9 +103,7 @@ class CrashRestoreHelperTest : public PlatformTest {
     NSFileManager* file_manager = [NSFileManager defaultManager];
     NSString* backup_path = [CrashRestoreHelper
         backupPathForSessionID:session_id
-                     directory:base::SysUTF8ToNSString(
-                                   browser_state->GetStatePath()
-                                       .AsUTF8Unsafe())];
+                     directory:browser_state->GetStatePath()];
     if (![file_manager fileExistsAtPath:backup_path])
       return false;
     [file_manager removeItemAtPath:backup_path error:nil];

ios/chrome/browser/sessions/session_restoration_browser_agent.mm

@@ -176,7 +176,6 @@ bool SessionRestorationBrowserAgent::RestoreSession() {
   PreviousSessionInfo* session_info = [PreviousSessionInfo sharedInstance];
   auto scoped_restore = [session_info startSessionRestoration];
 
-  const base::FilePath& path = browser_state_->GetStatePath();
   NSString* session_id =
       (IsMultiwindowSupported() && session_info.isMultiWindowEnabledSession)
           ? base::SysUTF8ToNSString(session_identifier_)
@@ -184,7 +183,7 @@ bool SessionRestorationBrowserAgent::RestoreSession() {
 
   SessionIOS* session = [session_service_
       loadSessionWithSessionID:session_id
-                     directory:base::SysUTF8ToNSString(path.AsUTF8Unsafe())];
+                     directory:browser_state_->GetStatePath()];
   SessionWindowIOS* session_window = nil;
 
   if (session) {
@@ -203,10 +202,9 @@ void SessionRestorationBrowserAgent::SaveSession(bool immediately) {
   if (!CanSaveSession())
     return;
 
-  const base::FilePath& path = browser_state_->GetStatePath();
   [session_service_ saveSession:session_ios_factory_
                       sessionID:base::SysUTF8ToNSString(session_identifier_)
-                      directory:base::SysUTF8ToNSString(path.AsUTF8Unsafe())
+                      directory:browser_state_->GetStatePath()
                     immediately:immediately];
 }
 

ios/chrome/browser/sessions/session_restoration_browser_agent_unittest.mm

@@ -223,8 +223,7 @@ TEST_F(SessionRestorationBrowserAgentTest, SaveAndRestoreEmptySession) {
   [test_session_service_ setPerformIO:NO];
 
   // Restore, expect that there are no sessions.
-  NSString* state_path = base::SysUTF8ToNSString(
-      chrome_browser_state_->GetStatePath().AsUTF8Unsafe());
+  const base::FilePath& state_path = chrome_browser_state_->GetStatePath();
   SessionIOS* session =
       [test_session_service_ loadSessionWithSessionID:nil directory:state_path];
   ASSERT_EQ(1u, session.sessionWindows.count);
@@ -253,8 +252,7 @@ TEST_F(SessionRestorationBrowserAgentTest, SaveAndRestoreSession) {
   // close all the webStates
   web_state_list_->CloseAllWebStates(WebStateList::CLOSE_NO_FLAGS);
 
-  NSString* state_path = base::SysUTF8ToNSString(
-      chrome_browser_state_->GetStatePath().AsUTF8Unsafe());
+  const base::FilePath& state_path = chrome_browser_state_->GetStatePath();
   SessionIOS* session =
       [test_session_service_ loadSessionWithSessionID:nil directory:state_path];
   ASSERT_EQ(1u, session.sessionWindows.count);

ios/chrome/browser/sessions/session_service_ios.h

@@ -8,6 +8,7 @@
 #import <Foundation/Foundation.h>
 
 #include "base/callback.h"
+#include "base/files/file_path.h"
 #include "base/sequenced_task_runner.h"
 
 @class SessionIOS;
@@ -35,32 +36,31 @@
 // avoid blocking the UI thread.
 - (void)saveSession:(__weak SessionIOSFactory*)factory
           sessionID:(NSString*)sessionID
-          directory:(NSString*)directory
+          directory:(const base::FilePath&)directory
         immediately:(BOOL)immediately;
 
 // 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;
+                              directory:(const base::FilePath&)directory;
 
 // Loads the session from |sessionPath| on the main thread. Returns nil in case
 // of errors.
 - (SessionIOS*)loadSessionFromPath:(NSString*)sessionPath;
 
-// Schedules deletion of the all session files from a specific browser state
-// |directory|.
-- (void)deleteAllSessionFilesInBrowserStateDirectory:(NSString*)directory
-                                          completion:
-                                              (base::OnceClosure)callback;
+// Schedules deletion of the all session files from a specific |directory|.
+- (void)deleteAllSessionFilesInDirectory:(const base::FilePath&)directory
+                              completion:(base::OnceClosure)callback;
 
 // Schedule deletion of session directories with |sessionIDs| which resides in
 // a specific browser state |directory|.
 - (void)deleteSessions:(NSArray<NSString*>*)sessionIDs
-    fromBrowserStateDirectory:(NSString*)directory;
+             directory:(const base::FilePath&)directory
+            completion:(base::OnceClosure)callback;
 
 // Returns the path of the session with |sessionID| within a |directory|.
 + (NSString*)sessionPathForSessionID:(NSString*)sessionID
-                           directory:(NSString*)directory;
+                           directory:(const base::FilePath&)directory;
 
 @end
 

ios/chrome/browser/sessions/session_service_ios.mm

@@ -45,11 +45,20 @@
 namespace {
 const NSTimeInterval kSaveDelay = 2.5;     // Value taken from Desktop Chrome.
 NSString* const kRootObjectKey = @"root";  // Key for the root object.
-NSString* const kSessionDirectory =
-    @"Sessions";  // The directory name inside BrowserState directory which
-                  // contain all sessions directories.
-NSString* const kSessionFileName =
-    @"session.plist";  // The session file name on disk.
+
+// The directory name inside BrowserStatedirectory which contain all sessions
+// directories.
+const base::FilePath::CharType kSessionDirectory[] =
+    FILE_PATH_LITERAL("Sessions");
+
+// The session file name on disk.
+const base::FilePath::CharType kSessionFileName[] =
+    FILE_PATH_LITERAL("session.plist");
+
+// Convert |path| to NSString.
+NSString* PathAsNSString(const base::FilePath& path) {
+  return base::SysUTF8ToNSString(path.AsUTF8Unsafe());
+}
 }
 
 @implementation NSKeyedUnarchiver (CrLegacySessionCompatibility)
@@ -104,7 +113,7 @@ NSString* const kSessionFileName =
 
 - (void)saveSession:(__weak SessionIOSFactory*)factory
           sessionID:(NSString*)sessionID
-          directory:(NSString*)directory
+          directory:(const base::FilePath&)directory
         immediately:(BOOL)immediately {
   NSString* sessionPath = [[self class] sessionPathForSessionID:sessionID
                                                       directory:directory];
@@ -123,7 +132,7 @@ NSString* const kSessionFileName =
 }
 
 - (SessionIOS*)loadSessionWithSessionID:(NSString*)sessionID
-                              directory:(NSString*)directory {
+                              directory:(const base::FilePath&)directory {
   NSString* sessionPath = [[self class] sessionPathForSessionID:sessionID
                                                       directory:directory];
   base::TimeTicks start_time = base::TimeTicks::Now();
@@ -175,59 +184,46 @@ NSString* const kSessionFileName =
   return base::mac::ObjCCastStrict<SessionIOS>(rootObject);
 }
 
-- (void)deleteAllSessionFilesInBrowserStateDirectory:(NSString*)directory
-                                          completion:
-                                              (base::OnceClosure)callback {
-  NSMutableArray<NSString*>* sessionFilesPaths = [[NSMutableArray alloc] init];
-  NSString* sessionsDirectoryPath =
-      [directory stringByAppendingPathComponent:kSessionDirectory];
+- (void)deleteAllSessionFilesInDirectory:(const base::FilePath&)directory
+                              completion:(base::OnceClosure)callback {
+  const base::FilePath sessionDirectory = directory.Append(kSessionDirectory);
   NSArray<NSString*>* allSessionIDs = [[NSFileManager defaultManager]
-      contentsOfDirectoryAtPath:sessionsDirectoryPath
+      contentsOfDirectoryAtPath:PathAsNSString(sessionDirectory)
                           error:nil];
-  for (NSString* sessionID in allSessionIDs) {
-    NSString* sessionPath =
-        [SessionServiceIOS sessionPathForSessionID:sessionID
-                                         directory:directory];
-    [sessionFilesPaths addObject:sessionPath];
-  }
 
   // If there were no session ids, then scenes are not supported fall back to
-  // the original location
-  if (sessionFilesPaths.count == 0) {
-    [sessionFilesPaths
-        addObject:[[self class] sessionPathForSessionID:nil
-                                              directory:directory]];
+  // the original location.
+  if ([allSessionIDs count] == 0) {
+    allSessionIDs = @[ @"" ];
   }
 
-  [self deletePaths:sessionFilesPaths completion:std::move(callback)];
+  [self deleteSessions:allSessionIDs
+             directory:directory
+            completion:std::move(callback)];
 }
 
 - (void)deleteSessions:(NSArray<NSString*>*)sessionIDs
-    fromBrowserStateDirectory:(NSString*)directory {
-  NSString* sessionsDirectoryPath =
-      [directory stringByAppendingPathComponent:kSessionDirectory];
+             directory:(const base::FilePath&)directory
+            completion:(base::OnceClosure)callback {
   NSMutableArray<NSString*>* paths =
       [NSMutableArray arrayWithCapacity:sessionIDs.count];
   for (NSString* sessionID : sessionIDs) {
-    [paths addObject:[sessionsDirectoryPath
-                         stringByAppendingPathComponent:sessionID]];
+    [paths addObject:[SessionServiceIOS sessionPathForSessionID:sessionID
+                                                      directory:directory]];
   }
-  [self deletePaths:paths completion:base::DoNothing()];
+  [self deletePaths:paths completion:std::move(callback)];
 }
 
 + (NSString*)sessionPathForSessionID:(NSString*)sessionID
-                           directory:(NSString*)directory {
+                           directory:(const base::FilePath&)directory {
   // TODO(crbug.com/1165798): remove when the sessionID is guaranteed to
   // always be an non-empty string.
   if (!sessionID.length)
-    return [directory stringByAppendingPathComponent:kSessionFileName];
-
-  return [NSString pathWithComponents:@[
-    directory,
-    kSessionDirectory,
-    sessionID,
-    kSessionFileName,
-  ]];
+    return PathAsNSString(directory.Append(kSessionFileName));
+
+  return PathAsNSString(directory.Append(kSessionDirectory)
+                            .Append(base::SysNSStringToUTF8(sessionID))
+                            .Append(kSessionFileName));
 }
 
 #pragma mark - Private methods

ios/chrome/browser/sessions/session_service_ios_unittest.mm

@@ -46,9 +46,8 @@ class SessionServiceTest : public PlatformTest {
   void SetUp() override {
     PlatformTest::SetUp();
     ASSERT_TRUE(scoped_temp_directory_.CreateUniqueTempDir());
-    base::FilePath directory_name = scoped_temp_directory_.GetPath();
-    directory_name = directory_name.Append(FILE_PATH_LITERAL("sessions"));
-    directory_ = base::SysUTF8ToNSString(directory_name.AsUTF8Unsafe());
+    directory_ =
+        scoped_temp_directory_.GetPath().Append(FILE_PATH_LITERAL("Sessions"));
 
     scoped_refptr<base::SequencedTaskRunner> task_runner =
         base::ThreadTaskRunnerHandle::Get();
@@ -93,30 +92,34 @@ class SessionServiceTest : public PlatformTest {
 
   SessionServiceIOS* session_service() { return session_service_; }
 
-  NSString* directory() { return directory_; }
+  const base::FilePath& directory() const { return directory_; }
+
+  NSString* directory_as_nsstring() const {
+    return base::SysUTF8ToNSString(directory().AsUTF8Unsafe());
+  }
 
  private:
   base::ScopedTempDir scoped_temp_directory_;
   base::test::TaskEnvironment task_environment_;
-  SessionServiceIOS* session_service_;
-  NSString* directory_;
+  SessionServiceIOS* session_service_ = nil;
   FakeWebStateListDelegate web_state_list_delegate_;
+  base::FilePath directory_;
 
   DISALLOW_COPY_AND_ASSIGN(SessionServiceTest);
 };
 
 TEST_F(SessionServiceTest, SessionPathForDirectory) {
+  const base::FilePath root(FILE_PATH_LITERAL("root"));
+
   EXPECT_NSEQ(@"root/session.plist",
-              [SessionServiceIOS sessionPathForSessionID:nil
-                                               directory:@"root"]);
+              [SessionServiceIOS sessionPathForSessionID:nil directory:root]);
 
   EXPECT_NSEQ(@"root/session.plist",
-              [SessionServiceIOS sessionPathForSessionID:@""
-                                               directory:@"root"]);
+              [SessionServiceIOS sessionPathForSessionID:@"" directory:root]);
 
   EXPECT_NSEQ(@"root/Sessions/session-id/session.plist",
               [SessionServiceIOS sessionPathForSessionID:@"session-id"
-                                               directory:@"root"]);
+                                               directory:root]);
 }
 
 TEST_F(SessionServiceTest, SaveSessionWindowToPath) {
@@ -134,14 +137,16 @@ TEST_F(SessionServiceTest, SaveSessionWindowToPath) {
   base::RunLoop().RunUntilIdle();
 
   NSFileManager* file_manager = [NSFileManager defaultManager];
-  EXPECT_TRUE([file_manager removeItemAtPath:directory() error:nullptr]);
+  EXPECT_TRUE([file_manager removeItemAtPath:directory_as_nsstring()
+                                       error:nullptr]);
 }
 
 TEST_F(SessionServiceTest, SaveSessionWindowToPathDirectoryExists) {
-  ASSERT_TRUE([[NSFileManager defaultManager] createDirectoryAtPath:directory()
-                                        withIntermediateDirectories:YES
-                                                         attributes:nil
-                                                              error:nullptr]);
+  ASSERT_TRUE([[NSFileManager defaultManager]
+            createDirectoryAtPath:directory_as_nsstring()
+      withIntermediateDirectories:YES
+                       attributes:nil
+                            error:nullptr]);
   std::unique_ptr<WebStateList> web_state_list = CreateWebStateList(0);
   SessionIOSFactory* factory =
       [[SessionIOSFactory alloc] initWithWebStateList:web_state_list.get()];
@@ -157,7 +162,8 @@ TEST_F(SessionServiceTest, SaveSessionWindowToPathDirectoryExists) {
   base::RunLoop().RunUntilIdle();
 
   NSFileManager* file_manager = [NSFileManager defaultManager];
-  EXPECT_TRUE([file_manager removeItemAtPath:directory() error:nullptr]);
+  EXPECT_TRUE([file_manager removeItemAtPath:directory_as_nsstring()
+                                       error:nullptr]);
 }
 
 TEST_F(SessionServiceTest, LoadSessionFromDirectoryNoFile) {

ios/chrome/browser/sessions/test_session_service.mm

@@ -20,7 +20,7 @@
 
 - (void)saveSession:(__weak SessionIOSFactory*)factory
           sessionID:(NSString*)sessionID
-          directory:(NSString*)directory
+          directory:(const base::FilePath&)directory
         immediately:(BOOL)immediately {
   NSString* sessionPath = [[self class] sessionPathForSessionID:sessionID
                                                       directory:directory];

ios/chrome/browser/test/perf_test_with_bvc_ios.mm

@@ -96,8 +96,7 @@ void PerfTestWithBVC::SetUp() {
       chrome_browser_state_.get());
 
   // Use the session to create a window which will contain the tabs.
-  NSString* state_path = base::SysUTF8ToNSString(
-      chrome_browser_state_->GetStatePath().AsUTF8Unsafe());
+  const base::FilePath& state_path = chrome_browser_state_->GetStatePath();
   SessionIOS* session =
       [[SessionServiceIOS sharedService] loadSessionWithSessionID:nil
                                                         directory:state_path];