[Offline Pages] Implement OfflinePageStorageManager.

components/offline_pages/offline_page_storage_manager.h

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_OFFLINE_PAGES_OFFLINE_PAGE_STORAGE_MANAGER_H_
+#define COMPONENTS_OFFLINE_PAGES_OFFLINE_PAGE_STORAGE_MANAGER_H_
+
+#include <stdint.h>
+
+#include <vector>
+
+#include "components/offline_pages/client_policy_controller.h"
+#include "components/offline_pages/offline_page_model.h"

[fgorski, 2016/05/05 04:53:08]

Both can be forward declared. Include in .cc

[romax, 2016/05/05 21:00:03]

Done.

[romax, 2016/05/06 01:02:32]

I don't know how to forward declare a typedef in offline_page_model.h so i only
removed client policy controller...

+
+namespace offline_pages {
+
+// TODO(romax): Keep this as a default value until I find a way to get
+// storage size in C++. (20MB)
+static const int64_t kDefaultStorageSize = 20 * (1 << 20);
+
+// This class is used for storage management of offline pages. It provides
+// a ClearPagesIfNeeded method which is used to clear expired offline pages
+// based on last_access_time and lifetime policy of its namespace.
+// It has its own throttle mechanism so calling the method would not be
+// guaranteed to clear the pages immediately.
+// This manager should be owned by OfflinePageModel since it needs the

[fgorski, 2016/05/05 04:53:09]

This sentence is misleading.

Message 1: OPM should own and control the life of OPSM
Message 2: OPM will be used to get/remove pages.

They can be in separate sentences. There is no causal dependency between them.

[romax, 2016/05/05 21:00:03]

Done.

+// functionality to delete/get pages.
+class OfflinePageStorageManager {

[fgorski, 2016/05/05 04:53:09]

please create explicit tests for this class.

[fgorski, 2016/05/06 05:57:24]

??

+ public:
+  typedef base::Callback<void(int)> ClearPageCallback;

[fgorski, 2016/05/05 04:53:09]

ClearPagesCallback.

add at least a boolean variable to pass the status (to communicate failure)
You can also consider an enum, to communicate the failure even better.

also I find it reasonable to add a comment next to type, e.g. int /* number of
deleted pages */

[romax, 2016/05/06 01:02:32]

In the next patch I'll be using DeletePageResult. But i'm open to add a new kind
of enum here.

+
+  OfflinePageStorageManager(OfflinePageModel* model,

[fgorski, 2016/05/05 04:53:09]

1. Will this work as an implicit constructor if I only have model where I expect
the Manager? Please figure it out, I am curious

2. Turn it into an explicit ctor accepting the model. Set size_ to the value of
const in initializer list.

[romax, 2016/05/06 01:02:32]

Done.

+                            int64_t totalSize = kDefaultStorageSize);
+  ~OfflinePageStorageManager();
+
+  void ClearPagesIfNeeded(const ClearPageCallback& callback);

[fgorski, 2016/05/05 04:53:09]

documentation.

[romax, 2016/05/05 21:00:03]

Acknowledged.

+
+ private:
+  void PurgePages(const ClearPageCallback& callback,
+                  const OfflinePageModel::MultipleOfflinePageItemResult& pages);
+
+  int GetExpiredPageIds(

[fgorski, 2016/05/05 04:53:09]

document. What is the result?

[romax, 2016/05/05 21:00:03]

it was the same as offline_ids.size().. so changed to void

+      const OfflinePageModel::MultipleOfflinePageItemResult& pages,
+      std::vector<int64_t>& offline_ids);

[fgorski, 2016/05/05 04:53:09]

const & ?

[romax, 2016/05/05 21:00:03]

no I meant to change the value...

+
+  void OnExpiredPagesDeleted(const ClearPageCallback& callback,
+                             int pages_to_clear,
+                             OfflinePageModel::DeletePageResult result);
+
+  bool ShouldClearPages();
+
+  bool isPageExpired(const OfflinePageItem& page);

[fgorski, 2016/05/05 04:53:08]

IsPageExpired

[romax, 2016/05/05 21:00:03]

Done.

+
+  // Not owning.

[fgorski, 2016/05/05 04:53:09]

Not owned.

[romax, 2016/05/05 21:00:03]

Done.

+  OfflinePageModel* model_;
+
+  // Not owning

[fgorski, 2016/05/05 04:53:09]

Not owned.

[romax, 2016/05/05 21:00:03]

Done.

+  ClientPolicyController* policy_controller_;
+
+  bool in_progress_;
+
+  int64_t size_;
+
+  base::WeakPtrFactory<OfflinePageStorageManager> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(OfflinePageStorageManager);

[fgorski, 2016/05/05 04:53:09]

include base/macros.h

[romax, 2016/05/05 21:00:03]

Done.

+};
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_OFFLINE_PAGE_STORAGE_MANAGER_H_