libaddressinput: less copying of rules data

- Change Downloader interface to pass scoped_ptr
- Pass ownership of string through Timestamp/Checksum wrapping and unwrapping (also make it append instead of prepend)

BUG=337679

Review URL: https://codereview.chromium.org/145553010

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@246969 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 88 insertions, 91 deletions

diff --git a/third_party/libaddressinput/chromium/chrome_downloader_impl.cc b/third_party/libaddressinput/chromium/chrome_downloader_impl.cc
index 49d2437..4e6c587 100644
--- a/third_party/libaddressinput/chromium/chrome_downloader_impl.cc
+++ b/third_party/libaddressinput/chromium/chrome_downloader_impl.cc
@@ -37,10 +37,10 @@ void ChromeDownloaderImpl::OnURLFetchComplete(const net::URLFetcher* source) {
   DCHECK(request != requests_.end());
 
   bool ok = source->GetResponseCode() == net::HTTP_OK;
-  std::string data;
+  scoped_ptr<std::string> data(new std::string());
   if (ok)
-    source->GetResponseAsString(&data);
-  (*request->second->callback)(ok, request->second->url, data);
+    source->GetResponseAsString(&*data);
+  (*request->second->callback)(ok, request->second->url, data.Pass());
 
   delete request->first;
   delete request->second;
diff --git a/third_party/libaddressinput/chromium/chrome_downloader_impl_unittest.cc b/third_party/libaddressinput/chromium/chrome_downloader_impl_unittest.cc
index 44e3729..3d1cea1 100644
--- a/third_party/libaddressinput/chromium/chrome_downloader_impl_unittest.cc
+++ b/third_party/libaddressinput/chromium/chrome_downloader_impl_unittest.cc
@@ -36,27 +36,27 @@ class ChromeDownloaderImplTest : public testing::Test {
     base::MessageLoop::current()->RunUntilIdle();
   }
 
-  const std::string& data() { return data_; }
+  const std::string& data() { return *data_; }
   bool success() { return success_; }
 
  private:
   scoped_ptr<ChromeDownloaderImpl::Callback> BuildCallback() {
-    return ::i18n::addressinput::BuildCallback(
+    return ::i18n::addressinput::BuildScopedPtrCallback(
         this, &ChromeDownloaderImplTest::OnDownloaded);
   }
 
   // Callback for when download is finished.
   void OnDownloaded(bool success,
                     const std::string& url,
-                    const std::string& data) {
+                    scoped_ptr<std::string> data) {
     success_ = success;
-    data_ = data;
+    data_ = data.Pass();
   }
 
   base::MessageLoop loop_;
   net::URLFetcherImplFactory factory_;
   net::FakeURLFetcherFactory fake_factory_;
-  std::string data_;
+  scoped_ptr<std::string> data_;
   bool success_;
 };
 
diff --git a/third_party/libaddressinput/chromium/cpp/include/libaddressinput/downloader.h b/third_party/libaddressinput/chromium/cpp/include/libaddressinput/downloader.h
index c77ede0..55f1e68 100644
--- a/third_party/libaddressinput/chromium/cpp/include/libaddressinput/downloader.h
+++ b/third_party/libaddressinput/chromium/cpp/include/libaddressinput/downloader.h
@@ -38,7 +38,8 @@ namespace addressinput {
 //    };
 class Downloader {
  public:
-  typedef i18n::addressinput::Callback<std::string, std::string> Callback;
+  typedef i18n::addressinput::ScopedPtrCallback<std::string, std::string>
+      Callback;
 
   virtual ~Downloader() {}
 
diff --git a/third_party/libaddressinput/chromium/cpp/src/retriever.cc b/third_party/libaddressinput/chromium/cpp/src/retriever.cc
index 8d18f97..7e0da10 100644
--- a/third_party/libaddressinput/chromium/cpp/src/retriever.cc
+++ b/third_party/libaddressinput/chromium/cpp/src/retriever.cc
@@ -45,22 +45,20 @@ namespace {
 //    60 seconds per minute.
 static const double kStaleDataAgeInSeconds = 30.0 * 24.0 * 60.0 * 60.0;
 
-// The prefix for the timestamp line in the header.
+// The prefix for the timestamp line in the footer.
 const char kTimestampPrefix[] = "timestamp=";
-const size_t kTimestampPrefixLength = sizeof kTimestampPrefix - 1;
 
-// The prefix for the checksum line in the header.
+// The prefix for the checksum line in the footer.
 const char kChecksumPrefix[] = "checksum=";
-const size_t kChecksumPrefixLength = sizeof kChecksumPrefix - 1;
 
-// The separator between lines of header and data.
+// The separator between lines of footer and data.
 const char kSeparator = '\n';
 
 // Returns |data| with attached checksum and current timestamp. Format:
 //
-//    timestamp=<timestamp>
-//    checksum=<checksum>
 //    <data>
+//    checksum=<checksum>
+//    timestamp=<timestamp>
 //
 // The timestamp is the time_t that was returned from time(NULL) function. The
 // timestamp does not need to be portable because it is written and read only by
@@ -69,66 +67,65 @@ const char kSeparator = '\n';
 //
 // The checksum is the 32-character hexadecimal MD5 checksum of <data>. It is
 // meant to protect from random file changes on disk.
-std::string PrependTimestamp(const std::string& data) {
-  std::string wrapped;
-  wrapped.append(kTimestampPrefix, kTimestampPrefixLength);
-  wrapped.append(TimeToString(time(NULL)));
-  wrapped.push_back(kSeparator);
-
-  wrapped.append(kChecksumPrefix, kChecksumPrefixLength);
-  wrapped.append(MD5String(data));
-  wrapped.push_back(kSeparator);
-  wrapped.append(data);
-
-  return wrapped;
+void AppendTimestamp(std::string* data) {
+  std::string md5 = MD5String(*data);
+
+  data->push_back(kSeparator);
+  data->append(kChecksumPrefix);
+  data->append(md5);
+
+  data->push_back(kSeparator);
+  data->append(kTimestampPrefix);
+  data->append(TimeToString(time(NULL)));
 }
 
-// Places the header value into |header_value| parameter and the rest of the
-// data into |data| parameter. Returns |true| if the header format is valid.
-bool ExtractHeader(const std::string& header_and_data,
-                   const char* header_prefix,
-                   size_t header_prefix_length,
-                   std::string* header_value,
-                   std::string* data) {
-  assert(header_prefix != NULL);
-  assert(header_value != NULL);
+// Places the footer value into |footer_value| parameter and the rest of the
+// data into |data| parameter. Returns |true| if the footer format is valid.
+bool ExtractFooter(scoped_ptr<std::string> data_and_footer,
+                   const std::string& footer_prefix,
+                   std::string* footer_value,
+                   scoped_ptr<std::string>* data) {
+  assert(footer_value != NULL);
   assert(data != NULL);
 
-  if (header_and_data.compare(
-          0, header_prefix_length, header_prefix, header_prefix_length) != 0) {
+  std::string::size_type separator_position =
+      data_and_footer->rfind(kSeparator);
+  if (separator_position == std::string::npos) {
     return false;
   }
 
-  std::string::size_type separator_position =
-      header_and_data.find(kSeparator, header_prefix_length);
-  if (separator_position == std::string::npos) {
+  std::string::size_type footer_start = separator_position + 1;
+  if (data_and_footer->compare(footer_start,
+                               footer_prefix.length(),
+                               footer_prefix) != 0) {
     return false;
   }
 
-  data->assign(header_and_data, separator_position + 1, std::string::npos);
-  header_value->assign(header_and_data, header_prefix_length,
-                       separator_position - header_prefix_length);
+  *footer_value =
+      data_and_footer->substr(footer_start + footer_prefix.length());
+  *data = data_and_footer.Pass();
+  (*data)->resize(separator_position);
   return true;
 }
 
-// Strips out the timestamp and checksum from |header_and_data|. Validates the
-// checksum. Saves the header-less data into |data|. Compares the parsed
+// Strips out the timestamp and checksum from |data_and_footer|. Validates the
+// checksum. Saves the footer-less data into |data|. Compares the parsed
 // timestamp with current time and saves the difference into |age_in_seconds|.
 //
-// The parameters should not be NULL. Does not take ownership of its parameters.
+// The parameters should not be NULL.
 //
-// Returns |true| if |header_and_data| is correctly formatted and has the
+// Returns |true| if |data_and_footer| is correctly formatted and has the
 // correct checksum.
-bool VerifyAndExtractTimestamp(const std::string& header_and_data,
-                               std::string* data,
+bool VerifyAndExtractTimestamp(const std::string& data_and_footer,
+                               scoped_ptr<std::string>* data,
                                double* age_in_seconds) {
   assert(data != NULL);
   assert(age_in_seconds != NULL);
 
   std::string timestamp_string;
-  std::string checksum_and_data;
-  if (!ExtractHeader(header_and_data, kTimestampPrefix, kTimestampPrefixLength,
-                     &timestamp_string, &checksum_and_data)) {
+  scoped_ptr<std::string> checksum_and_data;
+  if (!ExtractFooter(make_scoped_ptr(new std::string(data_and_footer)),
+                     kTimestampPrefix, &timestamp_string, &checksum_and_data)) {
     return false;
   }
 
@@ -143,12 +140,12 @@ bool VerifyAndExtractTimestamp(const std::string& header_and_data,
   }
 
   std::string checksum;
-  if (!ExtractHeader(checksum_and_data, kChecksumPrefix, kChecksumPrefixLength,
-                     &checksum, data)) {
+  if (!ExtractFooter(checksum_and_data.Pass(),
+                     kChecksumPrefix, &checksum, data)) {
     return false;
   }
 
-  return checksum == MD5String(*data);
+  return checksum == MD5String(**data);
 }
 
 }  // namespace
@@ -189,31 +186,33 @@ void Retriever::OnDataRetrievedFromStorage(bool success,
                                            const std::string& key,
                                            const std::string& stored_data) {
   if (success) {
-    std::string unwrapped;
+    scoped_ptr<std::string> unwrapped;
     double age_in_seconds = 0.0;
     if (VerifyAndExtractTimestamp(stored_data, &unwrapped, &age_in_seconds)) {
       if (age_in_seconds < kStaleDataAgeInSeconds) {
-        InvokeCallbackForKey(key, success, unwrapped);
+        InvokeCallbackForKey(key, success, *unwrapped);
         return;
       }
-      stale_data_[key] = unwrapped;
+      stale_data_[key].swap(*unwrapped);
     }
   }
 
   downloader_->Download(GetUrlForKey(key),
-                        BuildCallback(this, &Retriever::OnDownloaded));
+                        BuildScopedPtrCallback(this, &Retriever::OnDownloaded));
 }
 
 void Retriever::OnDownloaded(bool success,
                              const std::string& url,
-                             const std::string& downloaded_data) {
+                             scoped_ptr<std::string> downloaded_data) {
   const std::string& key = GetKeyForUrl(url);
   std::map<std::string, std::string>::iterator stale_data_it =
       stale_data_.find(key);
 
   if (success) {
-    storage_->Put(key, PrependTimestamp(downloaded_data));
-    InvokeCallbackForKey(key, success, downloaded_data);
+    InvokeCallbackForKey(key, success, *downloaded_data);
+    AppendTimestamp(downloaded_data.get());
+    // TODO(estade): storage should take a scoped_ptr.
+    storage_->Put(key, *downloaded_data);
   } else if (stale_data_it != stale_data_.end()) {
     InvokeCallbackForKey(key, true, stale_data_it->second);
   } else {
diff --git a/third_party/libaddressinput/chromium/cpp/src/retriever.h b/third_party/libaddressinput/chromium/cpp/src/retriever.h
index 4413f05..469f66b 100644
--- a/third_party/libaddressinput/chromium/cpp/src/retriever.h
+++ b/third_party/libaddressinput/chromium/cpp/src/retriever.h
@@ -62,7 +62,7 @@ class Retriever {
   // Callback for when a rule is retrieved by |downloader_|.
   void OnDownloaded(bool success,
                     const std::string& url,
-                    const std::string& downloaded_data);
+                    scoped_ptr<std::string> downloaded_data);
 
   // Returns the URL where the |key| can be retrieved. For example, returns
   // "https://i18napis.appspot.com/ssl-aggregate-address/data/US" for input
diff --git a/third_party/libaddressinput/chromium/cpp/src/time_to_string.h b/third_party/libaddressinput/chromium/cpp/src/time_to_string.h
index d13d04c..ccd7ad9 100644
--- a/third_party/libaddressinput/chromium/cpp/src/time_to_string.h
+++ b/third_party/libaddressinput/chromium/cpp/src/time_to_string.h
@@ -15,8 +15,8 @@
 #ifndef I18N_ADDRESSINPUT_TIME_TO_STRING_H_
 #define I18N_ADDRESSINPUT_TIME_TO_STRING_H_
 
-#include <string>
 #include <ctime>
+#include <string>
 
 namespace i18n {
 namespace addressinput {
diff --git a/third_party/libaddressinput/chromium/cpp/test/fake_downloader.cc b/third_party/libaddressinput/chromium/cpp/test/fake_downloader.cc
index c48c619..a58679a 100644
--- a/third_party/libaddressinput/chromium/cpp/test/fake_downloader.cc
+++ b/third_party/libaddressinput/chromium/cpp/test/fake_downloader.cc
@@ -109,7 +109,7 @@ void FakeDownloader::Download(const std::string& url,
     success = true;
     data = "{}";
   }
-  (*downloaded)(success, url, data);
+  (*downloaded)(success, url, make_scoped_ptr(new std::string(data)));
 }
 
 }  // namespace addressinput
diff --git a/third_party/libaddressinput/chromium/cpp/test/fake_downloader_test.cc b/third_party/libaddressinput/chromium/cpp/test/fake_downloader_test.cc
index f397490..f9dd0ce 100644
--- a/third_party/libaddressinput/chromium/cpp/test/fake_downloader_test.cc
+++ b/third_party/libaddressinput/chromium/cpp/test/fake_downloader_test.cc
@@ -36,22 +36,22 @@ class FakeDownloaderTest : public testing::TestWithParam<std::string> {
   virtual ~FakeDownloaderTest() {}
 
   scoped_ptr<Downloader::Callback> BuildCallback() {
-    return ::i18n::addressinput::BuildCallback(
+    return ::i18n::addressinput::BuildScopedPtrCallback(
         this, &FakeDownloaderTest::OnDownloaded);
   }
 
   FakeDownloader downloader_;
   bool success_;
   std::string url_;
-  std::string data_;
+  scoped_ptr<std::string> data_;
 
  private:
   void OnDownloaded(bool success,
                     const std::string& url,
-                    const std::string& data) {
+                    scoped_ptr<std::string> data) {
     success_ = success;
     url_ = url;
-    data_ = data;
+    data_ = data.Pass();
   }
 };
 
@@ -91,7 +91,7 @@ TEST_P(FakeDownloaderTest, FakeDownloaderHasValidDataForRegion) {
 
   EXPECT_TRUE(success_);
   EXPECT_EQ(url, url_);
-  EXPECT_TRUE(DataIsValid(data_, key));
+  EXPECT_TRUE(DataIsValid(*data_, key));
 };
 
 // Test all region codes.
@@ -107,7 +107,7 @@ TEST_F(FakeDownloaderTest, DownloadMissingKeyReturnsEmptyDictionary) {
 
   EXPECT_TRUE(success_);
   EXPECT_EQ(kJunkUrl, url_);
-  EXPECT_EQ("{}", data_);
+  EXPECT_EQ("{}", *data_);
 }
 
 // Verifies that downloading an empty key will return "{}".
@@ -117,7 +117,7 @@ TEST_F(FakeDownloaderTest, DownloadEmptyKeyReturnsEmptyDictionary) {
 
   EXPECT_TRUE(success_);
   EXPECT_EQ(kPrefixOnlyUrl, url_);
-  EXPECT_EQ("{}", data_);
+  EXPECT_EQ("{}", *data_);
 }
 
 // Verifies that downloading a real URL fails.
@@ -127,7 +127,7 @@ TEST_F(FakeDownloaderTest, DownloadRealUrlFals) {
 
   EXPECT_FALSE(success_);
   EXPECT_EQ(kRealUrl, url_);
-  EXPECT_TRUE(data_.empty());
+  EXPECT_TRUE(data_->empty());
 }
 
 }  // namespace
diff --git a/third_party/libaddressinput/chromium/cpp/test/retriever_test.cc b/third_party/libaddressinput/chromium/cpp/test/retriever_test.cc
index 1fbb563..f1ba089 100644
--- a/third_party/libaddressinput/chromium/cpp/test/retriever_test.cc
+++ b/third_party/libaddressinput/chromium/cpp/test/retriever_test.cc
@@ -44,6 +44,12 @@ const char kEmptyData[] = "{}";
 // integrity.
 const char kEmptyDataChecksum[] = "99914b932bd37a50b983c5e7c90ae93b";
 
+std::string Wrap(const std::string& data,
+                 const std::string& checksum,
+                 const std::string& timestamp) {
+  return data + "\n" + "checksum=" + checksum + "\n" + "timestamp=" + timestamp;
+}
+
 }  // namespace
 
 // Tests for Retriever object.
@@ -135,7 +141,7 @@ class FaultyDownloader : public Downloader {
   // Downloader implementation.
   virtual void Download(const std::string& url,
                         scoped_ptr<Callback> downloaded) {
-    (*downloaded)(false, url, "garbage");
+    (*downloaded)(false, url, make_scoped_ptr(new std::string("garbage")));
   }
 };
 
@@ -174,9 +180,7 @@ TEST_F(RetrieverTest, NoChecksumAndTimestampWillRedownload) {
 
 TEST_F(RetrieverTest, ChecksumAndTimestampWillNotRedownload) {
   storage_->Put(kKey,
-                "timestamp=" + TimeToString(time(NULL)) + "\n" +
-                    "checksum=" + kEmptyDataChecksum + "\n" +
-                    kEmptyData);
+                Wrap(kEmptyData, kEmptyDataChecksum, TimeToString(time(NULL))));
   retriever_->Retrieve(kKey, BuildCallback());
   EXPECT_TRUE(success_);
   EXPECT_EQ(kKey, key_);
@@ -184,10 +188,7 @@ TEST_F(RetrieverTest, ChecksumAndTimestampWillNotRedownload) {
 }
 
 TEST_F(RetrieverTest, OldTimestampWillRedownload) {
-  storage_->Put(kKey,
-                std::string("timestamp=0\n") +
-                    "checksum=" + kEmptyDataChecksum + "\n" +
-                    kEmptyData);
+  storage_->Put(kKey, Wrap(kEmptyData, kEmptyDataChecksum, "0"));
   retriever_->Retrieve(kKey, BuildCallback());
   EXPECT_TRUE(success_);
   EXPECT_EQ(kKey, key_);
@@ -200,10 +201,7 @@ TEST_F(RetrieverTest, OldTimestampOkIfDownloadFails) {
   Retriever bad_retriever(FakeDownloader::kFakeDataUrl,
                           scoped_ptr<Downloader>(new FaultyDownloader),
                           scoped_ptr<Storage>(storage_));
-  storage_->Put(kKey,
-                std::string("timestamp=0\n") +
-                    "checksum=" + kEmptyDataChecksum + "\n" +
-                    kEmptyData);
+  storage_->Put(kKey, Wrap(kEmptyData, kEmptyDataChecksum, "0"));
   bad_retriever.Retrieve(kKey, BuildCallback());
   EXPECT_TRUE(success_);
   EXPECT_EQ(kKey, key_);
@@ -212,10 +210,9 @@ TEST_F(RetrieverTest, OldTimestampOkIfDownloadFails) {
 
 TEST_F(RetrieverTest, WrongChecksumWillRedownload) {
   static const char kNonEmptyData[] = "{\"non-empty\": \"data\"}";
-  storage_->Put(kKey,
-                "timestamp=" + TimeToString(time(NULL)) + "\n" +
-                    "checksum=" + kEmptyDataChecksum + "\n" +
-                    kNonEmptyData);
+  storage_->Put(
+      kKey,
+      Wrap(kNonEmptyData, kEmptyDataChecksum, TimeToString(time(NULL))));
   retriever_->Retrieve(kKey, BuildCallback());
   EXPECT_TRUE(success_);
   EXPECT_EQ(kKey, key_);