Report downloaded resources at most once

components/precache/core/precache_url_table.cc

 // 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.
 
 #include "components/precache/core/precache_url_table.h"
 
 #include <string>
 
 #include "base/logging.h"
 #include "sql/connection.h"
@@ skipping 21 matching lines @@
 
 PrecacheURLTable::~PrecacheURLTable() {}
 
 bool PrecacheURLTable::Init(sql::Connection* db) {
   DCHECK(!db_);  // Init must only be called once.
   DCHECK(db);    // The database connection must be non-NULL.
   db_ = 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) {
   Statement statement(db_->GetCachedStatement(
       SQL_FROM_HERE,
       "SELECT is_precached, was_used FROM precache_urls WHERE url=?"));
   statement.BindString(0, GetKey(url));
 
   if (statement.Step()) {
     return {/*present=*/true, /*is_precached=*/statement.ColumnBool(0),
@@ skipping 19 matching lines @@
       db_->GetCachedStatement(SQL_FROM_HERE,
                               "UPDATE precache_urls SET is_precached=0 "
                               "WHERE url=? and is_precached=1"));
   statement.BindString(0, GetKey(url));
   statement.Run();
 }
 
 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();
   }
 }
 
 void PrecacheURLTable::ClearAllForReferrerHost(int64_t referrer_host_id) {
   // Delete the URLs that are not precached.
   Statement delete_statement(
       db_->GetCachedStatement(SQL_FROM_HERE,
                               "DELETE FROM precache_urls WHERE "
                               "referrer_host_id=? AND is_precached=0"));
   delete_statement.BindInt64(0, referrer_host_id);
@@ skipping 29 matching lines @@
       SQL_FROM_HERE,
       "SELECT url, time FROM precache_urls where is_precached=1"));
 
   while (statement.Step()) {
     GURL url = GURL(statement.ColumnString(0));
     (*map)[url] = base::Time::FromInternalValue(statement.ColumnInt64(1));
   }
 }
 
 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;
 }
 
 }  // namespace precache