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 8a42e01c8e7aadf6954e947c8003d51afe5b5e3a..e7f9ecf84f861d350e8d332ee5ca3dcb486bab76 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"
@@ -155,75 +156,104 @@ void SetSupportedLanguages(const std::string& language_list,
   }
 }
 
-net::URLFetcher* CreateAndStartFetch(int id,
-                                     const GURL& url,
-                                     net::URLFetcherDelegate* delegate) {
-  DCHECK(delegate);
-  VLOG(9) << "Fetch supporting language list from: " << url.spec().c_str();
+}  // namespace
 
-  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();
+TranslateLanguageList::LanguageListFetcher::LanguageListFetcher(
+    bool include_alpha_languages)
+  : include_alpha_languages_(include_alpha_languages),
+    state_(IDLE) {
 }
 
-}  // namespace
+TranslateLanguageList::LanguageListFetcher::~LanguageListFetcher() {
+}
 
-// This must be kept in sync with the &cb= value in the kLanguageListFetchURL.
-const char TranslateLanguageList::kLanguageListCallbackName[] = "sl(";
-const char TranslateLanguageList::kTargetLanguagesKey[] = "tl";
+void TranslateLanguageList::LanguageListFetcher::Request(
+    const TranslateLanguageList::LanguageListFetcher::Callback& callback) {
+  if (state_ == REQUESTING) {
+    callback.Run(include_alpha_languages_, false, std::string());

[MAD, 2013/06/07 13:29:24]

If callbacks are comparable, shouldn't we simply DCHECK that callback ==
callback_?

[Takashi Toyoshima, 2013/06/07 14:42:55]

(state_ == REQUESTING) means another Request() is called before previous
Request() is not finished.
This case, callback_ holds a callback for the first Request(), so callback !=
callback_ may happen.

Here, I just abort the second request by invoking callback, and allow the first
Request() to continue to perform.

[MAD, 2013/06/07 15:02:15]

As I mentioned below, if callback == callback_ when they have the same
method/this pointers, then I think it's better not to call with false in that
case. Only callback with false success when callback are not the same.

[Takashi Toyoshima, 2013/06/10 13:36:57]

Ah, now I understand what you concerned about.

I can add a check as DCHECK(!callback_.Equals(callback)), but now I feel it must
return false synchronously here. As you mentioned below, asynchronous errors
look a little difficult to understand what happens.

+    return;
+  }
 
-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();
+  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);
+  }
+
+  VLOG(9) << "Fetch supporting language list from: " << url.spec().c_str();

[Takashi Toyoshima, 2013/06/07 05:17:17]

FYI, VLOG(9) will be replace with translate-internals Event Logs by another
prior CL. Rebase will merge them.

[MAD, 2013/06/07 15:02:15]

OK.

