[Top Sites] Encoding redirects field in TopSitesDatabase, and adding validations

chrome/browser/history/top_sites_database.cc

Index: chrome/browser/history/top_sites_database.cc
diff --git a/chrome/browser/history/top_sites_database.cc b/chrome/browser/history/top_sites_database.cc
index ff20d4db8e3756beaeec1e264ac13030bced5bce..386875f8baf8049b395b6ac9694559823364bcf1 100644
--- a/chrome/browser/history/top_sites_database.cc
+++ b/chrome/browser/history/top_sites_database.cc
@@ -78,22 +78,6 @@ bool InitTables(sql::Connection* db) {
   return db->Execute(kThumbnailsSql);
 }
 
-// Encodes redirects into a string.
-std::string GetRedirects(const history::MostVisitedURL& url) {
-  std::vector<std::string> redirects;
-  for (size_t i = 0; i < url.redirects.size(); i++)
-    redirects.push_back(url.redirects[i].spec());
-  return JoinString(redirects, ' ');
-}
-
-// Decodes redirects from a string and sets them for the url.
-void SetRedirects(const std::string& redirects, history::MostVisitedURL* url) {
-  std::vector<std::string> redirects_vector;
-  base::SplitStringAlongWhitespace(redirects, &redirects_vector);
-  for (size_t i = 0; i < redirects_vector.size(); ++i)
-    url->redirects.push_back(GURL(redirects_vector[i]));
-}
-
 // Track various failure (and success) cases in recovery code.
 //
 // TODO(shess): The recovery code is complete, but by nature runs in challenging
@@ -132,6 +116,8 @@ enum RecoveryEventType {
   RECOVERY_EVENT_MAX,
 };
 
