Skip to content

Commit

Permalink
Fix Language List in Settings.
Browse files Browse the repository at this point in the history
In this change we fix:
1) Translation can be enable/disabled for region-specific languages too.
2) The state of translation is kept in sync for each language of the same family.
3) Adding a language does not remove translation of that language family if
   there is at least another language from that family in the list already.
4) Removing a language will remove it from the blocked list too.
5) All Chinese languages are handled correctly now.

All changes are behind a default-disabled feature.

This is the second change for Language Settings: the first one was 670459.

Bug: 719760
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I5599b74797f2f135ef0be2e5e03c11c9ca37881b
Reviewed-on: https://chromium-review.googlesource.com/683935
Commit-Queue: Claudio M <claudiomagni@chromium.org>
Reviewed-by: Hajime Hoshi <hajimehoshi@chromium.org>
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Dave Schuyler <dschuyler@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508323}
  • Loading branch information
claudio-chromium authored and Commit Bot committed Oct 12, 2017
1 parent e172457 commit 497822f
Show file tree
Hide file tree
Showing 20 changed files with 625 additions and 82 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include <utility>
#include <vector>

#include "base/feature_list.h"
#include "base/i18n/rtl.h"
#include "base/memory/ptr_util.h"
#include "base/stl_util.h"
Expand All @@ -35,6 +36,8 @@
#include "components/language/core/browser/language_model.h"
#include "components/spellcheck/common/spellcheck_common.h"
#include "components/translate/core/browser/translate_download_manager.h"
#include "components/translate/core/browser/translate_prefs.h"
#include "components/translate/core/common/translate_util.h"
#include "third_party/icu/source/i18n/unicode/coll.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/base/l10n/l10n_util_collator.h"
Expand Down Expand Up @@ -238,7 +241,14 @@ LanguageSettingsPrivateGetLanguageListFunction::Run() {
language.supports_ui.reset(new bool(true));
if (spellcheck_language_set.count(pair.first) > 0)
language.supports_spellcheck.reset(new bool(true));
if (translate_language_set.count(pair.first) > 0)

std::string supports_translate_code = pair.first;
if (base::FeatureList::IsEnabled(translate::kImprovedLanguageSettings)) {
// Extract the base language: if the base language can be translated, then
// even the regional one should be marked as such.
translate::ToTranslateLanguageSynonym(&supports_translate_code);
}
if (translate_language_set.count(supports_translate_code) > 0)
language.supports_translate.reset(new bool(true));

language_list->Append(language.ToValue());
Expand Down Expand Up @@ -267,14 +277,15 @@ LanguageSettingsPrivateEnableLanguageFunction::Run() {

std::vector<std::string> languages;
translate_prefs->GetLanguageList(&languages);
std::string chrome_language = language_code;
translate::ToChromeLanguageSynonym(&chrome_language);

if (base::ContainsValue(languages, language_code)) {
LOG(ERROR) << "Language " << language_code << " already enabled";
if (base::ContainsValue(languages, chrome_language)) {
LOG(ERROR) << "Language " << chrome_language << " already enabled";
return RespondNow(NoArguments());
}

languages.push_back(parameters->language_code);
translate_prefs->UpdateLanguageList(languages);
translate_prefs->AddToLanguageList(language_code, /*force_blocked=*/false);

return RespondNow(NoArguments());
}
Expand All @@ -300,15 +311,47 @@ LanguageSettingsPrivateDisableLanguageFunction::Run() {

std::vector<std::string> languages;
translate_prefs->GetLanguageList(&languages);
std::string chrome_language = language_code;
translate::ToChromeLanguageSynonym(&chrome_language);

auto it = std::find(languages.begin(), languages.end(), language_code);
auto it = std::find(languages.begin(), languages.end(), chrome_language);
if (it == languages.end()) {
LOG(ERROR) << "Language " << language_code << " not enabled";
LOG(ERROR) << "Language " << chrome_language << " not enabled";
return RespondNow(NoArguments());
}

languages.erase(it);
translate_prefs->UpdateLanguageList(languages);
translate_prefs->RemoveFromLanguageList(language_code);

return RespondNow(NoArguments());
}

LanguageSettingsPrivateSetEnableTranslationForLanguageFunction::
LanguageSettingsPrivateSetEnableTranslationForLanguageFunction()
: chrome_details_(this) {}

LanguageSettingsPrivateSetEnableTranslationForLanguageFunction::
~LanguageSettingsPrivateSetEnableTranslationForLanguageFunction() {}

ExtensionFunction::ResponseAction
LanguageSettingsPrivateSetEnableTranslationForLanguageFunction::Run() {
const std::unique_ptr<
language_settings_private::SetEnableTranslationForLanguage::Params>
parameters = language_settings_private::SetEnableTranslationForLanguage::
Params::Create(*args_);
EXTENSION_FUNCTION_VALIDATE(parameters.get());
const std::string& language_code = parameters->language_code;
// True if translation enabled, false if disabled.
const bool enable = parameters->enable;

std::unique_ptr<translate::TranslatePrefs> translate_prefs =
ChromeTranslateClient::CreateTranslatePrefs(
chrome_details_.GetProfile()->GetPrefs());

if (enable) {
translate_prefs->UnblockLanguage(language_code);
} else {
translate_prefs->BlockLanguage(language_code);
}

return RespondNow(NoArguments());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,28 @@ class LanguageSettingsPrivateDisableLanguageFunction
DISALLOW_COPY_AND_ASSIGN(LanguageSettingsPrivateDisableLanguageFunction);
};

// Implements the languageSettingsPrivate.setEnableTranslationForLanguage
// method.
class LanguageSettingsPrivateSetEnableTranslationForLanguageFunction
: public UIThreadExtensionFunction {
public:
LanguageSettingsPrivateSetEnableTranslationForLanguageFunction();
DECLARE_EXTENSION_FUNCTION(
"languageSettingsPrivate.setEnableTranslationForLanguage",
LANGUAGESETTINGSPRIVATE_SETENABLETRANSLATIONFORLANGUAGE)

protected:
~LanguageSettingsPrivateSetEnableTranslationForLanguageFunction() override;

// ExtensionFunction overrides.
ResponseAction Run() override;

private:
ChromeExtensionFunctionDetails chrome_details_;
DISALLOW_COPY_AND_ASSIGN(
LanguageSettingsPrivateSetEnableTranslationForLanguageFunction);
};

// Implements the languageSettingsPrivate.getSpellcheckDictionaryStatuses
// method.
class LanguageSettingsPrivateGetSpellcheckDictionaryStatusesFunction
Expand Down
14 changes: 7 additions & 7 deletions chrome/browser/resources/settings/languages_page/languages.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ var kLanguageCodeToTranslateCode = {
'zh-HK': 'zh-TW',
'zh-MO': 'zh-TW',
'zh-SG': 'zh-CN',
'zh': 'zh-CH',
};

// Some ISO 639 language codes have been renamed, e.g. "he" to "iw", but
Expand Down Expand Up @@ -294,6 +295,8 @@ Polymer({
this.languages.prospectiveUILanguage) {
continue;
}
// This conversion primarily strips away the region part.
// For example "fr-CA" --> "fr".
var translateCode = this.convertLanguageCodeForTranslate(
this.languages.enabled[i].language.code);
this.set(
Expand Down Expand Up @@ -542,7 +545,6 @@ Polymer({
return;

this.languageSettingsPrivate_.enableLanguage(languageCode);
this.disableTranslateLanguage(languageCode);
},

/**
Expand Down Expand Up @@ -573,7 +575,6 @@ Polymer({

// Remove the language from preferred languages.
this.languageSettingsPrivate_.disableLanguage(languageCode);
this.enableTranslateLanguage(languageCode);
},

/**
Expand Down Expand Up @@ -668,8 +669,8 @@ Polymer({
* @param {string} languageCode
*/
enableTranslateLanguage: function(languageCode) {
languageCode = this.convertLanguageCodeForTranslate(languageCode);
this.deletePrefListItem('translate_blocked_languages', languageCode);
this.languageSettingsPrivate_.setEnableTranslationForLanguage(
languageCode, true);
},

/**
Expand All @@ -678,9 +679,8 @@ Polymer({
* @param {string} languageCode
*/
disableTranslateLanguage: function(languageCode) {
this.appendPrefListItem(
'translate_blocked_languages',
this.convertLanguageCodeForTranslate(languageCode));
this.languageSettingsPrivate_.setEnableTranslationForLanguage(
languageCode, false);
},

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1275,7 +1275,7 @@ TEST_F(TranslateManagerRenderViewHostTest, NeverTranslateLanguagePref) {
EXPECT_TRUE(translate_prefs->CanTranslateLanguage(accept_languages, "fr"));
SetPrefObserverExpectation(
translate::TranslatePrefs::kPrefTranslateBlockedLanguages);
translate_prefs->BlockLanguage("fr");
translate_prefs->AddToLanguageList("fr", /*force_blocked=*/true);
EXPECT_TRUE(translate_prefs->IsBlockedLanguage("fr"));
EXPECT_FALSE(translate_prefs->IsSiteBlacklisted(url.host()));
EXPECT_FALSE(translate_prefs->CanTranslateLanguage(accept_languages, "fr"));
Expand Down Expand Up @@ -1435,7 +1435,7 @@ TEST_F(TranslateManagerRenderViewHostTest, ContextMenu) {
Profile::FromBrowserContext(web_contents()->GetBrowserContext());
std::unique_ptr<translate::TranslatePrefs> translate_prefs(
ChromeTranslateClient::CreateTranslatePrefs(profile->GetPrefs()));
translate_prefs->BlockLanguage("fr");
translate_prefs->AddToLanguageList("fr", /*force_blocked=*/true);
translate_prefs->BlacklistSite(url.host());
EXPECT_TRUE(translate_prefs->IsBlockedLanguage("fr"));
EXPECT_TRUE(translate_prefs->IsSiteBlacklisted(url.host()));
Expand Down
4 changes: 4 additions & 0 deletions chrome/common/extensions/api/language_settings_private.idl
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,10 @@ namespace languageSettingsPrivate {
// Disables a language, removing it from the Accept-Language list.
static void disableLanguage(DOMString languageCode);

// Enables or disables translation for a given language.
static void setEnableTranslationForLanguage(
DOMString languageCode, boolean enable);

// Gets the current status of the chosen spell check dictionaries.
static void getSpellcheckDictionaryStatuses(
GetSpellcheckDictionaryStatusesCallback callback);
Expand Down
12 changes: 6 additions & 6 deletions chrome/renderer/translate/translate_helper_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -492,11 +492,11 @@ TEST_F(TranslateHelperBrowserTest, LanguageCommonMistakesAreCorrected) {

// Tests that a back navigation gets a translate language message.
TEST_F(TranslateHelperBrowserTest, BackToTranslatablePage) {
LoadHTML("<html><head><meta http-equiv=\"content-language\" content=\"zh\">"
"</head><body>This page is in Chinese.</body></html>");
LoadHTML("<html><head><meta http-equiv=\"content-language\" content=\"es\">"
"</head><body>This page is in Spanish.</body></html>");
base::RunLoop().RunUntilIdle();
ASSERT_TRUE(fake_translate_driver_.called_new_page_);
EXPECT_EQ("zh", fake_translate_driver_.details_->adopted_language);
EXPECT_EQ("es", fake_translate_driver_.details_->adopted_language);
fake_translate_driver_.ResetNewPageValues();

content::PageState back_state = GetCurrentPageState();
Expand All @@ -509,11 +509,11 @@ TEST_F(TranslateHelperBrowserTest, BackToTranslatablePage) {
fake_translate_driver_.ResetNewPageValues();

GoBack(GURL("data:text/html;charset=utf-8,<html><head>"
"<meta http-equiv=\"content-language\" content=\"zh\">"
"</head><body>This page is in Chinese.</body></html>"),
"<meta http-equiv=\"content-language\" content=\"es\">"
"</head><body>This page is in Spanish.</body></html>"),
back_state);

base::RunLoop().RunUntilIdle();
ASSERT_TRUE(fake_translate_driver_.called_new_page_);
EXPECT_EQ("zh", fake_translate_driver_.details_->adopted_language);
EXPECT_EQ("es", fake_translate_driver_.details_->adopted_language);
}
24 changes: 24 additions & 0 deletions chrome/test/data/webui/settings/fake_language_settings_private.js
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,30 @@ cr.define('settings', function() {
}
}

/**
* Enables/Disables translation for the given language.
* This respectively removes/adds the language to the blocked set in the
* preferences.
* @param {string} languageCode
* @param {boolean} enable
*/
setEnableTranslationForLanguage(languageCode, enable) {
var index =
this.settingsPrefs_.prefs.translate_blocked_languages.value.indexOf(
languageCode);
if (enable) {
if (index == -1)
return;
this.settingsPrefs_.splice(
'prefs.translate_blocked_languages.value', index, 1);
} else {
if (index != -1)
return;
this.settingsPrefs_.push(
'prefs.translate_blocked_languages.value', languageCode);
}
}

/**
* Gets the current status of the chosen spell check dictionaries.
* @param {function(!Array<
Expand Down
Loading

0 comments on commit 497822f

Please sign in to comment.