Translate: display alpha language name with (alpha) suffix

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 99e3c6e650ae436119538dc2f83a72459232ca08..d12b49dbb086a74c380e0ef51476f22fabcfafca 100644
--- a/chrome/browser/translate/translate_language_list.cc
+++ b/chrome/browser/translate/translate_language_list.cc
@@ -105,6 +105,76 @@ const char kAlphaLanguageQueryValue[] = "1";
 // Retry parameter for fetching supporting language list.
 const int kMaxRetryLanguageListFetch = 5;
 
+// Parses |language_list| containing the list of languages that the translate
+// server can translate to and from, and fills |set| with them.
+void SetSupportedLanguages(const std::string& language_list,
+                           std::set<std::string>* set) {

[MAD, 2013/05/30 14:17:59]

DCHECK(set);

[Takashi Toyoshima, 2013/05/30 14:41:38]

Done.

+  // The format is:
+  // sl({"sl": {"XX": "LanguageName", ...}, "tl": {"XX": "LanguageName", ...}})
+  // Where "sl(" is set in kLanguageListCallbackName
+  // and "tl" is kTargetLanguagesKey
+  if (!StartsWithASCII(language_list,
+                       TranslateLanguageList::kLanguageListCallbackName,
+                       false) ||
+      !EndsWith(language_list, ")", false)) {
+    // We don't have a NOTREACHED here since this can happen in ui_tests, even
+    // though the the BrowserMain function won't call us with parameters.ui_task
+    // is NULL some tests don't set it, so we must bail here.
+    return;
+  }
+  static const size_t kLanguageListCallbackNameLength =
+      strlen(TranslateLanguageList::kLanguageListCallbackName);
+  std::string languages_json = language_list.substr(
+      kLanguageListCallbackNameLength,
+      language_list.size() - kLanguageListCallbackNameLength - 1);
+  scoped_ptr<Value> json_value(
+      base::JSONReader::Read(languages_json, base::JSON_ALLOW_TRAILING_COMMAS));
+  if (json_value == NULL || !json_value->IsType(Value::TYPE_DICTIONARY)) {
+    NOTREACHED();
+    return;
+  }
+  // The first level dictionary contains two sub-dict, one for source languages
+  // and the other for target languages, we want to use the target languages.
+  DictionaryValue* language_dict =
+      static_cast<DictionaryValue*>(json_value.get());
+  DictionaryValue* target_languages = NULL;
+  if (!language_dict->GetDictionary(TranslateLanguageList::kTargetLanguagesKey,
+                                    &target_languages) ||
+      target_languages == NULL) {
+    NOTREACHED();
+    return;
+  }
+
+  // Now we can clear language list.
+  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(const GURL& url,
+                                     net::URLFetcherDelegate* delegate) {
+  DCHECK(delegate);
+  VLOG(9) << "Fetch supporting language list from: " << url.spec().c_str();
+
+  scoped_ptr<net::URLFetcher> fetcher;
+  fetcher.reset(net::URLFetcher::Create(1,
+                                        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();
+}
+
 }  // namespace
 
 // This must be kept in sync with the &cb= value in the kLanguageListFetchURL.
@@ -122,16 +192,26 @@ TranslateLanguageList::TranslateLanguageList() {
 TranslateLanguageList::~TranslateLanguageList() {}
 
 void TranslateLanguageList::OnURLFetchComplete(const net::URLFetcher* source) {
-  DCHECK(url_fetcher_.get() == source);
-  scoped_ptr<const net::URLFetcher> delete_ptr(url_fetcher_.release());
+  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);
-    SetSupportedLanguages(data);
+    if (language_list_fetcher_.get() == source) {
+      delete_ptr.reset(language_list_fetcher_.get());

[MAD, 2013/05/30 14:17:59]

Shouldn't the .get() be a .release()?

[Takashi Toyoshima, 2013/05/30 14:41:38]

Thank you for catching this.

+      SetSupportedLanguages(data, &supported_languages_);
+    } else if (alpha_language_list_fetcher_.get() == source) {
+      delete_ptr.reset(alpha_language_list_fetcher_.get());

[MAD, 2013/05/30 14:17:59]

Same here.

[Takashi Toyoshima, 2013/05/30 14:41:38]

Done.

+      SetSupportedLanguages(data, &supported_alpha_languages_);
+    } else {
+      NOTREACHED();
+    }
   } 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;
   }
 }
