Database usage adjustment for .../history

chrome/browser/history/visitsegment_database.cc

 // Copyright (c) 2011 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 "chrome/browser/history/visitsegment_database.h"
 
 #include <math.h>
 
 #include <algorithm>
 #include <string>
@@ skipping 30 matching lines @@
 }
 
 bool VisitSegmentDatabase::InitSegmentTables() {
   // Segments table.
   if (!GetDB().DoesTableExist("segments")) {
     if (!GetDB().Execute("CREATE TABLE segments ("
         "id INTEGER PRIMARY KEY,"
         "name VARCHAR,"
         "url_id INTEGER NON NULL,"
         "pres_index INTEGER DEFAULT -1 NOT NULL)")) {
-      NOTREACHED();
       return false;
     }
 
-    if (!GetDB().Execute("CREATE INDEX segments_name ON segments(name)")) {
-      NOTREACHED();
+    if (!GetDB().Execute(
+        "CREATE INDEX segments_name ON segments(name)")) {
       return false;
     }
   }
 
   // This was added later, so we need to try to create it even if the table
   // already exists.
   if (!GetDB().Execute("CREATE INDEX IF NOT EXISTS segments_url_id ON "
                        "segments(url_id)"))
     return false;
 
   // Segment usage table.
   if (!GetDB().DoesTableExist("segment_usage")) {
     if (!GetDB().Execute("CREATE TABLE segment_usage ("
         "id INTEGER PRIMARY KEY,"
         "segment_id INTEGER NOT NULL,"
         "time_slot INTEGER NOT NULL,"
         "visit_count INTEGER DEFAULT 0 NOT NULL)")) {
-      NOTREACHED();
       return false;
     }
     if (!GetDB().Execute(
         "CREATE INDEX segment_usage_time_slot_segment_id ON "
         "segment_usage(time_slot, segment_id)")) {
-      NOTREACHED();
       return false;
     }
   }
 
   // Added in a later version, so we always need to try to creat this index.
   if (!GetDB().Execute("CREATE INDEX IF NOT EXISTS segments_usage_seg_id "
                        "ON segment_usage(segment_id)"))
     return false;
 
   // Presentation index table.
@@ skipping 46 matching lines @@
   r.ClearRef();
   r.ClearPort();
 
   return url.ReplaceComponents(r).spec();
 }
 
 SegmentID VisitSegmentDatabase::GetSegmentNamed(
     const std::string& segment_name) {
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "SELECT id FROM segments WHERE name = ?"));
-  if (!statement)
-    return 0;
+  statement.BindString(0, segment_name);
 
-  statement.BindString(0, segment_name);
   if (statement.Step())
     return statement.ColumnInt64(0);
   return 0;
 }
 
 bool VisitSegmentDatabase::UpdateSegmentRepresentationURL(SegmentID segment_id,
                                                           URLID url_id) {
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "UPDATE segments SET url_id = ? WHERE id = ?"));
-  if (!statement)
-    return false;
-
   statement.BindInt64(0, url_id);
   statement.BindInt64(1, segment_id);
+
   return statement.Run();
 }
 
 URLID VisitSegmentDatabase::GetSegmentRepresentationURL(SegmentID segment_id) {
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "SELECT url_id FROM segments WHERE id = ?"));
-  if (!statement)
-    return 0;
+  statement.BindInt64(0, segment_id);
 
-  statement.BindInt64(0, segment_id);
   if (statement.Step())
     return statement.ColumnInt64(0);
   return 0;
 }
 
 SegmentID VisitSegmentDatabase::CreateSegment(URLID url_id,
                                               const std::string& segment_name) {
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "INSERT INTO segments (name, url_id) VALUES (?,?)"));
-  if (!statement)
-    return false;
-
   statement.BindString(0, segment_name);
   statement.BindInt64(1, url_id);
+
   if (statement.Run())
     return GetDB().GetLastInsertRowId();
-  return false;
+  return 0;
 }
 
 bool VisitSegmentDatabase::IncreaseSegmentVisitCount(SegmentID segment_id,
                                                      base::Time ts,
                                                      int amount) {
   base::Time t = ts.LocalMidnight();
 
   sql::Statement select(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "SELECT id, visit_count FROM segment_usage "
       "WHERE time_slot = ? AND segment_id = ?"));
-  if (!select)
+  select.BindInt64(0, t.ToInternalValue());
+  select.BindInt64(1, segment_id);
+
+  if (!select.is_valid())
     return false;
 
-  select.BindInt64(0, t.ToInternalValue());
-  select.BindInt64(1, segment_id);
   if (select.Step()) {
     sql::Statement update(GetDB().GetCachedStatement(SQL_FROM_HERE,
         "UPDATE segment_usage SET visit_count = ? WHERE id = ?"));
-    if (!update)
-      return false;
-
     update.BindInt64(0, select.ColumnInt64(1) + static_cast<int64>(amount));
     update.BindInt64(1, select.ColumnInt64(0));
+
     return update.Run();
-
   } else {
     sql::Statement insert(GetDB().GetCachedStatement(SQL_FROM_HERE,
         "INSERT INTO segment_usage "
         "(segment_id, time_slot, visit_count) VALUES (?, ?, ?)"));
-    if (!insert)
-      return false;
-
     insert.BindInt64(0, segment_id);
     insert.BindInt64(1, t.ToInternalValue());
     insert.BindInt64(2, static_cast<int64>(amount));
+
     return insert.Run();
   }
 }
 
 void VisitSegmentDatabase::QuerySegmentUsage(
     base::Time from_time,
     int max_result_count,
     std::vector<PageUsageData*>* results) {
   // This function gathers the highest-ranked segments in two queries.
   // The first gathers scores for all segments.
   // The second gathers segment data (url, title, etc.) for the highest-ranked
   // segments.
   // TODO(evanm): this disregards the "presentation index", which was what was
   // used to lock results into position.  But the rest of our code currently
   // does as well.
 
   // Gather all the segment scores.
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "SELECT segment_id, time_slot, visit_count "
       "FROM segment_usage WHERE time_slot >= ? "
       "ORDER BY segment_id"));
