[Offline Pages] Introducing StorageManagerClient interface.

components/offline_pages/offline_page_storage_manager.cc

Index: components/offline_pages/offline_page_storage_manager.cc
diff --git a/components/offline_pages/offline_page_storage_manager.cc b/components/offline_pages/offline_page_storage_manager.cc
index e4deea95d6e2e8e242fd7ba2c41f595701352113..25967a7d53944633928dc90e6a4afc0257060c41 100644
--- a/components/offline_pages/offline_page_storage_manager.cc
+++ b/components/offline_pages/offline_page_storage_manager.cc
@@ -8,27 +8,27 @@
 #include "components/offline_pages/client_policy_controller.h"
 #include "components/offline_pages/offline_page_client_policy.h"
 #include "components/offline_pages/offline_page_item.h"
-#include "components/offline_pages/offline_page_model.h"
 #include "components/offline_pages/offline_page_types.h"
+#include "components/offline_pages/storage_manager_client.h"
 
 namespace offline_pages {
 
-OfflinePageStorageManager::OfflinePageStorageManager(OfflinePageModel* model)
-    : model_(model),
-      policy_controller_(model->GetPolicyController()),
+OfflinePageStorageManager::OfflinePageStorageManager(
+    StorageManagerClient* client)
+    : client_(client),
+      policy_controller_(client->GetPolicyController()),

[fgorski, 2016/05/11 22:39:00]

I am wondering if injecting policy controller independently would make more
sense. If this is the only place where you are using GetPolicyController method
from the interface, perhaps it does not belong there. Think about it and we can
discuss it.

[romax, 2016/05/11 23:54:57]

yes i agree with this.

       in_progress_(false),
       weak_ptr_factory_(this) {}
 
-OfflinePageStorageManager::~OfflinePageStorageManager() {
-}
+OfflinePageStorageManager::~OfflinePageStorageManager() {}
 
 void OfflinePageStorageManager::ClearPagesIfNeeded(
     const ClearPageCallback& callback) {
   if (!ShouldClearPages())
     return;
   in_progress_ = true;
-  model_->GetAllPages(base::Bind(&OfflinePageStorageManager::ClearExpiredPages,
-                                 weak_ptr_factory_.GetWeakPtr(), callback));
+  client_->GetAllPages(base::Bind(&OfflinePageStorageManager::ClearExpiredPages,
+                                  weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
 void OfflinePageStorageManager::ClearExpiredPages(
@@ -37,7 +37,7 @@ void OfflinePageStorageManager::ClearExpiredPages(
   DCHECK(in_progress_);
   std::vector<int64_t> offline_ids;
   GetExpiredPageIds(pages, offline_ids);
-  model_->DeletePagesByOfflineId(
+  client_->DeletePagesByOfflineId(
       offline_ids,
       base::Bind(&OfflinePageStorageManager::OnExpiredPagesDeleted,
                  weak_ptr_factory_.GetWeakPtr(), callback, offline_ids.size()));
@@ -61,7 +61,7 @@ void OfflinePageStorageManager::OnExpiredPagesDeleted(
 }
 
 bool OfflinePageStorageManager::ShouldClearPages() {
-  return !in_progress_ && model_->is_loaded();
+  return !in_progress_ && client_->IsLoaded();

[fgorski, 2016/05/11 22:39:00]

I suspect that IsLoaded (in either form) will not survive on the client
interface and we will drop it. After all, we can initialize the storage manager
after the model is loaded and this check never has to happen.

Think about that, please.

Both suggestions in this file serve minimize the interface (and therefore
dependencies), but let me know if you think there is anything wrong with that.

[romax, 2016/05/11 23:54:57]

Done.

 }
 
 bool OfflinePageStorageManager::IsPageExpired(const OfflinePageItem& page) {