Translate: language list smart updater

chrome/browser/translate/translate_language_list.cc

Index: chrome/browser/translate/translate_language_list.cc
diff --git a/chrome/browser/translate/translate_language_list.cc b/chrome/browser/translate/translate_language_list.cc
index 597ca46b80d185c2ecfebead8b25b740e8519463..5e4404bb59f5dad5208fdb4ed9e912071ac7f5ae 100644
--- a/chrome/browser/translate/translate_language_list.cc
+++ b/chrome/browser/translate/translate_language_list.cc
@@ -6,6 +6,7 @@
 
 #include <set>
 
+#include "base/bind.h"
 #include "base/json/json_reader.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
@@ -104,8 +105,13 @@ const char kAlphaLanguageQueryName[] = "alpha";
 const char kAlphaLanguageQueryValue[] = "1";
 
 // Retry parameter for fetching supporting language list.
-const int kMaxRetryLanguageListFetch = 5;
+const int kMaxRetryLanguageListFetchOn5xx = 5;
 
+// Retry parameter for LanguageListFetcher.
+const int kMaxRetryLanguageListFetch = 16;
+
+// Assign following IDs to URLFetchers so that tests can distinguish each
+// request in order to simiulate respectively.
 const int kFetcherIdForLanguageList = 1;
 const int kFetcherIdForAlphaLanguageList = 2;
 
@@ -176,33 +182,107 @@ void SetSupportedLanguages(const std::string& language_list,
   NotifyEvent(__LINE__, message);
 }
 
-// Creates URLFetcher, sets arguments to start, and returns the object.
-net::URLFetcher* CreateAndStartFetch(int id,
-                                     const GURL& url,
-                                     net::URLFetcherDelegate* delegate) {
-  DCHECK(delegate);
+// Check if |n| is 2^n (n >= 0).
+bool IsOneHot(int n) {

[MAD, 2013/06/10 18:28:10]

I would prefer to name this: IsPowerOfTwo.

And here's a trick to do it more easily:
http://bits.stephan-brumme.com/isPowerOfTwo.html
http://stackoverflow.com/questions/600293/how-to-check-if-a-number-is-a-power...

(n != 0) && ((n & (n - 1)) == 0)

[Takashi Toyoshima, 2013/06/12 05:20:43]

I'll stop to use exponential backoff here.
Because retry request is not timer-based, but user action or monitoring based.
The retry cycle can be reasonable.
It looks safe just to have a maximum limit.

+  int hot_count = 0;
+  for (int bit = 1; bit != 0; bit <<= 1)
+    if (n & bit)
+      hot_count++;
+  return hot_count == 1;
+}
+
+}  // namespace
+
+TranslateLanguageList::LanguageListFetcher::LanguageListFetcher(
+    bool include_alpha_languages)
+  : include_alpha_languages_(include_alpha_languages),
+    state_(IDLE),
+    retry_count_(0) {

[MAD, 2013/06/10 18:28:10]

You never reset it to 0, so it's not just retries, it's for ALL calls, which I
don't think is what you intend this for, right?

[Takashi Toyoshima, 2013/06/12 05:20:43]

If I reset retry_count whenever a request is issued, interval will be 1, 2, 4,
8, ....
But, also it works without reseting this because (2-1), (4-2), (8-4), (16-8),
... is also 1, 2, 4, 8, ....

+}
+
+TranslateLanguageList::LanguageListFetcher::~LanguageListFetcher() {
+}
+
+bool TranslateLanguageList::LanguageListFetcher::Request(
+    const TranslateLanguageList::LanguageListFetcher::Callback& callback) {
+  // This function is not supporsed to be called before previous operaion is not

[MAD, 2013/06/10 18:28:10]

supporsed -> supposed

[Takashi Toyoshima, 2013/06/12 05:20:43]

Done.

+  // finished.
+  if (state_ == REQUESTING) {
+    NOTREACHED();
+    return false;
+  }
+
+  // Omit requests to realize an exponential backoff'd retries.
+  // Request will be issued when |retry_count_| is 2^n (n >= 0) and it doesn't
+  // reach to limitation.
+  if (retry_count_ >= (1 << kMaxRetryLanguageListFetch)) {
+    NotifyEvent(__LINE__, "Request is omitted due to retry limitation");
+    return false;
+  }
+  if (!IsOneHot(++retry_count_)) {

[MAD, 2013/06/10 18:28:10]

Actually, the URLFetcher already supports backoff delays:
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...

[Takashi Toyoshima, 2013/06/12 05:20:43]

Yes. But it works only against HTTP status 5xx.
This retry covers network offline, flaky network, and so on.

+    NotifyEvent(__LINE__, "Request is omitted due to exponential backoff");
+    return false;
+  }
+
+  state_ = REQUESTING;
+  callback_ = callback;
+
+  GURL url = GURL(kLanguageListFetchURL);
+  url = TranslateURLUtil::AddHostLocaleToUrl(url);
+  url = TranslateURLUtil::AddApiKeyToUrl(url);
+  if (include_alpha_languages_) {
+    url = net::AppendQueryParameter(url,
+                                    kAlphaLanguageQueryName,
+                                    kAlphaLanguageQueryValue);
+  }
+
   std::string message = base::StringPrintf(
       "%s list fetch starts (URL: %s)",
-      (id == kFetcherIdForLanguageList) ? "Language" : "Alpha language",
+      include_alpha_languages_ ? "Language" : "Alpha language",
       url.spec().c_str());
   NotifyEvent(__LINE__, message);
 
-  scoped_ptr<net::URLFetcher> fetcher;
-  fetcher.reset(net::URLFetcher::Create(id,
-                                        url,
-                                        net::URLFetcher::GET,
-                                        delegate));
-  fetcher->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
-                        net::LOAD_DO_NOT_SAVE_COOKIES);
-  fetcher->SetRequestContext(g_browser_process->system_request_context());
-  fetcher->SetMaxRetriesOn5xx(kMaxRetryLanguageListFetch);
-  fetcher->Start();
-
-  return fetcher.release();
+  fetcher_.reset(net::URLFetcher::Create(
+      include_alpha_languages_ ? kFetcherIdForAlphaLanguageList :
+                                 kFetcherIdForLanguageList,
+      url,
+      net::URLFetcher::GET,
+      this));
+  fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
+                         net::LOAD_DO_NOT_SAVE_COOKIES);
+  fetcher_->SetRequestContext(g_browser_process->system_request_context());
+  fetcher_->SetMaxRetriesOn5xx(kMaxRetryLanguageListFetchOn5xx);
+  fetcher_->Start();
+
+  return true;
 }
 
