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/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/translate/translate_url_util.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/load_flags.h"
 #include "net/base/url_util.h"
@@ skipping 129 matching lines @@
   set->clear();
   // ... and replace it with the values we just fetched from the server.
   for (DictionaryValue::Iterator iter(*target_languages);
        !iter.IsAtEnd();
        iter.Advance()) {
     // TODO(toyoshim): Check if UI libraries support adding locale.
     set->insert(iter.key());
   }
 }
 
-net::URLFetcher* CreateAndStartFetch(int id,
-                                     const GURL& url,
-                                     net::URLFetcherDelegate* delegate) {
-  DCHECK(delegate);
+}  // namespace
+
+TranslateLanguageList::LanguageListFetcher::LanguageListFetcher(
+    bool include_alpha_languages)
+  : include_alpha_languages_(include_alpha_languages),
+    state_(IDLE) {
+}
+
+TranslateLanguageList::LanguageListFetcher::~LanguageListFetcher() {
+}
+
+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;
+  }
+
+  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.

 
-  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_ ? 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();
 }
 
-}  // namespace
+void TranslateLanguageList::LanguageListFetcher::OnURLFetchComplete(
+    const net::URLFetcher* source) {
+  DCHECK(fetcher_.get() == source);
+
+  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);
+    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() {}
-
-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) {
-      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();
-  } 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;
-  }
+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) {
+  VLOG(9) << "OnNetworkChanged: " << type << ", "
+          << (net::NetworkChangeNotifier::IsOffline() ? " offline" : "online");
+  if (net::NetworkChangeNotifier::IsOffline())
+    return;
+  RequestLanguageList();
+}
+
+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;
 
-  // 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(1, 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(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() {
   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);
 }