@@ -139,8 +219,11 @@ void TranslateLanguageList::OnURLFetchComplete(const net::URLFetcher* source) {
 void TranslateLanguageList::GetSupportedLanguages(
     std::vector<std::string>* languages) {
   DCHECK(languages && languages->empty());
-  std::set<std::string>::const_iterator iter = supported_languages_.begin();
-  for (; iter != supported_languages_.end(); ++iter)
+  std::set<std::string>& list =
+      supported_alpha_languages_.empty() ? supported_languages_ :

[MAD, 2013/05/30 14:17:59]

You used to always return the supported_laguages_ list here (as it used to also
contain the alpha languages), and now you only return the alpha languages as
soon as you have at least one, is this really what we want?

[Takashi Toyoshima, 2013/05/30 14:41:38]

It's no problem that providing only supported_languages_ when alpha language is
not fetched yet.
In that case, language menu contains only stable languages.

Also, this works before both languages are not fetched yet by initial language
list.

In the first logic, supported_languages_ contains all languages from stable and
alpha language lists.

[MAD, 2013/05/30 14:58:01]

Sorry, I still don't understand.

What I was trying to say is if user asks IsSupportedLanguage for language A that
is in supported_languages_ and not in supported_alpha_languages_, we return
true. But when we ask for GetSupportedLanguages and supported_alpha_languages_
is not empty, language A is not part of the supported languages. I find this
confusing.

Unless we are sure that ALL languages in supported_languages_ are ALSO in
supported_alpha_languages_, then we should DCHECK that somewhere...

+                                           supported_alpha_languages_;
+  for (std::set<std::string>::const_iterator iter = list.begin();
+       iter != list.end(); ++iter)
     languages->push_back(*iter);
 }
 
@@ -157,86 +240,40 @@ std::string TranslateLanguageList::GetLanguageCode(
   return chrome_locale.substr(0, hypen_index);
 }
 
-bool TranslateLanguageList::IsSupportedLanguage(
-    const std::string& page_language) {
-  return supported_languages_.count(page_language) != 0;
+bool TranslateLanguageList::IsSupportedLanguage(const std::string& language) {
+  // Langauge should exist at least one of these two language lists.
+  return supported_alpha_languages_.count(language) != 0 ||
+         supported_languages_.count(language) != 0;

[MAD, 2013/05/30 14:17:59]

I guess if we do this, we need to return the content of both lists in
GetSupportedLanguages.

[Takashi Toyoshima, 2013/05/30 14:41:38]

When supported_alpha_languages_ is empty,

GetSupportedLanguages() returns supported_languages_. It's a stable language
list.
Also, IsSupportedLanguage check is equivalent to
supported_languages_.count(language) != 0 because the first condition is always
false. So these two functions are consistent.

When it's not empty,
GetSupportedLanguages() returns supported_alpha_languages_. Basically it
contains stable and alpha languages.
Also, IsSupportedLanguage check is equivalent to
supported_alpha_languages_.count(language) because true set of the latter
condition is contained by true set of the former condition.

Thus, these two functions are consistent on two different conditions. Note that
supported_languages_ is always non-empty set because it contains initial
language set until it fetches server providing language list.

 }
 
-void TranslateLanguageList::RequestLanguageList() {
-  if (url_fetcher_.get())

[MAD, 2013/05/30 14:17:59]

Why didn't you keep this?

[Takashi Toyoshima, 2013/05/30 14:41:38]

Oops, you are right. I should keep it here.
Done.

-    return;
+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() {
+  // 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(language_list_fetch_url, this));
+
+  // TODO(toyoshim): Make it enabled by default. http://crbug.com/242178
   const CommandLine& command_line = *CommandLine::ForCurrentProcess();
-  if (command_line.HasSwitch(switches::kEnableTranslateAlphaLanguages)) {
-    language_list_fetch_url = net::AppendQueryParameter(
-        language_list_fetch_url,
-        kAlphaLanguageQueryName,
-        kAlphaLanguageQueryValue);
-  }
+  if (!command_line.HasSwitch(switches::kEnableTranslateAlphaLanguages))
+    return;
 
-  VLOG(9) << "Fetch supporting language list from: "
-          << language_list_fetch_url.spec().c_str();
-
-  url_fetcher_.reset(net::URLFetcher::Create(1,
-                                             language_list_fetch_url,
-                                             net::URLFetcher::GET,
-                                             this));
-  url_fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
-                             net::LOAD_DO_NOT_SAVE_COOKIES);
-  url_fetcher_->SetRequestContext(g_browser_process->system_request_context());
-  url_fetcher_->SetMaxRetriesOn5xx(kMaxRetryLanguageListFetch);
-  url_fetcher_->Start();
-}
+  // Fetch the alpha language list.
+  language_list_fetch_url = net::AppendQueryParameter(
+      language_list_fetch_url,
+      kAlphaLanguageQueryName,
+      kAlphaLanguageQueryValue);
 
-void TranslateLanguageList::SetSupportedLanguages(
-    const std::string& language_list) {
-  // The format is:
-  // sl({"sl": {"XX": "LanguageName", ...}, "tl": {"XX": "LanguageName", ...}})
-  // Where "sl(" is set in kLanguageListCallbackName
-  // and "tl" is kTargetLanguagesKey
-  if (!StartsWithASCII(language_list,
-                       TranslateLanguageList::kLanguageListCallbackName,
-                       false) ||
-      !EndsWith(language_list, ")", false)) {
-    // We don't have a NOTREACHED here since this can happen in ui_tests, even
-    // though the the BrowserMain function won't call us with parameters.ui_task
-    // is NULL some tests don't set it, so we must bail here.
-    return;
-  }
-  static const size_t kLanguageListCallbackNameLength =
-      strlen(TranslateLanguageList::kLanguageListCallbackName);
-  std::string languages_json = language_list.substr(
-      kLanguageListCallbackNameLength,
-      language_list.size() - kLanguageListCallbackNameLength - 1);
-  scoped_ptr<Value> json_value(
-      base::JSONReader::Read(languages_json, base::JSON_ALLOW_TRAILING_COMMAS));
-  if (json_value == NULL || !json_value->IsType(Value::TYPE_DICTIONARY)) {
-    NOTREACHED();
-    return;
-  }
-  // The first level dictionary contains two sub-dict, one for source languages
-  // and the other for target languages, we want to use the target languages.
-  DictionaryValue* language_dict =
-      static_cast<DictionaryValue*>(json_value.get());
-  DictionaryValue* target_languages = NULL;
-  if (!language_dict->GetDictionary(TranslateLanguageList::kTargetLanguagesKey,
-                                    &target_languages) ||
-      target_languages == NULL) {
-    NOTREACHED();
-    return;
-  }
-  // Now we can clear our current state...
-  supported_languages_.clear();
-  // ... and replace it with the values we just fetched from the server.
-  for (DictionaryValue::Iterator iter(*target_languages);
-       !iter.IsAtEnd();
-       iter.Advance()) {
-    supported_languages_.insert(iter.key());
-  }
+  alpha_language_list_fetcher_.reset(
+      CreateAndStartFetch(language_list_fetch_url, this));
 }