-}  // namespace
+void TranslateLanguageList::LanguageListFetcher::OnURLFetchComplete(
+    const net::URLFetcher* source) {
+  DCHECK(fetcher_.get() == source);
 
+  std::string data;
+  if (source->GetStatus().status() == net::URLRequestStatus::SUCCESS &&
+      source->GetResponseCode() == net::HTTP_OK) {
+    state_ = COMPLETED;
+    source->GetResponseAsString(&data);
+    std::string message = base::StringPrintf(
+        "%s list is updated",
+        include_alpha_languages_ ? "Alpha language" : "Language");
+    NotifyEvent(__LINE__, message);
+  } else {
+    state_ = FAILED;
+    std::string message = base::StringPrintf(
+        "Failed to Fetch languages from: %s",
+        source->GetURL().spec().c_str());
+    NotifyEvent(__LINE__, message);
+  }
+
+  scoped_ptr<const net::URLFetcher> delete_ptr(fetcher_.release());
+  callback_.Run(include_alpha_languages_, state_ == COMPLETED, data);
+}
+
+// Transfer URLFetcher's ownership before invoking a callback.
 // This must be kept in sync with the &cb= value in the kLanguageListFetchURL.
 const char TranslateLanguageList::kLanguageListCallbackName[] = "sl(";
 const char TranslateLanguageList::kTargetLanguagesKey[] = "tl";
@@ -214,42 +294,15 @@ TranslateLanguageList::TranslateLanguageList() {
   for (size_t i = 0; i < arraysize(kDefaultSupportedLanguages); ++i)
     supported_languages_.insert(kDefaultSupportedLanguages[i]);
   UpdateSupportedLanguages();
-}
 
-TranslateLanguageList::~TranslateLanguageList() {}
+  language_list_fetcher_.reset(new LanguageListFetcher(false));
+  alpha_language_list_fetcher_.reset(new LanguageListFetcher(true));
 
-void TranslateLanguageList::OnURLFetchComplete(const net::URLFetcher* source) {
-  scoped_ptr<const net::URLFetcher> delete_ptr;
+  net::NetworkChangeNotifier::AddNetworkChangeObserver(this);
+}
 
