Translate: display alpha language name with (alpha) suffix

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/command_line.h"
 #include "base/json/json_reader.h"
@@ skipping 87 matching lines @@
     "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;
 
+// 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.
 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]);
 }
 
 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;
   }
 }
 
 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);
 }
 
 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& 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.

+}
+
+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 (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;
-
+  // 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();
+  // Fetch the alpha language list.
+  language_list_fetch_url = net::AppendQueryParameter(
+      language_list_fetch_url,
+      kAlphaLanguageQueryName,
+      kAlphaLanguageQueryValue);
 
-  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();
+  alpha_language_list_fetcher_.reset(
+      CreateAndStartFetch(language_list_fetch_url, this));
 }
-
-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());
-  }
-}