Database usage adjustment for .../history

chrome/browser/history/visitsegment_database.cc

Index: chrome/browser/history/visitsegment_database.cc
diff --git a/chrome/browser/history/visitsegment_database.cc b/chrome/browser/history/visitsegment_database.cc
index c8bbfd35811479e7359bc94b0c55eb6f2781af77..c873b859f5502c99bc0b2fc1cfa5cb59967e1aa4 100644
--- a/chrome/browser/history/visitsegment_database.cc
+++ b/chrome/browser/history/visitsegment_database.cc
@@ -48,12 +48,11 @@ bool VisitSegmentDatabase::InitSegmentTables() {
         "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;
     }
   }
@@ -71,13 +70,11 @@ bool VisitSegmentDatabase::InitSegmentTables() {
         "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;
     }
   }
@@ -144,10 +141,8 @@ 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);
+
   if (statement.Step())
     return statement.ColumnInt64(0);
   return 0;
@@ -157,21 +152,17 @@ 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);
+
   if (statement.Step())
     return statement.ColumnInt64(0);
   return 0;
@@ -181,11 +172,9 @@ 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;

[Scott Hess - ex-Googler, 2012/01/03 23:41:57]

Ick.  It seems unlikely that |false| is the right return value in this case.

[Greg Billock, 2012/01/04 19:20:20]

Yes. Ouch. The .h file says returns 0 on failure, so changing.

On 2012/01/03 23:41:57, shess wrote:

@@ -199,15 +188,16 @@ bool VisitSegmentDatabase::IncreaseSegmentVisitCount(SegmentID segment_id,
   sql::Statement select(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "SELECT id, visit_count FROM segment_usage "
       "WHERE time_slot = ? AND segment_id = ?"));
-  if (!select)
-    return false;
-
   select.BindInt64(0, t.ToInternalValue());
   select.BindInt64(1, segment_id);
+
+  if (!select.is_valid())
+    return false;
+
   if (select.Step()) {
     sql::Statement update(GetDB().GetCachedStatement(SQL_FROM_HERE,
         "UPDATE segment_usage SET visit_count = ? WHERE id = ?"));
-    if (!update)
+    if (!update.is_valid())
       return false;

[Scott Hess - ex-Googler, 2012/01/03 23:41:57]

Can be removed.

[Greg Billock, 2012/01/04 19:20:20]

Done.

 
     update.BindInt64(0, select.ColumnInt64(1) + static_cast<int64>(amount));
@@ -218,7 +208,7 @@ bool VisitSegmentDatabase::IncreaseSegmentVisitCount(SegmentID segment_id,
     sql::Statement insert(GetDB().GetCachedStatement(SQL_FROM_HERE,
         "INSERT INTO segment_usage "
         "(segment_id, time_slot, visit_count) VALUES (?, ?, ?)"));
-    if (!insert)
+    if (!insert.is_valid())
       return false;

[Scott Hess - ex-Googler, 2012/01/03 23:41:57]

Can be removed.

[Greg Billock, 2012/01/04 19:20:20]

Done.

 
     insert.BindInt64(0, segment_id);
@@ -245,10 +235,8 @@ void VisitSegmentDatabase::QuerySegmentUsage(
       "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());
@@ -304,10 +292,10 @@ void VisitSegmentDatabase::QuerySegmentUsage(
       "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());
@@ -319,46 +307,40 @@ void VisitSegmentDatabase::QuerySegmentUsage(
   }
 }
 
-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());
-  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)
+  if (!select.is_valid())
     return false;
 
   sql::Statement delete_seg(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "DELETE FROM segments WHERE id = ?"));
-  if (!delete_seg)
+  if (!delete_seg.is_valid())
     return false;
 
   sql::Statement delete_usage(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "DELETE FROM segment_usage WHERE segment_id = ?"));
-  if (!delete_usage)
+  if (!delete_usage.is_valid())
     return false;

[Scott Hess - ex-Googler, 2012/01/03 23:41:57]

Would you be willing to rewrite this to a pair of simpler statements?  I mean
like:

DELETE FROM segment_usage WHERE segment_id IN (SELECT id FROM segments WHERE
url_id = ?);
  and
DELETE FROM segments WHERE url_id = ?;

Performance-wise, they should be identical, but brittle-code-wise the two
simpler statements should be full of win.  Otherwise maybe just drop a
TODO(shess) in here?

[Greg Billock, 2012/01/04 19:20:20]

Done. Yes, it cleans up real nice. :-)

On 2012/01/03 23:41:57, shess wrote:

 
   bool r = true;
@@ -369,16 +351,13 @@ bool VisitSegmentDatabase::DeleteSegmentForURL(URLID url_id) {
     SegmentID segment_id = select.ColumnInt64(0);
 
     delete_usage.BindInt64(0, segment_id);
-    if (!delete_usage.Run()) {
-      NOTREACHED();
+    if (!delete_usage.Run())
       r = false;
-    }
 
     delete_seg.BindInt64(0, segment_id);
-    if (!delete_seg.Run()) {
-      NOTREACHED();
+    if (!delete_seg.Run())
       r = false;
-    }
+
     delete_usage.Reset();
     delete_seg.Reset();
   }