Translate: language list smart updater

chrome/browser/translate/translate_language_list.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/translate/translate_language_list.h"
 
 #include <set>
 
+#include "base/bind.h"
 #include "base/json/json_reader.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/translate/translate_event_details.h"
 #include "chrome/browser/translate/translate_manager.h"
 #include "chrome/browser/translate/translate_url_util.h"
@@ skipping 78 matching lines @@
 // Constant URL string to fetch server supporting language list.
 const char kLanguageListFetchURL[] =
     "https://translate.googleapis.com/translate_a/l?client=chrome&cb=sl";
 
 // Used in kTranslateScriptURL to request supporting languages list including
 // "alpha languages".
 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;
 
 // Show a message in chrome:://translate-internals Event Logs.
 void NotifyEvent(int line, const std::string& message) {
   TranslateManager* manager = TranslateManager::GetInstance();
   DCHECK(manager);
 
   TranslateEventDetails details(__FILE__, line, message);
   manager->NotifyTranslateEvent(details);
@@ skipping 50 matching lines @@
     // TODO(toyoshim): Check if UI libraries support adding locale.
     set->insert(iter.key());
     if (message.empty())
       message += iter.key();
     else
       message += ", " + iter.key();
   }
   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);
-  std::string message = base::StringPrintf(
-      "%s list fetch starts (URL: %s)",
-      (id == kFetcherIdForLanguageList) ? "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();
+// 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)",
+      include_alpha_languages_ ? "Language" : "Alpha language",
+      url.spec().c_str());
+  NotifyEvent(__LINE__, message);
+
+  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;
+}
+
+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";
 
 TranslateLanguageList::TranslateLanguageList() {
   // We default to our hard coded list of languages in
   // |kDefaultSupportedLanguages|. This list will be overriden by a server
   // providing supported langauges list.
   for (size_t i = 0; i < arraysize(kDefaultSupportedLanguages); ++i)
     supported_languages_.insert(kDefaultSupportedLanguages[i]);
   UpdateSupportedLanguages();
+
+  language_list_fetcher_.reset(new LanguageListFetcher(false));
+  alpha_language_list_fetcher_.reset(new LanguageListFetcher(true));
+
+  net::NetworkChangeNotifier::AddNetworkChangeObserver(this);
 }
 
-TranslateLanguageList::~TranslateLanguageList() {}
-
-void TranslateLanguageList::OnURLFetchComplete(const net::URLFetcher* source) {
-  scoped_ptr<const net::URLFetcher> delete_ptr;
-
-  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(
     std::vector<std::string>* languages) {
   DCHECK(languages && languages->empty());
   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(
     const std::string& chrome_locale) {
   // Only remove the country code for country specific languages we don't
   // support specifically yet.
   if (IsSupportedLanguage(chrome_locale))
     return chrome_locale;
 
   size_t hypen_index = chrome_locale.find('-');
   if (hypen_index == std::string::npos)
     return chrome_locale;
   return chrome_locale.substr(0, hypen_index);
 }
 
 bool TranslateLanguageList::IsSupportedLanguage(const std::string& language) {
   return all_supported_languages_.count(language) != 0;
 }
 
 bool TranslateLanguageList::IsAlphaLanguage(const std::string& language) {
   // |language| should exist only in the alpha language list.
   return supported_alpha_languages_.count(language) != 0 &&
          supported_languages_.count(language) == 0;
 }
 
 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);
+  RequestLanguageList();
+}
 
-  language_list_fetcher_.reset(
-      CreateAndStartFetch(kFetcherIdForLanguageList,
-                          language_list_fetch_url,
-                          this));
+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;
 
-  // 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));
+  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() {
   all_supported_languages_.clear();
   std::set<std::string>::const_iterator iter;
   for (iter = supported_languages_.begin();
       iter != supported_languages_.end();
       ++iter)
     all_supported_languages_.insert(*iter);
   for (iter = supported_alpha_languages_.begin();
       iter != supported_alpha_languages_.end();
       ++iter)
     all_supported_languages_.insert(*iter);
 }