SavedFilesService interface in //extensions

apps/app_restore_service_browsertest.cc

Index: apps/app_restore_service_browsertest.cc
diff --git a/apps/app_restore_service_browsertest.cc b/apps/app_restore_service_browsertest.cc
index 9946a8a87c46881bda7200a6e7e3bf3b37fa3f87..4cbaa89d6ac761f5346a8131372c05e79a136831 100644
--- a/apps/app_restore_service_browsertest.cc
+++ b/apps/app_restore_service_browsertest.cc
@@ -4,13 +4,14 @@
 
 #include "apps/app_restore_service.h"
 #include "apps/app_restore_service_factory.h"
-#include "apps/saved_files_service.h"
+#include "apps/saved_files_service_impl.h"
 #include "base/threading/thread_restrictions.h"
 #include "chrome/browser/apps/app_browsertest_util.h"
 #include "chrome/browser/extensions/api/file_system/file_system_api.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/test/test_utils.h"
+#include "extensions/browser/api/file_system/saved_files_service.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/notification_types.h"
 #include "extensions/common/extension.h"
@@ -20,6 +21,7 @@ using extensions::Extension;
 using extensions::ExtensionPrefs;
 using extensions::ExtensionSystem;
 using extensions::FileSystemChooseEntryFunction;
+using extensions::SavedFilesService;
 
 // TODO(benwells): Move PlatformAppBrowserTest to apps namespace in apps
 // component.
@@ -131,9 +133,10 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, FileAccessIsSavedToPrefs) {
       "file_access_saved_to_prefs_test", "fileWritten");
   ASSERT_TRUE(extension);
 
-  SavedFilesService* saved_files_service = SavedFilesService::Get(profile());
+  SavedFilesServiceImpl* saved_files_service =

[benwells, 2017/06/22 07:56:09]

Hmmm ... getting an Impl class like this looks a bit strange. After seeing this
I think the second suggestion I had wrt naming would be better.

That is, keep SavedFilesService as it is (i.e. no SavedFilesService) but
introduce a SavedFilesServiceInterface.

Then users of the class would not be dealing with Impls but 'proper' classes. It
would also make the fallout from this change much less.

WDYT?

PS sorry to be a pain.

[michaelpg, 2017/06/23 20:44:04]

Done. It does make the change tighter, thanks. I also reverted the
SavedFileEntry => ::Entry change because SavedFilesServiceInterface::Entry was a
bit much. SavedFileEntry now has its own header file though.

+      SavedFilesServiceImpl::Get(profile());
 
-  std::vector<SavedFileEntry> file_entries =
+  std::vector<SavedFilesService::Entry> file_entries =
       saved_files_service->GetAllFileEntries(extension->id());
   // One for the read-only file entry and one for the writable file entry.
   ASSERT_EQ(2u, file_entries.size());
@@ -177,15 +180,17 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, MAYBE_FileAccessIsRestored) {
 
   ExtensionPrefs* extension_prefs =
       ExtensionPrefs::Get(browser()->profile());
-  SavedFilesService* saved_files_service = SavedFilesService::Get(profile());
-  std::vector<SavedFileEntry> file_entries =
+  SavedFilesServiceImpl* saved_files_service =
+      SavedFilesServiceImpl::Get(profile());
+  std::vector<SavedFilesService::Entry> file_entries =
       saved_files_service->GetAllFileEntries(extension->id());
   extension_suspended.Wait();
 
   // Simulate a restart by populating the preferences as if the browser didn't
   // get time to clean itself up.
   extension_prefs->SetExtensionRunning(extension->id(), true);
-  for (std::vector<SavedFileEntry>::const_iterator it = file_entries.begin();
+  for (std::vector<SavedFilesService::Entry>::const_iterator it =
+           file_entries.begin();
        it != file_entries.end(); ++it) {
     saved_files_service->RegisterFileEntry(
         extension->id(), it->id, it->path, it->is_directory);