+
+  fetcher_.reset(net::URLFetcher::Create(include_alpha_languages_ ? 2 : 1,

[MAD, 2013/06/07 13:29:24]

I didn't notice in previous version that we were using IDs, please add a comment
to explain why.

[Takashi Toyoshima, 2013/06/07 14:42:55]

In another change, I defined const int for these 1 and 2.
URLFetcher class also explains that this ID is aimed for testing.
These IDs are used in testing to distinguish each Fetch for simulating results.
I'll add comment around const int definitions.

[MAD, 2013/06/07 15:02:15]

OK thanks.

[Takashi Toyoshima, 2013/06/10 13:36:57]

Done.

+                                         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(kMaxRetryLanguageListFetch);
+  fetcher_->Start();
 }
 
-TranslateLanguageList::~TranslateLanguageList() {}
+void TranslateLanguageList::LanguageListFetcher::OnURLFetchComplete(
+    const net::URLFetcher* source) {
+  DCHECK(fetcher_.get() == source);
 
-void TranslateLanguageList::OnURLFetchComplete(const net::URLFetcher* source) {
-  scoped_ptr<const net::URLFetcher> delete_ptr;
+  scoped_ptr<const net::URLFetcher> delete_ptr(fetcher_.release());

[hajimehoshi, 2013/06/07 04:15:44]

Remove this line (as we discussed).

[Takashi Toyoshima, 2013/06/07 05:17:17]

Thanks.
And after another discussion, I notice that this is not so bad to remove
URLFetcher safely.
I reorganized delete_ptr and callback invocation to improve readability.

 
   if (source->GetStatus().status() == net::URLRequestStatus::SUCCESS &&
       source->GetResponseCode() == net::HTTP_OK) {
+    state_ = COMPLETED;
     std::string data;
     source->GetResponseAsString(&data);
-    if (language_list_fetcher_.get() == source) {
-      delete_ptr.reset(language_list_fetcher_.release());
-      SetSupportedLanguages(data, &supported_languages_);
-    } else if (alpha_language_list_fetcher_.get() == source) {
-      delete_ptr.reset(alpha_language_list_fetcher_.release());
-      SetSupportedLanguages(data, &supported_alpha_languages_);
-    } else {
-      NOTREACHED();
-    }
-    UpdateSupportedLanguages();
+    callback_.Run(include_alpha_languages_, true, data);
   } 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().
     VLOG(9) << "Failed to Fetch languages from: " << kLanguageListFetchURL;
+    state_ = FAILED;
+    callback_.Run(include_alpha_languages_, false, std::string());
   }
 }
 
+// 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() {
+  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(
@@ -250,27 +280,47 @@ bool TranslateLanguageList::IsAlphaLanguage(const std::string& language) {
 }
 
 void TranslateLanguageList::RequestLanguageList() {
-  if (language_list_fetcher_.get() || alpha_language_list_fetcher_.get())
-    return;
+  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)));
+  }
 
-  // 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);
+  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)));
+  }
+}
 
-  language_list_fetcher_.reset(
-      CreateAndStartFetch(1, language_list_fetch_url, this));
+void TranslateLanguageList::OnNetworkChanged(
+    net::NetworkChangeNotifier::ConnectionType type) {
+  VLOG(9) << "OnNetworkChanged: " << type << ", "
+          << (net::NetworkChangeNotifier::IsOffline() ? " offline" : "online");
+  if (net::NetworkChangeNotifier::IsOffline())
+    return;
+  RequestLanguageList();
+}
 
-  // Fetch the alpha language list.
-  language_list_fetch_url = net::AppendQueryParameter(
-      language_list_fetch_url,
-      kAlphaLanguageQueryName,
-      kAlphaLanguageQueryValue);
+void TranslateLanguageList::OnLanguageListFetchComplete(
+    bool include_alpha_languages,
+    bool success,
+    const std::string& data) {
+  if (!success)

[MAD, 2013/06/07 13:29:24]

If we would only call here when there's a real error (and note when we call
twice as commented on above), then we should be able to handle the exponential
backoff'd retries here.

[Takashi Toyoshima, 2013/06/07 14:42:55]

Retry is issued when network status is changed to online, or when EULA is
accepted (EULA handling is implemented in the next CL).
Also, when infobar appears, also it retries to fetch the language list if list
is not updated yet.

But, as you suggested, it must be better to reduce retry opportunity
exponentially.

[MAD, 2013/06/07 15:02:15]

And what I meant is that, since you return an error when there's already a
pending request, this code here can not tell the difference between a real error
and an overlap...

This is why I think the code above should compare the callbacks (I think they
are the same when the Bind objects are created with the same method and this
pointer, but I'm not sure) and only return an error when it's not the same,
because the second callback will never be called afterward, so it's OK to call
it with an error, as opposed to when the callback is the same, then it will
eventually be called with proper results... Is this clear enough? Maybe I don't
explain it correctly...

[Takashi Toyoshima, 2013/06/10 13:36:57]

Thank you for clarification.
After understanding your concerns, I think the simplest solution is that
Request() returns false if another Request() is not finished.

+    return;
 
-  alpha_language_list_fetcher_.reset(
-      CreateAndStartFetch(2, 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() {