+const char kDataUrlPrefix[] = "data:";
+
 void RecordRecoveryEvent(RecoveryEventType recovery_event) {
   UMA_HISTOGRAM_ENUMERATION("History.TopSitesRecovery",
                             recovery_event, RECOVERY_EVENT_MAX);
@@ -460,8 +446,8 @@ void TopSitesDatabase::GetPageThumbnails(MostVisitedURLList* urls,
     url.title = statement.ColumnString16(2);
     url.last_forced_time =
         base::Time::FromInternalValue(statement.ColumnInt64(10));
-    std::string redirects = statement.ColumnString(4);
-    SetRedirects(redirects, &url);
+    std::string encoded_redirects = statement.ColumnString(4);
+    DecodeRedirects(encoded_redirects, &url.redirects);
     urls->push_back(url);
 
     std::vector<unsigned char> data;
@@ -510,7 +496,7 @@ bool TopSitesDatabase::UpdatePageThumbnail(
     statement.BindBlob(1, thumbnail.thumbnail->front(),
                        static_cast<int>(thumbnail.thumbnail->size()));
   }
-  statement.BindString(2, GetRedirects(url));
+  statement.BindString(2, EncodeRedirects(url.redirects));
   const ThumbnailScore& score = thumbnail.thumbnail_score;
   statement.BindDouble(3, score.boring_score);
   statement.BindBool(4, score.good_clipping);
@@ -539,7 +525,7 @@ void TopSitesDatabase::AddPageThumbnail(const MostVisitedURL& url,
     statement.BindBlob(3, thumbnail.thumbnail->front(),
                        static_cast<int>(thumbnail.thumbnail->size()));
   }
-  statement.BindString(4, GetRedirects(url));
+  statement.BindString(4, EncodeRedirects(url.redirects));
   const ThumbnailScore& score = thumbnail.thumbnail_score;
   statement.BindDouble(5, score.boring_score);
   statement.BindBool(6, score.good_clipping);
@@ -559,6 +545,102 @@ void TopSitesDatabase::AddPageThumbnail(const MostVisitedURL& url,
     UpdatePageRankNoTransaction(url, new_rank);
 }
 
+// static
+std::string TopSitesDatabase::EncodeCSVString(
+    const std::vector<std::string> str_list) {
+  std::string csv;
+  for (std::vector<std::string>::const_iterator it = str_list.begin();
+       it != str_list.end(); ++it) {
+    const std::string& str = *it;
+    if (it != str_list.begin())
+      csv += ',';
+    csv += '"';
+    for (std::string::const_iterator jt = str.begin(); jt != str.end(); ++jt) {
+      if (*jt == '"')
+        csv += '"';

[beaudoin, 2014/09/09 21:32:27]

Looks like you escape quotes as two quotes. Your comment should be clarified.

[huangs, 2014/09/11 01:58:13]

Comment in .h file updated.

+      csv += *jt;
+    }
+    csv += '"';
+  }
+  return csv;
+}
+
+// static
+bool TopSitesDatabase::DecodeCSVString(const std::string csv,
+                                       std::vector<std::string>* str_list) {
+  if (csv.empty()) {
+    str_list->clear();
+    return true;
+  }
+
+  enum {
+    SEEK_QUOTE,
+    READ_STRING,
+    SAW_ONE_QUOTE
+  } state = SEEK_QUOTE;
+  std::vector<std::string> out_list;
+  std::string str;
+  for (std::string::const_iterator it = csv.begin(); it != csv.end(); ++it) {
+    const char ch = *it;
+    if (state == SEEK_QUOTE) {
+      if (ch != '"')
+        return false;
+      state = READ_STRING;
+    } else if (state == READ_STRING) {
+      if (ch == '"')
+        state = SAW_ONE_QUOTE;
+      else
+        str += ch;
+    } else if (state == SAW_ONE_QUOTE) {
+      if (ch == '"') {
+        str += ch;
+        state = READ_STRING;
+      } else if (ch == ',') {
+        out_list.push_back(str);
+        str.clear();
+        state = SEEK_QUOTE;
+      } else {
+        return false;

[beaudoin, 2014/09/09 21:32:27]

You're strict about the fact that the comma must immediately follow the closing
quote without spaces. You should make that clear.

[huangs, 2014/09/11 01:58:13]

Done.

+      }
+    } else {
+      NOTREACHED();
+      return false;
+    }
+  }
+  if (state != SAW_ONE_QUOTE)
+    return false;
+  out_list.push_back(str);
+  str_list->swap(out_list);
+  return true;
+}
+
+// static
+std::string TopSitesDatabase::EncodeRedirects(const RedirectList& redirects) {
+  std::vector<std::string> valid_urls;
+  for (size_t i = 0; i < redirects.size(); i++) {
+    // Example of invalid URL that may end up here:
+    // "data:text/plain,this string contains space".
+    if (redirects[i].is_valid())
+      valid_urls.push_back(redirects[i].spec());
+  }
+  return EncodeCSVString(valid_urls);
+}
+
+// static
+void TopSitesDatabase::DecodeRedirects(const std::string& encoded_redirects,
+                                       RedirectList* redirects) {
+  std::vector<std::string> redirects_vector;
+  if (!DecodeCSVString(encoded_redirects, &redirects_vector)) {
+    // Fall back to space-delimited list for backward compatibility.
+    base::SplitStringAlongWhitespace(encoded_redirects, &redirects_vector);

[beaudoin, 2014/09/09 21:32:27]

I see, so you support DB in the old format too. It's a bit fishy. I'd rather you
bumped the version number and rejected badly formatted entries.

[huangs, 2014/09/11 01:58:13]

Changing version, but have to maintain backward compatibility for unit tests...

+  }
+  for (size_t i = 0; i < redirects_vector.size(); ++i) {
+    GURL redirect_url(redirects_vector[i]);
+    if (redirect_url.is_valid())
+      redirects->push_back(redirect_url);
+  }
+}
+
 void TopSitesDatabase::UpdatePageRank(const MostVisitedURL& url,
                                       int new_rank) {
   DCHECK((url.last_forced_time.ToInternalValue() == 0) ==