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

components/translate/core/browser/translate_manager.cc

 // Copyright 2014 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 "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"
 #include "components/prefs/pref_service.h"
 #include "components/translate/core/browser/language_state.h"
 #include "components/translate/core/browser/page_translated_details.h"
 #include "components/translate/core/browser/translate_accept_languages.h"
 #include "components/translate/core/browser/translate_browser_metrics.h"
 #include "components/translate/core/browser/translate_client.h"
 #include "components/translate/core/browser/translate_download_manager.h"
 #include "components/translate/core/browser/translate_driver.h"
 #include "components/translate/core/browser/translate_error_details.h"
 #include "components/translate/core/browser/translate_experiment.h"
 #include "components/translate/core/browser/translate_language_list.h"
 #include "components/translate/core/browser/translate_prefs.h"
 #include "components/translate/core/browser/translate_script.h"
 #include "components/translate/core/browser/translate_url_util.h"
 #include "components/translate/core/common/language_detection_details.h"
 #include "components/translate/core/common/translate_constants.h"
 #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"
 #include "net/http/http_status_code.h"
 
 namespace translate {
 
+const base::Feature kTranslateLanguageByULP{"TranslateLanguageByULP",
+                                            base::FEATURE_DISABLED_BY_DEFAULT};
 namespace {
 
 // Callbacks for translate errors.
 TranslateManager::TranslateErrorCallbackList* g_callback_list_ = NULL;
 
 const char kReportLanguageDetectionErrorURL[] =
     "https://translate.google.com/translate_error?client=cr&action=langidc";
 
 // Used in kReportLanguageDetectionErrorURL to specify the original page
 // language.
 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() {}
 
 // static
 std::unique_ptr<TranslateManager::TranslateErrorCallbackList::Subscription>
 TranslateManager::RegisterTranslateErrorCallback(
     const TranslateManager::TranslateErrorCallback& callback) {
   if (!g_callback_list_)
     g_callback_list_ = new TranslateErrorCallbackList;
@@ skipping 117 matching lines @@
     TranslateBrowserMetrics::ReportInitiationStatus(
         TranslateBrowserMetrics::INITIATION_STATUS_DISABLED_BY_CONFIG);
     return;
   }
 
   // If the user has previously selected "always translate" for this language we
   // automatically translate.  Note that in incognito mode we disable that
   // 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()) {
       TranslateBrowserMetrics::ReportInitiationStatus(
           TranslateBrowserMetrics::INITIATION_STATUS_AUTO_BY_CONFIG);
       TranslatePage(language_code, auto_target_lang, false);
       return;
     }
   }
 
   std::string auto_translate_to = language_state_.AutoTranslateTo();
   if (!auto_translate_to.empty()) {
     // This page was navigated through a click from a translated page.
     TranslateBrowserMetrics::ReportInitiationStatus(
         TranslateBrowserMetrics::INITIATION_STATUS_AUTO_BY_LINK);
     TranslatePage(language_code, auto_translate_to, false);
     return;
   }
 
+  if (LanguageInULP(translate_prefs.get(), language_code)) {
+    TranslateBrowserMetrics::ReportInitiationStatus(
+        TranslateBrowserMetrics::INITIATION_STATUS_LANGUAGE_IN_ULP);
+    return;
+  }
+
   TranslateBrowserMetrics::ReportInitiationStatus(
       TranslateBrowserMetrics::INITIATION_STATUS_SHOW_INFOBAR);
 
   // Prompts the user if they want the page translated.
   translate_client_->ShowTranslateUI(translate::TRANSLATE_STEP_BEFORE_TRANSLATE,
                                      language_code,
                                      target_lang,
                                      TranslateErrors::NONE,
                                      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) {
   if (!translate_driver_->HasCurrentPage()) {
     NOTREACHED();
     return;
   }
 
   // Translation can be kicked by context menu against unsupported languages.
   // Unsupported language strings should be replaced with
@@ skipping 114 matching lines @@
         source_lang,
         target_lang,
         TranslateErrors::NETWORK,
         false);
     NotifyTranslateError(TranslateErrors::NETWORK);
   }
 }
 
 // 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;
 
-  if (TranslateDownloadManager::IsSupportedLanguage(ui_lang))
-    return ui_lang;
+  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.

+
+  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.
   std::vector<std::string> accept_languages_list;
   prefs->GetLanguageList(&accept_languages_list);
   for (const auto& lang : accept_languages_list) {
     std::string lang_code = TranslateDownloadManager::GetLanguageCode(lang);
     if (TranslateDownloadManager::IsSupportedLanguage(lang_code))
       return lang_code;
   }
   return std::string();
 }
 
 // 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) {
   std::string auto_target_lang;
   if (translate_prefs->ShouldAutoTranslate(original_language,
                                            &auto_target_lang)) {
     // We need to confirm that the saved target language is still supported.
     // Also, GetLanguageCode will take care of removing country code if any.
     auto_target_lang =
         TranslateDownloadManager::GetLanguageCode(auto_target_lang);
     if (TranslateDownloadManager::IsSupportedLanguage(auto_target_lang))
       return auto_target_lang;
   }
   return std::string();
 }
 
 LanguageState& TranslateManager::GetLanguageState() {
   return language_state_;
 }
 
 bool TranslateManager::ignore_missing_key_for_testing_ = false;
 
 // static
 void TranslateManager::SetIgnoreMissingKeyForTesting(bool ignore) {
   ignore_missing_key_for_testing_ = ignore;
 }
 
 }  // namespace translate