[rAc - libaddressinput] slay a Helper class.

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

Index: third_party/libaddressinput/chromium/cpp/src/retriever.cc
diff --git a/third_party/libaddressinput/chromium/cpp/src/retriever.cc b/third_party/libaddressinput/chromium/cpp/src/retriever.cc
index 3881d83da0e13ec91d7f3506195873055385f50a..dc25af9802a8c4fedaf9214cccab9a59cba0cf39 100644
--- a/third_party/libaddressinput/chromium/cpp/src/retriever.cc
+++ b/third_party/libaddressinput/chromium/cpp/src/retriever.cc
@@ -22,68 +22,15 @@
 
 #include <cassert>
 #include <cstddef>
+#include <map>
 #include <string>
 
 #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 +41,53 @@ 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) {
+  if (requests_.count(key) > 0) {
+    (*retrieved)(false, key, std::string());

[please use gerrit instead, 2014/01/07 02:37:28]

I recommend using (requests_.find(key) != requests_.end()). The find() method
has the benefit of stopping the search after finding the first element. If you
ever change |requests_| to be a data structure that can contain duplicates (e.g.
a vector a multimap), then count() would be checking all elements every time.

nit:

I think assert(requests_.find(key) == requests_.end()); would work better here,
because this condition should never happen. If we let it happen, I foresee weird
bugs where some addresses do not get validated due to user clicking the "submit"
button twice, for example.

[Evan Stade, 2014/01/07 20:23:32]

Done.

+    return;
+  }
+
+  requests_.insert(std::make_pair(key, retrieved.release()));

[please use gerrit instead, 2014/01/07 02:37:28]

nit:

#include <utility> 

(std::make_pair is in the <utility> header.)

[Evan Stade, 2014/01/07 20:23:32]

Done.

+  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