Precache tracking database

components/precache/core/precache_database.h

+// Copyright 2013 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_PRECACHE_CORE_PRECACHE_DATABASE_H_
+#define COMPONENTS_PRECACHE_CORE_PRECACHE_DATABASE_H_
+
+#include "base/basictypes.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/threading/non_thread_safe.h"
+#include "base/time/time.h"
+#include "url/gurl.h"
+
+namespace sql {
+class Connection;
+}
+
+namespace precache {
+
+class PrecacheURLTable;
+class PrecacheStatisticsTable;

[mmenke, 2013/10/30 18:29:08]

nit:  Alphabetize.

[sclittle, 2013/10/30 20:29:48]

Done.

+
+// Class that tracks information related to precaching. This class can be
+// constructed or destroyed on any threads, but all other methods must be called
+// on the same thread (e.g. the DB thread).
+class PrecacheDatabase : public base::RefCountedThreadSafe<PrecacheDatabase>,
+                         base::NonThreadSafe {
+ public:
+  // A PrecacheDatabase can be constructed on any thread.
+  PrecacheDatabase();
+
+  // Initializes the precache database with the specified connection. The
+  // PrecacheDatabase takes ownership of |db|. Init must be called before any
+  // other methods.
+  void Init(scoped_ptr<sql::Connection> db);
+
+  // Reports UMA of precache statistics for days up to and including the day of
+  // |end_date|. The precache statistics for these days are deleted from the
+  // database after they have been reported, and expired precache history is
+  // deleted from the precache URL table. |end_date| is passed by value and not
+  // by const reference so that this method can be posted on a thread.

[mmenke, 2013/10/30 18:29:08]

I don't believe this is true - when you pass by reference, the data is actually
still copied to the callback.

struct CallbackParamTraits {
  typedef const T& ForwardType;  // <-- type for the function is a reference
  typedef T StorageType;  // <-- but it's copied for storage.
};

Time is often passed by value rather than reference, so think the code is fine,
should just remove that comment.

[sclittle, 2013/10/30 20:29:48]

Done. I've changed it to pass Time by reference as well and forward declared
Time.

+  void ReportAndDeleteOldStats(base::Time end_date);
+
+  // Update precache-related metrics in response to a URL being fetched.
+  // Parameters are passed by value and not by const reference so that this
+  // method can be posted on a thread.

[mmenke, 2013/10/30 18:29:08]

Again, this is not true.  Should pass URL by reference.  By value is fine for
everything else.  Can then also remove gurl.h from the includes.

[sclittle, 2013/10/30 20:29:48]

Done.

+  void RecordURLFetched(GURL url, base::Time fetch_time, int64 size,
+                        bool was_cached, bool is_precaching, bool is_cellular);

[mmenke, 2013/10/30 18:29:08]

You definitely need to define is_precaching here.  I had assumed it meant
precaching was enabled, and it seems to depend on command line flags, rather
than what initiated the request.

[mmenke, 2013/10/30 18:29:08]

nit:  |is_cellular| could be clearer - I suggest is_connection_cellular.

[sclittle, 2013/10/30 20:29:48]

Done.

[sclittle, 2013/10/30 20:29:48]

It actually meant whether or not we are currently in the middle of precaching,
i.e. that the fetch is a precache.

Renamed to |is_precache| to be clearer, and added comment.

+
+ private:
+  friend class base::RefCountedThreadSafe<PrecacheDatabase>;
+  friend class PrecacheDatabaseTest;
+
+  ~PrecacheDatabase();
+
+  bool IsDatabaseAccessible() const;
+
+  scoped_ptr<sql::Connection> db_;
+  scoped_ptr<PrecacheURLTable> precache_url_table_;
+  scoped_ptr<PrecacheStatisticsTable> precache_statistics_table_;
+
+  DISALLOW_COPY_AND_ASSIGN(PrecacheDatabase);
+};
+
+}  // namespace precache
+
+#endif  // COMPONENTS_PRECACHE_CORE_PRECACHE_DATABASE_H_