Translate: remove --disable-translate flag (issue 2819813002 by toyoshim@chromium.org)

[toyo...@chromium.org]

Reviewers: groby
CL: https://codereview.chromium.org/2819813002/

Message:
Hi groby,

On creating another patch to remove the alpha language support, I notice that we
have many disabler, but some of them are not maintained well.

What do you think about removing the switch?

Description:
Translate: remove --disable-translate flag

Today, the flag is not maintained well, and is not consistently checked
to invoke the translation feature.

For testing, the translate feature is automatically disabled when
the API Key is not used to compile chromium. This check is also rough,
but should work similarly.
See, https://codereview.chromium.org/1185703007

To disable the feature completely, PolicyList can overwrite the translate
setting. This should work as users disable the feature in chrome://settings.
See, https://www.chromium.org/administrators/policy-list-3#TranslateEnabled

BUG=n/a

Affected files (+21, -57 lines):
M components/translate/core/browser/translate_browser_metrics.h
M components/translate/core/browser/translate_browser_metrics_unittest.cc
M components/translate/core/browser/translate_download_manager.h
M components/translate/core/browser/translate_download_manager.cc
M components/translate/core/browser/translate_manager.cc
M components/translate/core/common/translate_switches.h
M components/translate/core/common/translate_switches.cc