-  if (source->GetStatus().status() == net::URLRequestStatus::SUCCESS &&
-      source->GetResponseCode() == net::HTTP_OK) {
-    std::string data;
-    source->GetResponseAsString(&data);
-    if (language_list_fetcher_.get() == source) {
-      NotifyEvent(__LINE__, "Language list is updated");
-      delete_ptr.reset(language_list_fetcher_.release());
-      SetSupportedLanguages(data, &supported_languages_);
-    } else if (alpha_language_list_fetcher_.get() == source) {
-      NotifyEvent(__LINE__, "Alpha language list is updated");
-      delete_ptr.reset(alpha_language_list_fetcher_.release());
-      SetSupportedLanguages(data, &supported_alpha_languages_);
-    } else {
-      NOTREACHED();
-    }
-    UpdateSupportedLanguages();
-  } else {
-    // TODO(toyoshim): Try again. http://crbug.com/244202 .
-    // Also In CrOS, FetchLanguageList is not called at launching Chrome. It
-    // will solve this problem that check if FetchLanguageList is already
-    // called, and call it if needed in InitSupportedLanguage().
-    std::string message = base::StringPrintf(
-        "%s list update fails (Status: %d, URL: %s)",
-        (language_list_fetcher_.get() == source) ? "Language" :
-                                                   "Alpha language",
-        source->GetResponseCode(),
-        source->GetOriginalURL().spec().c_str());
-    NotifyEvent(__LINE__, message);
-  }
+TranslateLanguageList::~TranslateLanguageList() {
+  net::NetworkChangeNotifier::RemoveNetworkChangeObserver(this);
 }
 
 void TranslateLanguageList::GetSupportedLanguages(
@@ -258,6 +311,10 @@ void TranslateLanguageList::GetSupportedLanguages(
   std::set<std::string>::const_iterator iter = all_supported_languages_.begin();
   for (; iter != all_supported_languages_.end(); ++iter)
     languages->push_back(*iter);
+
+  // Update language lists if they are not updated after Chrome was launched
+  // for later requests.
+  RequestLanguageList();
 }
 
 std::string TranslateLanguageList::GetLanguageCode(
@@ -284,31 +341,50 @@ bool TranslateLanguageList::IsAlphaLanguage(const std::string& language) {
 }
 
 void TranslateLanguageList::RequestLanguageList() {
-  if (language_list_fetcher_.get() || alpha_language_list_fetcher_.get())
+  if (language_list_fetcher_.get() &&
+      (language_list_fetcher_->state() == LanguageListFetcher::IDLE ||
+       language_list_fetcher_->state() == LanguageListFetcher::FAILED)) {
+    language_list_fetcher_->Request(
+        base::Bind(&TranslateLanguageList::OnLanguageListFetchComplete,
+                   base::Unretained(this)));
+  }
+
+  if (alpha_language_list_fetcher_.get() &&
+      (alpha_language_list_fetcher_->state() == LanguageListFetcher::IDLE ||
+       alpha_language_list_fetcher_->state() == LanguageListFetcher::FAILED)) {
+    alpha_language_list_fetcher_->Request(
+        base::Bind(&TranslateLanguageList::OnLanguageListFetchComplete,
+                   base::Unretained(this)));
+  }
+}
+
+void TranslateLanguageList::OnNetworkChanged(
+    net::NetworkChangeNotifier::ConnectionType type) {
+  std::string message = base::StringPrintf("OnNetworkChanged: %d, %s", type,
+      net::NetworkChangeNotifier::IsOffline() ? "offline" : "online");
+  NotifyEvent(__LINE__, message);
+
+  if (net::NetworkChangeNotifier::IsOffline())
     return;
 
-  // Fetch the stable language list.
-  GURL language_list_fetch_url = GURL(kLanguageListFetchURL);
-  language_list_fetch_url =
-      TranslateURLUtil::AddHostLocaleToUrl(language_list_fetch_url);
-  language_list_fetch_url =
-      TranslateURLUtil::AddApiKeyToUrl(language_list_fetch_url);
-
-  language_list_fetcher_.reset(
-      CreateAndStartFetch(kFetcherIdForLanguageList,
-                          language_list_fetch_url,
-                          this));
-
-  // Fetch the alpha language list.
-  language_list_fetch_url = net::AppendQueryParameter(
-      language_list_fetch_url,
-      kAlphaLanguageQueryName,
-      kAlphaLanguageQueryValue);
-
-  alpha_language_list_fetcher_.reset(
-      CreateAndStartFetch(kFetcherIdForAlphaLanguageList,
-                          language_list_fetch_url,
-                          this));
+  RequestLanguageList();
+}
+
+void TranslateLanguageList::OnLanguageListFetchComplete(
+    bool include_alpha_languages,
+    bool success,
+    const std::string& data) {
+  if (!success)

[MAD, 2013/06/10 18:28:10]

You don't actually retry on failures, and you don't look at the return value of
LanguageListFetcher::Request(), so your backoff system doesn't really work, and
is not needed, it just prevents the fetch from happening more and more as we get
the languages more and more...

And actually, since you don't check if the network is available (you rely on the
URLFetcher to fail when there's no network?) and you don't have the wait for
EULA yet (it's in your other patch), I don't think you need more than what the
URLFetch already offer as a retry/backoff service.

[Takashi Toyoshima, 2013/06/12 05:20:43]

No. It was invoked in GetSupportedLanguages() if the list is not updated.
This function is called whenever infobar appears.

Intentionally, I prevent to invoke retry here because we fails fetch just now
even if URLFetcher performed internal retry, and there rarely  internal retry,
and there is rarely a chance to success.

Also I invoked retry when network becomes online. See, OnNetworkChanged() of
this change set.
Or OnResourceRequestsAllowed() of the change set 12

+    return;
+
+  if (!include_alpha_languages) {
+    SetSupportedLanguages(data, &supported_languages_);
+    language_list_fetcher_.reset();
+  } else {
+    SetSupportedLanguages(data, &supported_alpha_languages_);
+    alpha_language_list_fetcher_.reset();
+  }
+  UpdateSupportedLanguages();
 }
 
 void TranslateLanguageList::UpdateSupportedLanguages() {