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 <memory>
+#include <vector>
+
+#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;

[Scott Hess - ex-Googler, 2016/05/02 20:36:19]

Of these, AFAICT only Connection is necessary here.

[bburns, 2016/05/02 21:44:18]

Done.

+}
+
+namespace base {
+class SequencedTaskRunner;
+}

[Scott Hess - ex-Googler, 2016/05/02 20:36:19]

I'd put the base namespace before sql namespace.  YMMV.

[bburns, 2016/05/02 21:44:18]

Done.

+
+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);
+  ~OfflinePageMetadataStoreSQL() override;
+
+  // Implementation methods.
+  void Load(const LoadCallback& callback) override;
+  void AddOrUpdateOfflinePage(const OfflinePageItem& offline_page,
+                              const UpdateCallback& callback) override;
+  void RemoveOfflinePages(const std::vector<int64_t>& offline_ids,
+                          const UpdateCallback& callback) override;
+  void Reset(const ResetCallback& callback) override;
+
+  // If true, use an in-memory database to make testing easier.
+  // Only has an effect if you call it before a call to Load(...).
+  // Should only be used for testing.
+  void SetInMemoryDatabaseForTesting(bool in_memory);

[Scott Hess - ex-Googler, 2016/05/02 20:36:19]

I was going to suggest making this private and using FRIEND_TEST_ALL_PREFIXES()
to give appropriate access, but then I couldn't find any callers.

[Of course, if there are no callers, use_in_memory_ can go away, as can
propagating it.]

[bburns, 2016/05/02 21:44:18]

Removed.

+
+ private:
+  // Synchronous implementations, these are run on the background thread
+  // and actually do the work to access SQL.  The implementations above
+  // simply dispatch to the corresponding *Sync method on the background thread.
+  // 'runner' is where to run the callback.

[Scott Hess - ex-Googler, 2016/05/02 20:36:19]

Is there any benefit to a private static method over having them in an anonymous
namespace?  AFAICT the enum and typedefs from OfflinePageMetadataStore are
public:, so you could skip the OfflinePageMetadataStore:: in some cases (but
mostly you don't, so I might be wrong).  On the savings side, you could skip
OfflinePageMetadataStoreSQL:: in some places with an anonymous namespace.

I guess I could see some rationale to keeping in sync with the existing code,
but against that you should consider where you want the code to be long term
when the leveldb version is long gone.

[bburns, 2016/05/02 21:44:18]

I think that actually I prefer it since it makes the header a little more
self-documenting without diving into the .cc, but I could go either way.  Let me
know if you feel strongly.

[Scott Hess - ex-Googler, 2016/05/02 23:33:31]

I was more thinking in terms of not exposing any implementation details that
clients don't need to know about :-).  But IMHO this is a style issue, and it's
your code, so your preference is fine with me.

[bburns, 2016/05/03 17:15:55]

Acknowledged.

+  static void LoadSync(sql::Connection* db,
+                       const base::FilePath& path,
+                       bool use_in_memory,
+                       scoped_refptr<base::SingleThreadTaskRunner> runner,
+                       const LoadCallback& callback);
+  static void AddOrUpdateOfflinePageSync(
+      const OfflinePageItem& offline_page,
+      sql::Connection* db,
+      scoped_refptr<base::SingleThreadTaskRunner> runner,
+      const UpdateCallback& callback);
+  static void RemoveOfflinePagesSync(
+      const std::vector<int64_t>& offline_ids,
+      sql::Connection* db,
+      scoped_refptr<base::SingleThreadTaskRunner> runner,
+      const UpdateCallback& callback);
+  static void ResetSync(sql::Connection* db,
+                        scoped_refptr<base::SingleThreadTaskRunner> runner,
+                        const ResetCallback& callback);
+
+  static void NotifyLoadResult(
+      scoped_refptr<base::SingleThreadTaskRunner> runner,
+      const LoadCallback& callback,
+      LoadStatus status,
+      const std::vector<OfflinePageItem>& result);
+
+  // Background thread where all SQL access should be run.
+  scoped_refptr<base::SequencedTaskRunner> background_task_runner_;

[Scott Hess - ex-Googler, 2016/05/02 20:36:18]

Either a blank line here, or consistently run all your instance variables
together.  If I were going to group things, I'd group the path with the db.

[bburns, 2016/05/02 21:44:18]

Done.

+  // Path to the database on disk.
+  base::FilePath db_file_path_;
+
+  std::unique_ptr<sql::Connection> db_;
+
+  // only useful for testing.
+  bool use_in_memory_;
+
+  DISALLOW_COPY_AND_ASSIGN(OfflinePageMetadataStoreSQL);
+};
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_OFFLINE_PAGE_METADATA_STORE_SQL_H_