Index: components/translate/core/browser/translate_browser_metrics.h
diff --git a/components/translate/core/browser/translate_browser_metrics.h b/components/translate/core/browser/translate_browser_metrics.h
index cad292988204a7d5ce25848aa3d7b055e1b66342..c83c6bb995a9b404defe3cdf94be870404dc4b13 100644
--- a/components/translate/core/browser/translate_browser_metrics.h
+++ b/components/translate/core/browser/translate_browser_metrics.h
@@ -29,7 +29,7 @@ enum MetricsNameIndex {
// below.
enum InitiationStatusType {
INITIATION_STATUS_DISABLED_BY_PREFS,
- INITIATION_STATUS_DISABLED_BY_SWITCH,
+ DEPRECATE_INITIATION_STATUS_DISABLED_BY_SWITCH,
INITIATION_STATUS_DISABLED_BY_CONFIG,
INITIATION_STATUS_LANGUAGE_IS_NOT_SUPPORTED,
INITIATION_STATUS_URL_IS_NOT_SUPPORTED,
Index: components/translate/core/browser/translate_browser_metrics_unittest.cc
diff --git a/components/translate/core/browser/translate_browser_metrics_unittest.cc b/components/translate/core/browser/translate_browser_metrics_unittest.cc
index ef0fb141ac2939551ae52382f3e81ed9889ff54e..283dfd5ea240af38c49980742a9ca72d3a81e301 100644
--- a/components/translate/core/browser/translate_browser_metrics_unittest.cc
+++ b/components/translate/core/browser/translate_browser_metrics_unittest.cc
@@ -30,7 +30,6 @@ class MetricsRecorder {
}

void CheckInitiationStatus(int expected_disabled_by_prefs,
- int expected_disabled_by_switch,
int expected_disabled_by_config,
int expected_disabled_by_build,
int expected_language_is_not_supported,
@@ -47,10 +46,6 @@ class MetricsRecorder {
GetCountWithoutSnapshot(translate::TranslateBrowserMetrics::
INITIATION_STATUS_DISABLED_BY_PREFS));
EXPECT_EQ(
- expected_disabled_by_switch,
- GetCountWithoutSnapshot(translate::TranslateBrowserMetrics::
- INITIATION_STATUS_DISABLED_BY_SWITCH));
- EXPECT_EQ(
expected_disabled_by_config,
GetCountWithoutSnapshot(translate::TranslateBrowserMetrics::
INITIATION_STATUS_DISABLED_BY_CONFIG));
@@ -132,46 +127,43 @@ TEST(TranslateBrowserMetricsTest, ReportInitiationStatus) {
MetricsRecorder recorder(translate::TranslateBrowserMetrics::GetMetricsName(
translate::TranslateBrowserMetrics::UMA_INITIATION_STATUS));

- recorder.CheckInitiationStatus(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
+ recorder.CheckInitiationStatus(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
translate::TranslateBrowserMetrics::ReportInitiationStatus(
translate::TranslateBrowserMetrics::INITIATION_STATUS_DISABLED_BY_PREFS);
- recorder.CheckInitiationStatus(1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
- translate::TranslateBrowserMetrics::ReportInitiationStatus(
- translate::TranslateBrowserMetrics::INITIATION_STATUS_DISABLED_BY_SWITCH);
- recorder.CheckInitiationStatus(1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
+ recorder.CheckInitiationStatus(1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
translate::TranslateBrowserMetrics::ReportInitiationStatus(
translate::TranslateBrowserMetrics::INITIATION_STATUS_DISABLED_BY_CONFIG);
- recorder.CheckInitiationStatus(1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0);
+ recorder.CheckInitiationStatus(1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0);
translate::TranslateBrowserMetrics::ReportInitiationStatus(
translate::TranslateBrowserMetrics::INITIATION_STATUS_DISABLED_BY_KEY);
- recorder.CheckInitiationStatus(1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0);
+ recorder.CheckInitiationStatus(1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0);
translate::TranslateBrowserMetrics::ReportInitiationStatus(
translate::TranslateBrowserMetrics::
INITIATION_STATUS_LANGUAGE_IS_NOT_SUPPORTED);
- recorder.CheckInitiationStatus(1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0);
+ recorder.CheckInitiationStatus(1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0);
translate::TranslateBrowserMetrics::ReportInitiationStatus(
translate::TranslateBrowserMetrics::
INITIATION_STATUS_MIME_TYPE_IS_NOT_SUPPORTED);
- recorder.CheckInitiationStatus(1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0);
+ recorder.CheckInitiationStatus(1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0);
translate::TranslateBrowserMetrics::ReportInitiationStatus(
translate::TranslateBrowserMetrics::
INITIATION_STATUS_URL_IS_NOT_SUPPORTED);
- recorder.CheckInitiationStatus(1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0);
+ recorder.CheckInitiationStatus(1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0);
translate::TranslateBrowserMetrics::ReportInitiationStatus(
translate::TranslateBrowserMetrics::INITIATION_STATUS_SIMILAR_LANGUAGES);
- recorder.CheckInitiationStatus(1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0);
+ recorder.CheckInitiationStatus(1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0);
translate::TranslateBrowserMetrics::ReportInitiationStatus(
translate::TranslateBrowserMetrics::INITIATION_STATUS_ACCEPT_LANGUAGES);
- recorder.CheckInitiationStatus(1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0);
+ recorder.CheckInitiationStatus(1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0);
translate::TranslateBrowserMetrics::ReportInitiationStatus(
translate::TranslateBrowserMetrics::INITIATION_STATUS_AUTO_BY_CONFIG);
- recorder.CheckInitiationStatus(1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0);
+ recorder.CheckInitiationStatus(1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0);
translate::TranslateBrowserMetrics::ReportInitiationStatus(
translate::TranslateBrowserMetrics::INITIATION_STATUS_AUTO_BY_LINK);
- recorder.CheckInitiationStatus(1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0);
+ recorder.CheckInitiationStatus(1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0);
translate::TranslateBrowserMetrics::ReportInitiationStatus(
translate::TranslateBrowserMetrics::INITIATION_STATUS_SHOW_INFOBAR);
- recorder.CheckInitiationStatus(1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1);
+ recorder.CheckInitiationStatus(1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1);
}

TEST(TranslateBrowserMetricsTest, ReportLanguageDetectionError) {
Index: components/translate/core/browser/translate_download_manager.cc
diff --git a/components/translate/core/browser/translate_download_manager.cc b/components/translate/core/browser/translate_download_manager.cc
index aebdd3dfc92d4c4c92dea2c3f7b791ec9f1f7da5..e5ac05e18888a850a429ca8119bb2f9012772745 100644
--- a/components/translate/core/browser/translate_download_manager.cc
+++ b/components/translate/core/browser/translate_download_manager.cc
@@ -4,7 +4,6 @@

#include "components/translate/core/browser/translate_download_manager.h"

-#include "base/command_line.h"
#include "base/logging.h"
#include "base/memory/singleton.h"
#include "components/prefs/pref_service.h"
@@ -31,30 +30,21 @@ void TranslateDownloadManager::Shutdown() {
}

// static
-void TranslateDownloadManager::RequestLanguageList() {
+void TranslateDownloadManager::RequestLanguageList(PrefService* prefs) {
+ // We don't want to do this when translate is disabled.
+ DCHECK(prefs != NULL);
+ if (!prefs->GetBoolean(prefs::kEnableTranslate))
+ return;
+
TranslateLanguageList* language_list = GetInstance()->language_list();
if (!language_list) {
NOTREACHED();
return;
}
-
language_list->RequestLanguageList();
}

// static
-void TranslateDownloadManager::RequestLanguageList(PrefService* prefs) {
- // We don't want to do this when translate is disabled.
- DCHECK(prefs != NULL);
- if (base::CommandLine::ForCurrentProcess()->HasSwitch(
- translate::switches::kDisableTranslate) ||
- !prefs->GetBoolean(prefs::kEnableTranslate)) {
- return;
- }
-
- RequestLanguageList();
-}
-
-// static
void TranslateDownloadManager::GetSupportedLanguages(
std::vector<std::string>* languages) {
TranslateLanguageList* language_list = GetInstance()->language_list();
Index: components/translate/core/browser/translate_download_manager.h
diff --git a/components/translate/core/browser/translate_download_manager.h b/components/translate/core/browser/translate_download_manager.h
index 5d1c4e2d85c8cd6f877ef3f85fda4396f2f003dd..7f16a86652cd15214d72c8e3d112faad5a013e55 100644
--- a/components/translate/core/browser/translate_download_manager.h
+++ b/components/translate/core/browser/translate_download_manager.h
@@ -51,13 +51,10 @@ class TranslateDownloadManager {
TranslateScript* script() { return script_.get(); }

// Let the caller decide if and when we should fetch the language list from
- // the translate server. This is a NOOP if switches::kDisableTranslate is set
- // or if prefs::kEnableTranslate is set to false.
+ // the translate server. This is a NOOP if prefs::kEnableTranslate is set to
+ // false.
static void RequestLanguageList(PrefService* prefs);

- // Fetches the language list from the translate server.
- static void RequestLanguageList();
-
// Fills |languages| with the list of languages that the translate server can
// translate to and from.
static void GetSupportedLanguages(std::vector<std::string>* languages);
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 0f2e8e45fd75441facffb630d0b2d1f966ca53db..f0adcee370956ffba12e3b1b4e896bb832f069a0 100644
--- a/components/translate/core/browser/translate_manager.cc
+++ b/components/translate/core/browser/translate_manager.cc
@@ -188,15 +188,6 @@ void TranslateManager::InitiateTranslation(const std::string& page_lang) {
return;
}

- // Allow disabling of translate from the command line to assist with
- // automated browser testing.
- if (base::CommandLine::ForCurrentProcess()->HasSwitch(
- translate::switches::kDisableTranslate)) {
- TranslateBrowserMetrics::ReportInitiationStatus(
- TranslateBrowserMetrics::INITIATION_STATUS_DISABLED_BY_SWITCH);
- return;
- }
-
// MHTML pages currently cannot be translated.
// See bug: 217945.
if (translate_driver_->GetContentsMimeType() == "multipart/related") {
Index: components/translate/core/common/translate_switches.cc
diff --git a/components/translate/core/common/translate_switches.cc b/components/translate/core/common/translate_switches.cc
index 2a6da776977d33710cd79aa1db4e4f1d5aa03040..9ee0352f97a8af02fa11cea43c8ca433940909dc 100644
--- a/components/translate/core/common/translate_switches.cc
+++ b/components/translate/core/common/translate_switches.cc
@@ -7,11 +7,6 @@
namespace translate {
namespace switches {

-// Allows disabling of translate from the command line to assist with automated
-// browser testing (e.g. Selenium/WebDriver). Normal browser users should
-// disable translate with the preference.
-const char kDisableTranslate[] = "disable-translate";
-
// Overrides the default server used for Google Translate.
const char kTranslateScriptURL[] = "translate-script-url";

Index: components/translate/core/common/translate_switches.h
diff --git a/components/translate/core/common/translate_switches.h b/components/translate/core/common/translate_switches.h
index 4db33fc8ff67dcfe07309dfe8e29712e8b525530..ad7010b038587cd05d6391bd24ac3b66e5da0dc6 100644
--- a/components/translate/core/common/translate_switches.h
+++ b/components/translate/core/common/translate_switches.h
@@ -8,7 +8,6 @@
namespace translate {
namespace switches {

-extern const char kDisableTranslate[];
extern const char kTranslateScriptURL[];
extern const char kTranslateSecurityOrigin[];
extern const char kTranslateRankerModelURL[];

[gr...@chromium.org]

On 2017/04/14 11:34:39, Takashi Toyoshima wrote:
> Hi groby,
>
> On creating another patch to remove the alpha language support, I notice that
we
> have many disabler, but some of them are not maintained well.
>
> What do you think about removing the switch?

Apologies for missing this. When we remove the switch, how will that affect
ChromePublic.apk? Will the bug (#500025) still be addressed?

https://codereview.chromium.org/2819813002/

[toyo...@chromium.org]

n/p. this isn't urgent at all :)
just a small cleanup that another code review reminds me.


https://codereview.chromium.org/2819813002/diff/1/components/translate/core/browser/translate_manager.cc
File components/translate/core/browser/translate_manager.cc (right):

https://codereview.chromium.org/2819813002/diff/1/components/translate/core/browser/translate_manager.cc#newcode170
components/translate/core/browser/translate_manager.cc:170:
!::google_apis::HasKeysConfigured()) {
Yes, this check was added to fix the issue 500025, and still works.

https://codereview.chromium.org/2819813002/

[gr...@chromium.org]

lgtm



https://codereview.chromium.org/2819813002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2819813002/20001

https://codereview.chromium.org/2819813002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/4baa4206fac22a91b3c76a429143fc061017f318

https://codereview.chromium.org/2819813002/

[szz...@gmail.com]

Why have you removed --disable-translate flag??? The popup window for translation is very very annoying!!! And now there is no possibility to get rid of it with a command line option.

[Rachel Blum]

Because you can disable it through settings. Go to chrome://settings/languages

There, click the drop-down arrow next to your current language (see picture)

Then, disable under "Offer to translate pages that aren't in a language you read" (see picture)

Hope that helps,

 - Rachel (she/her)

[szz...@gmail.com]

Yes, I know this possibility. But there are situations when this solution cannot be applied. For example in a live operating system: every time you start the browser have to do this setting manually. But if there would be a command line switch, the problem would be solved.

I also found on the net the "--disable-features=TranslateUI" flag but it does not work in my Chrome V66.0.3359.170. Is this flag implemented or only was a proposed flag to be implemented?

[szz...@gmail.com]

Still there is one way to disable page translation (outside of the manual setting). Set policy:

# mkdir -p /etc/opt/chrome/policies/managed
# touch /etc/opt/chrome/policies/managed/test_policy.json
# nano /etc/opt/chrome/policies/managed/test_policy.json

{
  "TranslateEnabled": false
}

But this is not a solution. Because not the option to translate the page is the problem but the annoying popup window. If I apply this policy will be disabled the translation for all pages and for all browser instances. Still there could be a situation when this feature would be useful...

The right Chrome behaviour would be: no automatic popup window by default (it may be enabled in settings) but page translation from menu or right click... Since even if a user uses this feature, surely he does not want to do this for all pages he opens. And if a user yet want to translate all pages, he could enable the popup window (or even automatic page translation...) in settings.
Or should be a command line switch to disable the automatic popup window if you still consider that it should be enabled by default. In this way, there would be a possibility to start Chrome witth the appropriate behaviour with different desktop files.

[Mario Valle]

My use case is Chrome used as application interface. I run the server that spawn the Chrome browser with these parameters:

        const commandLineParameters = [

                            `--app=${protocol}://${url}?${payload}`,

                            "--start-maximized",

                            "--allow-insecure-localhost",

                            "--new-window",

                            "--disable-extensions",

                            "--disable-features=TranslateUI",

                            `--user-data-dir=${temporaryProfileDirectory}`

        ];

The temporary profile directory is needed to makes the extensions not start in this application. I added the disable feature otherwise every time the server spawn the client it asks if should translate the page. At the moment seems to work, but I was expecting running with --app should disable everything by default.

Just my 0.02 €

mario

[Mario Valle]

Sorry, not TranslateUI but Translate

[Rachel Blum]

If you have control over the pages served, <html translate="no"> will disable translate for that page.

In an enterprise environment, you can disable it via policy: https://cloud.google.com/docs/chrome-enterprise/policies/?policy=TranslateEnabled (See also further up in thread)

There is currently no ability to disable the popup via settings or flags.

 - Rachel (she/her)