-  if (!statement) {
-    NOTREACHED() << GetDB().GetErrorMessage();
+  if (!statement.is_valid())
     return;
-  }
 
   base::Time ts = from_time.LocalMidnight();
   statement.BindInt64(0, ts.ToInternalValue());
 
   base::Time now = base::Time::Now();
   SegmentID last_segment_id = 0;
   PageUsageData* pud = NULL;
   float score = 0;
   while (statement.Step()) {
     SegmentID segment_id = statement.ColumnInt64(0);
@@ skipping 35 matching lines @@
     STLDeleteContainerPointers(results->begin() + max_result_count,
                                results->end());
     results->resize(max_result_count);
   }
 
   // Now fetch the details about the entries we care about.
   sql::Statement statement2(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "SELECT urls.url, urls.title FROM urls "
       "JOIN segments ON segments.url_id = urls.id "
       "WHERE segments.id = ?"));
-  if (!statement2) {
-    NOTREACHED() << GetDB().GetErrorMessage();
+
+  if (!statement2.is_valid())
     return;
-  }
+
   for (size_t i = 0; i < results->size(); ++i) {
     PageUsageData* pud = (*results)[i];
     statement2.BindInt64(0, pud->GetID());
     if (statement2.Step()) {
       pud->SetURL(GURL(statement2.ColumnString(0)));
       pud->SetTitle(UTF8ToUTF16(statement2.ColumnString(1)));
     }
     statement2.Reset();
   }
 }
 
-void VisitSegmentDatabase::DeleteSegmentData(base::Time older_than) {
+bool VisitSegmentDatabase::DeleteSegmentData(base::Time older_than) {
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "DELETE FROM segment_usage WHERE time_slot < ?"));
-  if (!statement)
-    return;
+  statement.BindInt64(0, older_than.LocalMidnight().ToInternalValue());
 
-  statement.BindInt64(0, older_than.LocalMidnight().ToInternalValue());
-  if (!statement.Run())
-    NOTREACHED();
+  return statement.Run();
 }
 
-void VisitSegmentDatabase::SetSegmentPresentationIndex(SegmentID segment_id,
+bool VisitSegmentDatabase::SetSegmentPresentationIndex(SegmentID segment_id,
                                                        int index) {
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "UPDATE segments SET pres_index = ? WHERE id = ?"));
-  if (!statement)
-    return;
-
   statement.BindInt(0, index);
   statement.BindInt64(1, segment_id);
-  if (!statement.Run())
-    NOTREACHED();
-  else
-    DCHECK_EQ(1, GetDB().GetLastChangeCount());
+
+  bool success = statement.Run();
+  DCHECK_EQ(1, GetDB().GetLastChangeCount());
+  return success;
 }
 
 bool VisitSegmentDatabase::DeleteSegmentForURL(URLID url_id) {
-  sql::Statement select(GetDB().GetCachedStatement(SQL_FROM_HERE,
-      "SELECT id FROM segments WHERE url_id = ?"));
-  if (!select)
-    return false;
+  sql::Statement delete_seg(GetDB().GetCachedStatement(SQL_FROM_HERE,
+      "DELETE FROM segments WHERE id IN "
+      "(SELECT id FROM segments WHERE url_id = ?)"));

[Scott Hess - ex-Googler, 2012/01/10 22:29:49]

I suspect you actually want to put this one after the other one.  If the tests
pass, it's possible that this isn't being tested!

And this one doesn't need the sub-select.

[Greg Billock, 2012/01/11 23:42:12]

Oy, good point. I see what you're saying. I think I ran the tests. I'll have a
look... Nope, if this is tested, it is way, way up the call stack, not directly
at all.

+  delete_seg.BindInt64(0, url_id);
 
-  sql::Statement delete_seg(GetDB().GetCachedStatement(SQL_FROM_HERE,
-      "DELETE FROM segments WHERE id = ?"));
-  if (!delete_seg)
+  if (!delete_seg.Run())
     return false;
 
   sql::Statement delete_usage(GetDB().GetCachedStatement(SQL_FROM_HERE,
-      "DELETE FROM segment_usage WHERE segment_id = ?"));
-  if (!delete_usage)
-    return false;
+      "DELETE FROM segment_usage WHERE segment_id IN "
+      "(SELECT id FROM segments WHERE url_id = ?)"));
+  delete_usage.BindInt64(0, url_id);
 
-  bool r = true;
-  select.BindInt64(0, url_id);
-  // In theory there could not be more than one segment using that URL but we
-  // loop anyway to cleanup any inconsistency.
-  while (select.Step()) {
-    SegmentID segment_id = select.ColumnInt64(0);
-
-    delete_usage.BindInt64(0, segment_id);
-    if (!delete_usage.Run()) {
-      NOTREACHED();
-      r = false;
-    }
-
-    delete_seg.BindInt64(0, segment_id);
-    if (!delete_seg.Run()) {
-      NOTREACHED();
-      r = false;
-    }
-    delete_usage.Reset();
-    delete_seg.Reset();
-  }
-  return r;
+  return delete_usage.Run();
 }
 
 }  // namespace history