using ulp to improve TranslateManager GetTargetLanguage() and InitiateTranslation()

components/translate/core/browser/translate_manager.cc

Index: components/translate/core/browser/translate_manager.cc
diff --git a/components/translate/core/browser/translate_manager.cc b/components/translate/core/browser/translate_manager.cc
index 9de6910142947c366c9ce227ff78d874f936f506..8d7b0480343549c650dc5b4892bb1d8a5ae8a77e 100644
--- a/components/translate/core/browser/translate_manager.cc
+++ b/components/translate/core/browser/translate_manager.cc
@@ -4,10 +4,13 @@
 
 #include "components/translate/core/browser/translate_manager.h"
 
+#include <map>
+
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/stringprintf.h"
 #include "base/time/time.h"
@@ -30,6 +33,7 @@
 #include "components/translate/core/common/translate_pref_names.h"
 #include "components/translate/core/common/translate_switches.h"
 #include "components/translate/core/common/translate_util.h"
+#include "components/variations/variations_associated_data.h"
 #include "google_apis/google_api_keys.h"
 #include "net/base/network_change_notifier.h"
 #include "net/base/url_util.h"
@@ -37,6 +41,8 @@
 
 namespace translate {
 
+const base::Feature kTranslateLanguageByULP{"TranslateLanguageByULP",
+                                            base::FEATURE_DISABLED_BY_DEFAULT};
 namespace {
 
 // Callbacks for translate errors.
@@ -52,6 +58,34 @@ const char kSourceLanguageQueryName[] = "sl";
 // Used in kReportLanguageDetectionErrorURL to specify the page URL.
 const char kUrlQueryName[] = "u";
 
+// Used for  ULP in GetTargetLanguage and InitiateTranslation
+const char kTranslateLanguageByULPTrialName[] = "TranslateLanguageByULP";

[groby-ooo-7-16, 2016/08/02 00:38:20]

Don't need the trial name - just use GetVariationParamsByFeature

[ftang, 2016/08/03 02:01:18]

Done.

+const char kTargetLanguageReadingConfidenceThreshold[] =
+    "target_language_reading_confidence_threshold";
+const char kTargetLanguageReadingProbabilityThreshold[] =
+    "target_language_reading_probability_threshold";
+const char kInitiateTranslationReadingConfidenceThreshold[] =

[groby-ooo-7-16, 2016/08/02 00:38:20]

I'm not sure what this threshold (and the following) is supposed to control -
can you add a comment, please?

[ftang, 2016/08/03 02:01:17]

Done.

+    "initiate_translation_reading_confidence_threshold";
+const char kInitiateTranslationReadingProbabilityThreshold[] =
+    "initiate_translation_reading_probability_threshold";
+const char kInitiateTranslationWritingConfidenceThreshold[] =
+    "initiate_translation_writing_confidence_threshold";
+const char kInitiateTranslationWritingProbabilityThreshold[] =
+    "initiate_translation_writing_probability_threshold";
+
+bool InListAboveThreshold(const std::string language_code,

[groby-ooo-7-16, 2016/08/02 00:38:20]

That combines logic and retrieval - how about just a GetLanguageThreshold
function that takes a probability list?

[ftang, 2016/08/03 02:01:17]

Done.

+                          const TranslatePrefs::LanguageProbabilityList& list,
+                          double threshold) {
+  for (auto it = list.begin(); it != list.end(); ++it) {
+    // We only consider the language if the probability > the thresold.

[groby-ooo-7-16, 2016/08/02 00:38:20]

c++11 loop iteration, please

    for(const auto& it : list) {
      if (it.second > threshold)
     ....

[ftang, 2016/08/03 02:01:17]

Done.

+    if (it->second > threshold &&
+        language_code == TranslateDownloadManager::GetLanguageCode(it->first)) {
+      return true;
+    }
+  }
+  return false;
+}
+
 }  // namespace
 
 TranslateManager::~TranslateManager() {}
@@ -189,8 +223,6 @@ void TranslateManager::InitiateTranslation(const std::string& page_lang) {
   // feature; the user will get an infobar, so they can control whether the
   // page's text is sent to the translate server.
   if (!translate_driver_->IsOffTheRecord()) {
-    std::unique_ptr<TranslatePrefs> translate_prefs =
-        translate_client_->GetTranslatePrefs();
     std::string auto_target_lang =
         GetAutoTargetLanguage(language_code, translate_prefs.get());
     if (!auto_target_lang.empty()) {
@@ -210,6 +242,12 @@ void TranslateManager::InitiateTranslation(const std::string& page_lang) {
     return;
   }
 
+  if (LanguageInULP(translate_prefs.get(), language_code)) {
+    TranslateBrowserMetrics::ReportInitiationStatus(
+        TranslateBrowserMetrics::INITIATION_STATUS_LANGUAGE_IN_ULP);
+    return;
+  }
+
   TranslateBrowserMetrics::ReportInitiationStatus(
       TranslateBrowserMetrics::INITIATION_STATUS_SHOW_INFOBAR);
 
@@ -221,6 +259,48 @@ void TranslateManager::InitiateTranslation(const std::string& page_lang) {
                                      false);
 }
 
+bool TranslateManager::LanguageInULP(const TranslatePrefs* translate_prefs,
+                                     const std::string& language) const {
+  if (base::FeatureList::IsEnabled(kTranslateLanguageByULP)) {
+    std::map<std::string, std::string> params;
+    variations::GetVariationParams(translate::kTranslateLanguageByULPTrialName,
+                                   &params);
+    double temp;
+    double reading_confidence_threshold =
+        base::StringToDouble(

[groby-ooo-7-16, 2016/08/02 00:38:20]

Probably worth factoring out to make this a bit more readable.

[ftang, 2016/08/03 02:01:17]

Done.

+            params[kInitiateTranslationReadingConfidenceThreshold], &temp)
+            ? temp
+            : 0.75;
+    double reading_probability_threshold =
+        base::StringToDouble(
+            params[kInitiateTranslationReadingProbabilityThreshold], &temp)
+            ? temp
+            : 0.5;
+    double writing_confidence_threshold =
+        base::StringToDouble(
+            params[kInitiateTranslationWritingConfidenceThreshold], &temp)
+            ? temp
+            : 0.8;
+    double writing_probability_threshold =
+        base::StringToDouble(
+            params[kInitiateTranslationWritingProbabilityThreshold], &temp)
+            ? temp
+            : 0.3;
+
+    TranslatePrefs::LanguageProbabilityList writing;
+    TranslatePrefs::LanguageProbabilityList reading;

[groby-ooo-7-16, 2016/08/02 00:38:20]

It seems odd we get this data piece by piece, and then process it in several
different places.

Personally, I think we should either get a single chunk of ULPData from the
preferences, or alternatively, have a
GetReadingProbability/GetWritingProbability for a single language.

What do you think?

[ftang, 2016/08/03 02:01:17]

Made some changes, not sure it fit what you ask for. Take a look the new code
first.

+    return (translate_prefs->GetWritingFromUserLanguageProfile(&writing) >
+                writing_confidence_threshold &&
+            InListAboveThreshold(language, writing,
+                                 writing_probability_threshold)) ||
+           (translate_prefs->GetReadingFromUserLanguageProfile(&reading) >
+                reading_confidence_threshold &&
+            InListAboveThreshold(language, reading,
+                                 reading_probability_threshold));
+  }
+  return false;
+}
+
 void TranslateManager::TranslatePage(const std::string& original_source_lang,
                                      const std::string& target_lang,
                                      bool triggered_from_menu) {
@@ -355,14 +435,19 @@ void TranslateManager::OnTranslateScriptFetchComplete(
 
 // static
 std::string TranslateManager::GetTargetLanguage(const TranslatePrefs* prefs) {
-  std::string ui_lang = TranslateDownloadManager::GetLanguageCode(
+  std::string language = TranslateDownloadManager::GetLanguageCode(

[groby-ooo-7-16, 2016/08/02 00:38:20]

Please keep a comment that this is the user's UI language.

[ftang, 2016/08/03 02:01:17]

Done.

       TranslateDownloadManager::GetInstance()->application_locale());
-  translate::ToTranslateLanguageSynonym(&ui_lang);
 
-  TranslateExperiment::OverrideUiLanguage(prefs->GetCountry(), &ui_lang);
+  std::string ulp_language = TranslateManager::GetTargetLanguageFromULP(prefs);
+  if (!ulp_language.empty())
+    language = ulp_language;
+
+  translate::ToTranslateLanguageSynonym(&language);

[groby-ooo-7-16, 2016/08/02 00:38:20]

I'm not happy we duplicate this here and in GetTargetLanguageFromULP. Is there
any way we can remove that? (I know, we currently need this because ULP only
overrides if it's valid...)

[ftang, 2016/08/03 02:01:17]

Done.

 
-  if (TranslateDownloadManager::IsSupportedLanguage(ui_lang))
-    return ui_lang;
+  TranslateExperiment::OverrideUiLanguage(prefs->GetCountry(), &language);

[groby-ooo-7-16, 2016/08/02 00:38:20]

Are you sure you want to do this after ULP? This targets specifically people who
are in a given country, with a given UI language. (ID/MY, with en_XX as UI
language)

[ftang, 2016/08/03 02:01:18]

Done.

+
+  if (TranslateDownloadManager::IsSupportedLanguage(language))
+    return language;
 
   // Will translate to the first supported language on the Accepted Language
   // list or not at all if no such candidate exists.
@@ -377,6 +462,44 @@ std::string TranslateManager::GetTargetLanguage(const TranslatePrefs* prefs) {
 }
 
 // static
+std::string TranslateManager::GetTargetLanguageFromULP(
+    const TranslatePrefs* prefs) {
+  if (base::FeatureList::IsEnabled(kTranslateLanguageByULP)) {
+    std::map<std::string, std::string> params;
+    variations::GetVariationParams(translate::kTranslateLanguageByULPTrialName,
+                                   &params);
+    double temp;
+    double reading_confidence_threshold =

[groby-ooo-7-16, 2016/08/02 00:38:20]

We have default values duplicated across two functions - we should probably
define them as constants instead. 

[ftang, 2016/08/03 02:01:17]

no they are not. We are not reuse threhold in these two functions.
I also do not want to hard code the threshold since we don't know what is the
right threshold. So I like to put them into the config which can be change by
experiment config.

[groby-ooo-7-16, 2016/08/04 02:33:42]

I'm absolutely on board with configuring via Finch. I specifically refer to the
values that you use as default if StringToDouble fails (i.e. 0.75 for reading
confidence, 0.5 for reading probability, etc.) These should probably be
constants.

+        base::StringToDouble(params[kTargetLanguageReadingConfidenceThreshold],
+                             &temp)
+            ? temp
+            : 0.75;
+    double reading_probability_threshold =
+        base::StringToDouble(params[kTargetLanguageReadingProbabilityThreshold],
+                             &temp)
+            ? temp
+            : 0.5;
+    TranslatePrefs::LanguageProbabilityList reading;
+    // We only consider ULP if the confidence is greater than the threshold.
+    if (prefs->GetReadingFromUserLanguageProfile(&reading) >
+        reading_confidence_threshold) {
+      for (auto it = reading.begin(); it != reading.end(); ++it) {

[groby-ooo-7-16, 2016/08/02 00:38:20]

We do a lot of "iterate and compare language code" - maybe this should be
transcoded in a map?

[groby-ooo-7-16, 2016/08/02 00:38:20]

C++11 style loop, please.

[ftang, 2016/08/03 02:01:17]

I think about that, but then we will lost the "order".

[ftang, 2016/08/03 02:01:17]

Done.

+        // We only consider the language if the probability > the thresold.
+        if (it->second > reading_probability_threshold) {
+          std::string ulp_language = it->first;
+          translate::ToTranslateLanguageSynonym(&ulp_language);

[groby-ooo-7-16, 2016/08/02 00:38:20]

It seems we _always_ treat ulp languages as TranslateLanguageSynonym - maybe we
should convert them once, when reading the data? (And also filter out the
invalid ones)

[ftang, 2016/08/03 02:01:17]

Done in the TranslatePrefs now.

+          // Also the language to be supported by Translate.

[groby-ooo-7-16, 2016/08/02 00:38:20]

s/to be/is/

[ftang, 2016/08/03 02:01:17]

Acknowledged.

+          if (TranslateDownloadManager::IsSupportedLanguage(ulp_language)) {
+            return ulp_language;
+          }

[groby-ooo-7-16, 2016/08/02 00:38:20]

In general, this is very deeply nested. I'd prefer it if you instead did
early-out - i.e.

std::string target_language;
if (GetReading <= reading_confidence_threshold)
  return target_language;

for(..) {
  if (it->second > reading_probability) {
    language = it->first;
  } 
}

if (!TDM::IsSupported)
  language.clear();

return language;

+        }
+      }
+    }
+  }
+  return std::string();
+}
+
+// static
 std::string TranslateManager::GetAutoTargetLanguage(
     const std::string& original_language,
     TranslatePrefs* translate_prefs) {