[rAc - libaddressinput] slay a Helper class.

For better memory ownership.

BUG=none

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

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

third_party/libaddressinput/chromium/cpp/src/retriever.cc

@@ -22,68 +22,16 @@
 
 #include <cassert>
 #include <cstddef>
+#include <map>
 #include <string>
+#include <utility>
 
 #include "lookup_key_util.h"
+#include "util/stl_util.h"
 
 namespace i18n {
 namespace addressinput {
 
-namespace {
-
-class Helper {
- public:
-  // Does not take ownership of |storage|.
-  Helper(const std::string& key,
-         scoped_ptr<Retriever::Callback> retrieved,
-         const LookupKeyUtil& lookup_key_util,
-         const Downloader& downloader,
-         Storage* storage)
-      : retrieved_(retrieved.Pass()),
-        lookup_key_util_(lookup_key_util),
-        downloader_(downloader),
-        storage_(storage) {
-    assert(storage_ != NULL);
-    storage_->Get(key, BuildCallback(this, &Helper::OnDataReady));
-  }
-
- private:
-  ~Helper() {}
-
-  void OnDataReady(bool success,
-                   const std::string& key,
-                   const std::string& data) {
-    if (success) {
-      (*retrieved_)(success, key, data);
-      delete this;
-    } else {
-      downloader_.Download(lookup_key_util_.GetUrlForKey(key),
-                           BuildCallback(this, &Helper::OnDownloaded));
-    }
-  }
-
-  void OnDownloaded(bool success,
-                    const std::string& url,
-                    const std::string& data) {
-    const std::string& key = lookup_key_util_.GetKeyForUrl(url);
-    if (success) {
-      // TODO(estade): this is really dangerous; storage_ is not owned.
-      storage_->Put(key, data);
-    }
-    (*retrieved_)(success, key, success ? data : std::string());
-    delete this;
-  }
-
-  scoped_ptr<Retriever::Callback> retrieved_;
-  const LookupKeyUtil& lookup_key_util_;
-  const Downloader& downloader_;
-  Storage* storage_;
-
-  DISALLOW_COPY_AND_ASSIGN(Helper);
-};
-
-}  // namespace
-
 Retriever::Retriever(const std::string& validation_data_url,
                      scoped_ptr<const Downloader> downloader,
                      scoped_ptr<Storage> storage)
@@ -94,15 +42,49 @@ Retriever::Retriever(const std::string& validation_data_url,
   assert(downloader_ != NULL);
 }
 
-Retriever::~Retriever() {}
+Retriever::~Retriever() {
+  STLDeleteValues(&requests_);
+}
 
 void Retriever::Retrieve(const std::string& key,
-                         scoped_ptr<Callback> retrieved) const {
-  new Helper(key,
-             retrieved.Pass(),
-             lookup_key_util_,
-             *downloader_,
-             storage_.get());
+                         scoped_ptr<Callback> retrieved) {
+  assert(requests_.find(key) == requests_.end());
+  requests_.insert(std::make_pair(key, retrieved.release()));
+  storage_->Get(key,
+                BuildCallback(this, &Retriever::OnDataRetrievedFromStorage));
+}
+
+void Retriever::OnDataRetrievedFromStorage(bool success,
+                                           const std::string& key,
+                                           const std::string& data) {
+  if (success) {
+    scoped_ptr<Callback> retrieved = GetCallbackForKey(key);
+    (*retrieved)(success, key, data);
+  } else {
+    downloader_->Download(lookup_key_util_.GetUrlForKey(key),
+                          BuildCallback(this, &Retriever::OnDownloaded));
+  }
+}
+
+void Retriever::OnDownloaded(bool success,
+                             const std::string& url,
+                             const std::string& data) {
+  const std::string& key = lookup_key_util_.GetKeyForUrl(url);
+  if (success) {
+    storage_->Put(key, data);
+  }
+  scoped_ptr<Callback> retrieved = GetCallbackForKey(key);
+  (*retrieved)(success, key, success ? data : std::string());
+}
+
+scoped_ptr<Retriever::Callback> Retriever::GetCallbackForKey(
+    const std::string& key) {
+  std::map<std::string, Callback*>::iterator iter =
+      requests_.find(key);
+  assert(iter != requests_.end());
+  scoped_ptr<Callback> callback(iter->second);
+  requests_.erase(iter);
+  return callback.Pass();
 }
 
 }  // namespace addressinput

third_party/libaddressinput/chromium/cpp/src/retriever.h

@@ -21,6 +21,7 @@
 #include <libaddressinput/util/basictypes.h>
 #include <libaddressinput/util/scoped_ptr.h>
 
+#include <map>
 #include <string>
 
 #include "lookup_key_util.h"
@@ -50,12 +51,28 @@ class Retriever {
   // Retrieves the data for |key| and invokes the |retrieved| callback. Checks
   // for the data in storage first. If storage does not have the data for |key|,
   // then downloads the data and places it in storage.
-  void Retrieve(const std::string& key, scoped_ptr<Callback> retrieved) const;
+  void Retrieve(const std::string& key, scoped_ptr<Callback> retrieved);
 
  private:
+  // Callback for when a rule is retrieved from |storage_|.
+  void OnDataRetrievedFromStorage(bool success,
+                                  const std::string& key,
+                                  const std::string& data);
+
+  // Callback for when a rule is retrieved by |downloader_|.
+  void OnDownloaded(bool success,
+                    const std::string& url,
+                    const std::string& data);
+
+  // Looks up the callback for |key| in |requests_|, removes it from the map
+  // and returns it. Assumes that |key| is in fact in |requests_|.
+  scoped_ptr<Callback> GetCallbackForKey(const std::string& key);
+
   const LookupKeyUtil lookup_key_util_;
   scoped_ptr<const Downloader> downloader_;
   scoped_ptr<Storage> storage_;
+  // Holds pending requests. The callback pointers are owned.
+  std::map<std::string, Callback*> requests_;
 
   DISALLOW_COPY_AND_ASSIGN(Retriever);
 };

third_party/libaddressinput/chromium/cpp/test/retriever_test.cc

@@ -122,6 +122,32 @@ TEST_F(RetrieverTest, FaultyDownloader) {
   EXPECT_TRUE(data_.empty());
 }
 
+// The downloader that doesn't get back to you.
+class HangingDownloader : public Downloader {
+ public:
+  HangingDownloader() {}
+  virtual ~HangingDownloader() {}
+
+  // Downloader implementation.
+  virtual void Download(const std::string& url,
+                        scoped_ptr<Callback> downloaded) const {}
+};
+
+TEST_F(RetrieverTest, RequestsDontStack) {
+  Retriever slow_retriever(FakeDownloader::kFakeDataUrl,
+                           scoped_ptr<const Downloader>(new HangingDownloader),
+                           scoped_ptr<Storage>(new FakeStorage));
+
+  slow_retriever.Retrieve(kKey, BuildCallback());
+  EXPECT_FALSE(success_);
+  EXPECT_TRUE(key_.empty());
+
+#if !defined(NDEBUG)
+  // This request should cause an assert.
+  ASSERT_DEATH(slow_retriever.Retrieve(kKey, BuildCallback()), "");
+#endif
+}
+
 }  // namespace
 
 }  // namespace addressinput