initial add of SQL based storage

components/offline_pages/offline_page_metadata_store_sql.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_METADATA_STORE_SQL_H_
+#define COMPONENTS_OFFLINE_PAGES_OFFLINE_PAGE_METADATA_STORE_SQL_H_
+
+#include <stdint.h>
+
+#include <vector>
+
+#include "base/callback.h"

[fgorski, 2016/03/24 16:27:56]

are you using this include?

[bburns, 2016/03/25 23:07:38]

removed.

+#include "base/files/file_path.h"
+#include "base/memory/weak_ptr.h"
+#include "components/offline_pages/offline_page_metadata_store.h"
+
+namespace sql {
+class Connection;
+class MetaTable;
+class Statement;
+class StatementID;
+}
+
+namespace base {
+class SequencedTaskRunner;
+}
+
+namespace offline_pages {
+
+// OfflinePageMetadataStoreSQL is an instance of OfflinePageMetadataStore
+// which is implemented using a SQLite database.
+class OfflinePageMetadataStoreSQL : public OfflinePageMetadataStore {
+ public:
+  OfflinePageMetadataStoreSQL(
+      scoped_refptr<base::SequencedTaskRunner> background_task_runner,
+      const base::FilePath& database_dir);
+  virtual ~OfflinePageMetadataStoreSQL();
+
+  // Implementation methods
+  virtual void Load(const LoadCallback& callback);
+  virtual void AddOrUpdateOfflinePage(const OfflinePageItem& offline_page,
+                                      const UpdateCallback& callback);
+  virtual void RemoveOfflinePages(const std::vector<int64_t>& offline_ids,
+                                  const UpdateCallback& callback);
+  virtual void Reset(const ResetCallback& callback);
+
+ private:
+  // Synchronous implementations

[fgorski, 2016/03/24 16:27:55]

Please explain the comment in more details.

Consider adding a class/file level comment that explains how this store is meant
to be used with respect to:
* concurrent access
* asynch/synch
* anything else that seems important enough.

[bburns, 2016/03/25 23:07:38]

Done (well, I explained the sync/threading stuff)  I figure the concurrent stuff
is covered in the interface definition.  (or it should be anyway)

+  void LoadSync(const LoadCallback& callback);
+  void AddOrUpdateOfflinePageSync(const OfflinePageItem& offline_page,
+                                  const UpdateCallback& callback);
+  void RemoveOfflinePagesSync(const std::vector<int64_t>& offline_ids,
+                              const UpdateCallback& callback);
+
+  void NotifyLoadResult(const LoadCallback& callback,
+                        LoadStatus status,
+                        const std::vector<OfflinePageItem>& result);
+

[fgorski, 2016/03/24 16:27:55]

Please document the fields with respect to their purpose. especially btr and
meta_table_

[bburns, 2016/03/25 23:07:38]

Done.

+  scoped_refptr<base::SequencedTaskRunner> background_task_runner_;
+  base::FilePath db_file_path_;
+  base::WeakPtrFactory<OfflinePageMetadataStoreSQL> weak_ptr_factory_;

[fgorski, 2016/03/24 16:27:55]

From weak_ptr.h:
// Member variables should appear before the WeakPtrFactory, to ensure
// that any WeakPtrs to Controller are invalidated before its members
// variable's destructors are executed, rendering them invalid.

[bburns, 2016/03/25 23:07:38]

Done.

+
+  scoped_ptr<sql::Connection> db_;

[fgorski, 2016/03/24 16:27:55]

include base/memory/scoped_ptr.h

[bburns, 2016/03/25 23:07:38]

Done.

+  scoped_ptr<sql::MetaTable> meta_table_;
+
+  // only useful for testing

[Dmitry Titov, 2016/03/24 23:22:20]

don't see anything modifying or initializing this.

[bburns, 2016/03/25 23:07:38]

Now that there are tests, its used.

+  bool use_in_memory_;
+};

[fgorski, 2016/03/24 16:27:55]

DISALLOW_COPY_AND_ASSIGN(OfflinePageMetadataStoreSql);

[bburns, 2016/03/25 23:07:38]

Done.

+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_OFFLINE_PAGE_METADATA_STORE_SQL_H_