Report downloaded resources at most once

components/precache/core/precache_url_table.cc

Index: components/precache/core/precache_url_table.cc
diff --git a/components/precache/core/precache_url_table.cc b/components/precache/core/precache_url_table.cc
index 830ef892af927bcbf56df86d37c10375d44827d0..18ebad3eb1d4503e8995edc5e79299f9b23a1321 100644
--- a/components/precache/core/precache_url_table.cc
+++ b/components/precache/core/precache_url_table.cc
@@ -39,21 +39,57 @@ bool PrecacheURLTable::Init(sql::Connection* db) {
   return CreateTableIfNonExistent();
 }
 
+bool PrecacheURLTable::BindStatementAndRun(const GURL& url,
+                                           int64_t referrer_host_id,
+                                           bool is_precached,
+                                           const base::Time& precache_time,
+                                           Statement* statement) {
+  statement->BindInt64(0, is_precached ? 1 : 0);
+  statement->BindInt64(1, precache_time.ToInternalValue());
+  statement->BindInt64(2, referrer_host_id);
+  statement->BindString(3, GetKey(url));

[twifkak, 2016/12/20 01:41:59]

Make the order of the bindvars and the order of the function params the same.

[jamartin, 2016/12/20 21:09:34]

NA.

+  return statement->Run();
+}
+
 void PrecacheURLTable::AddURL(const GURL& url,
                               int64_t referrer_host_id,
                               bool is_precached,
+                              bool was_cached,
                               const base::Time& precache_time) {
-  Statement statement(
-      db_->GetCachedStatement(SQL_FROM_HERE,
-                              "INSERT OR REPLACE INTO precache_urls (url, "
-                              "referrer_host_id, was_used, is_precached, time) "
-                              "VALUES(?,?,0,?,?)"));
-
-  statement.BindString(0, GetKey(url));
-  statement.BindInt64(1, referrer_host_id);
-  statement.BindInt64(2, is_precached ? 1 : 0);
-  statement.BindInt64(3, precache_time.ToInternalValue());
-  statement.Run();
+  Statement statement;
+  if (is_precached && !was_cached) {

[twifkak, 2016/12/20 01:41:59]

Note that "is_precached" here is a misnomer, since RecordURLPrefetch passes a
value of `!was_cached || precache_url_table_.GetURLInfo(url).is_precached`.
Since (A && B) || A == A, you can simplify your if statement to:

if (!was_cached) {

[jamartin, 2016/12/20 21:09:34]

Good catch.

This is not applicable anymore but, even though your logic is correct, I
wouldn't have made this change since it is easy to change the callsite or this
code and break this assumption. My code was superflueous but it was explicit.

+    // We just precached from the network. Set is_download_reported = 0.
+    Statement statement(
+        db_->GetCachedStatement(SQL_FROM_HERE,
+                                "INSERT OR REPLACE INTO precache_urls "
+                                "(was_used, is_precached, time, "
+                                " is_download_reported, referrer_host_id, url) "
+                                "VALUES(0,?,?,0,?,?)"));
+    BindStatementAndRun(url, referrer_host_id, is_precached, precache_time,
+                        &statement);
+  } else {
+    // Update the entry but do not modify is_download_reported.
+    // We need to do an INSERT possibly followed by an UPDATE because there is
+    // no INSERT OR UPDATE in sqlite.
+    Statement insert(db_->GetCachedStatement(
+        SQL_FROM_HERE,
+        "INSERT OR ABORT INTO precache_urls "
+        "(was_used, is_precached, time, referrer_host_id, url) "
+        "VALUES(0,?,?,?,?)"));
+    // If this FAILS during tests, it is because you need to set_error_callback
+    // in the SQL connection. The default behavior in DEBUG is crash whereas
+    // the default behavior live is the expected, just to return false.
+    if (!BindStatementAndRun(url, referrer_host_id, is_precached, precache_time,

[twifkak, 2016/12/20 01:42:00]

I'd rather move this logic out of PrecacheURLTable and into PrecacheDatabase,
for two reasons:

1. The only caller of AddURL is PrecacheDatabase::RecordURLPrefetch{,Internal},
which already has some confusing boolean logic. Spreading it across
caller/callee makes it unnecessarily more confusing. I think the intent of this
PrecacheURLTable class is to be as dumb as possible, and let the logic be in
PrecacheDatabase.

2. I'm not a fan of relying on an error condition (for a few reasons [*]), and
you should be able to avoid it by adding is_download_reported to PrecacheURLInfo
and moving this logic into RecordURLPrefetch, which already does a SELECT via
its GetURLInfo call.

That would change your FSM comment at the bottom to be
`AddUrl(!is_download_reported)`, which I guess is not entirely satisfying but
overall I think it'd be an improvement in readability. WDYT?

---

[*] a. As you state, it relies on some outside configuration, which is brittle
-- maybe they want to change the error_callback setting in the future, and will
have to contend with this code.

b. Maybe it's slower? I have no basis for this, other than many languages have
slow exceptions, but that's generally because they have to generate a backtrace.

c. (nit) What if it errors for some other reason than the "OR ABORT"? Per
https://sqlite.org/lang_conflict.html the abort happens if there's an
"applicable constraint violation", but maybe there's some other transient I/O
error or something. In this case, the result will be "It does a zero-row update"
which is equivalent to what would have happened anyway if the INSERT failed, so
this is just academic.

[jamartin, 2016/12/20 21:09:34]

Done.

+                             &insert)) {
+      Statement update(db_->GetCachedStatement(
+          SQL_FROM_HERE,
+          "UPDATE precache_urls SET "
+          "was_used=0, is_precached=?, time=?, referrer_host_id=?"
+          "WHERE url=?"));
+      BindStatementAndRun(url, referrer_host_id, is_precached, precache_time,
+                          &update);
+    }
+  }
 }
 
 PrecacheURLInfo PrecacheURLTable::GetURLInfo(const GURL& url) {
@@ -93,17 +129,27 @@ void PrecacheURLTable::SetURLAsNotPrecached(const GURL& url) {
 void PrecacheURLTable::GetURLListForReferrerHost(
     int64_t referrer_host_id,
     std::vector<GURL>* used_urls,
-    std::vector<GURL>* unused_urls) {
-  Statement statement(db_->GetCachedStatement(
-      SQL_FROM_HERE,
-      "SELECT url, was_used from precache_urls where referrer_host_id=?"));
+    std::vector<GURL>* downloaded_urls) {
+  Statement statement(
+      db_->GetCachedStatement(SQL_FROM_HERE,
+                              "SELECT url, was_used, is_download_reported "
+                              "from precache_urls where referrer_host_id=?"));
   statement.BindInt64(0, referrer_host_id);
   while (statement.Step()) {
     GURL url(statement.ColumnString(0));
     if (statement.ColumnInt(1))
       used_urls->push_back(url);
-    else
-      unused_urls->push_back(url);
+    if (!statement.ColumnInt(2))
+      downloaded_urls->push_back(url);
+  }
+  // Set is_download_reported for all the downloaded_urls.
+  if (!downloaded_urls->empty()) {

[twifkak, 2016/12/20 01:42:00]

Likewise, consider moving this block into a separate function and calling it
directly from PrecacheDatabase.

[jamartin, 2016/12/20 21:09:34]

Done. I should have noticed this when I was testing the same logic in two
different places :-(

Very good review. Kudos.

+    Statement update(db_->GetCachedStatement(
+        SQL_FROM_HERE,
+        "UPDATE precache_urls SET is_download_reported=1 "
+        "WHERE referrer_host_id=?"));
+    update.BindInt64(0, referrer_host_id);
+    update.Run();
   }
 }
 
@@ -153,26 +199,39 @@ void PrecacheURLTable::GetAllDataForTesting(std::map<GURL, base::Time>* map) {
 }
 
 bool PrecacheURLTable::CreateTableIfNonExistent() {
+  // TODO(jamartin): The PRIMARY KEY should be (url, referrer_host_id).
   if (!db_->DoesTableExist("precache_urls")) {
     return db_->Execute(
         "CREATE TABLE precache_urls "
         "(url TEXT PRIMARY KEY, referrer_host_id INTEGER, was_used INTEGER, "
         "is_precached INTEGER, "
-        "time INTEGER)");
+        "time INTEGER, is_download_reported INTEGER)");
   } else {
     // Migrate the table by creating the missing columns.
     if (!db_->DoesColumnExist("precache_urls", "was_used") &&
-        !db_->Execute("ALTER TABLE precache_urls ADD COLUMN was_used INTEGER"))
+        !db_->Execute("ALTER TABLE precache_urls "
+                      "ADD COLUMN was_used INTEGER")) {
       return false;
+    }
     if (!db_->DoesColumnExist("precache_urls", "is_precached") &&
-        !db_->Execute(
-            "ALTER TABLE precache_urls ADD COLUMN is_precached "
-            "INTEGER default 1"))
+        !db_->Execute("ALTER TABLE precache_urls ADD COLUMN is_precached "
+                      "INTEGER default 1")) {
       return false;
+    }
     if (!db_->DoesColumnExist("precache_urls", "referrer_host_id") &&
         !db_->Execute(
-            "ALTER TABLE precache_urls ADD COLUMN referrer_host_id INTEGER"))
+            "ALTER TABLE precache_urls ADD COLUMN referrer_host_id INTEGER")) {
+      return false;
+    }
+    if (!db_->DoesColumnExist("precache_urls", "is_download_reported") &&
+        !db_->Execute("ALTER TABLE precache_urls "
+                      "ADD COLUMN is_download_reported INTEGER")) {
+      // The FSM for is_download_reported has q_0 = 0 and delta of:
+      //   Q    Sigma (conditioned on host id)    Q'
+      //   0      GetURLListForReferrerHost       1  // Reported.
+      //   1  AddUrl(is_precached && !was_cached) 0  // Downloaded from network.
       return false;
+    }
   }
   return true;
 }