Remove the flag to disable new hotwording. (issue 981773003 by amistry@chromium.org)

[ami...@chromium.org]

Reviewers: kcarattini, Matt Giuca,

Description:
Remove the flag to disable new hotwording.

Removing all the dead code as a result of this is large change and will
be done separately.

BUG=439143

Please review this at https://codereview.chromium.org/981773003/

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+6, -41 lines):
M chrome/app/generated_resources.grd
M chrome/browser/about_flags.cc
M chrome/browser/extensions/hotword_browsertest.cc
M chrome/browser/search/hotword_service.h
M chrome/browser/search/hotword_service.cc
M chrome/browser/search/hotword_service_unittest.cc
M chrome/common/chrome_switches.h
M chrome/common/chrome_switches.cc


Index: chrome/app/generated_resources.grd
diff --git a/chrome/app/generated_resources.grd
b/chrome/app/generated_resources.grd
index
1ff003879b10158446f5931f4f95d8940f66233f..b9663f195bbe88d1e4797496503faaf7ef6d3ace
100644
--- a/chrome/app/generated_resources.grd
+++ b/chrome/app/generated_resources.grd
@@ -15491,14 +15491,6 @@ Do you accept?
</message>

<!-- Built-in hotword detection display strings -->
- <message name="IDS_FLAGS_DISABLE_EXPERIMENTAL_HOTWORDING_NAME"
desc="Name of about:flags option for hotword detection.">
- Disable version 2 'Ok Google' hotword detection features.
- </message>
- <message name="IDS_FLAGS_DISABLE_EXPERIMENTAL_HOTWORDING_DESCRIPTION"
desc="Description of about:flags option for hotword detection.">
- Disables the newest version of 'Ok Google' hotword detection, such
as using the built-in extension. The hotword extension from the Chrome Web
Store will be used instead.
- </message>
-
- <!-- Built-in hotword detection display strings -->
<message name="IDS_FLAGS_ENABLE_EXPERIMENTAL_HOTWORD_HARDWARE_NAME"
desc="Name of about:flags option for hotword hardware detection.">
Enable simulated hardware 'Ok Google' features.
</message>
Index: chrome/browser/about_flags.cc
diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc
index
8321571e8a6dc1f6d4a976c1cc2d24d063d53dfc..e78df20e47e0eb397f0aef455bee30671c0b263b
100644
--- a/chrome/browser/about_flags.cc
+++ b/chrome/browser/about_flags.cc
@@ -1900,13 +1900,6 @@ const Experiment kExperiments[] = {
},
#endif
{
- "disable-experimental-hotwording",
- IDS_FLAGS_DISABLE_EXPERIMENTAL_HOTWORDING_NAME,
- IDS_FLAGS_DISABLE_EXPERIMENTAL_HOTWORDING_DESCRIPTION,
- kOsDesktop,
- SINGLE_VALUE_TYPE(switches::kDisableExperimentalHotwording)
- },
- {
"enable-hotword-hardware",
IDS_FLAGS_ENABLE_EXPERIMENTAL_HOTWORD_HARDWARE_NAME,
IDS_FLAGS_ENABLE_EXPERIMENTAL_HOTWORD_HARDWARE_DESCRIPTION,
Index: chrome/browser/extensions/hotword_browsertest.cc
diff --git a/chrome/browser/extensions/hotword_browsertest.cc
b/chrome/browser/extensions/hotword_browsertest.cc
index
753a7981c20ed351293edd39843eea6d9d238cc2..dcde7180b194feb0e4edc1507b31b0d06392d4c0
100644
--- a/chrome/browser/extensions/hotword_browsertest.cc
+++ b/chrome/browser/extensions/hotword_browsertest.cc
@@ -35,9 +35,6 @@ class HotwordBrowserTest : public ExtensionBrowserTest {
// extension.
base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
switches::kForceFieldTrials, "VoiceTrigger/Install/");
- // This test is only valid with version 1 of hotwording.
- base::CommandLine::ForCurrentProcess()->AppendSwitch(
- switches::kDisableExperimentalHotwording);
// Load the hotword_helper extension.
ComponentLoader::EnableBackgroundExtensionsForTesting();

@@ -70,7 +67,9 @@ class HotwordBrowserTest : public ExtensionBrowserTest {
// Test we silently capture an exception from a message handler's response
// callback. This happens when the caller to chrome.runtime.sendMessage()
// doesn't specify a response callback.
-IN_PROC_BROWSER_TEST_F(HotwordBrowserTest, MessageSendResponseError) {
+// NOTE(amistry): Test is disabled instead of deleted since the
functionality
+// may still be required to implement crbug.com/436681
+IN_PROC_BROWSER_TEST_F(HotwordBrowserTest,
DISABLED_MessageSendResponseError) {
// Enable error reporting for the hotword helper extension.
error_console()->SetReportingAllForExtension(kHotwordHelperExtensionId,
true);

Index: chrome/browser/search/hotword_service.cc
diff --git a/chrome/browser/search/hotword_service.cc
b/chrome/browser/search/hotword_service.cc
index
ee90a544a30cc0b8a7d69083acafa8a04059ea90..2a53c1bb4d5a6e5f8b1131353c134927824ace46
100644
--- a/chrome/browser/search/hotword_service.cc
+++ b/chrome/browser/search/hotword_service.cc
@@ -221,7 +221,6 @@ namespace hotword_internal {
// Constants for the hotword field trial.
const char kHotwordFieldTrialName[] = "VoiceTrigger";
const char kHotwordFieldTrialDisabledGroupName[] = "Disabled";
-const char kHotwordFieldTrialExperimentalGroupName[] = "Experimental";
// Old preference constant.
const char kHotwordUnusablePrefName[] = "hotword.search_enabled";
// String passed to indicate the training state has changed.
@@ -296,15 +295,7 @@ bool
HotwordService::DoesHotwordSupportLanguage(Profile* profile) {

// static
bool HotwordService::IsExperimentalHotwordingEnabled() {
- std::string group = base::FieldTrialList::FindFullName(
- hotword_internal::kHotwordFieldTrialName);
- if (!group.empty() &&
- group == hotword_internal::kHotwordFieldTrialExperimentalGroupName) {
- return true;
- }
-
- base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
-
return !command_line->HasSwitch(switches::kDisableExperimentalHotwording);
+ return true;
}

#if defined(OS_CHROMEOS)
Index: chrome/browser/search/hotword_service.h
diff --git a/chrome/browser/search/hotword_service.h
b/chrome/browser/search/hotword_service.h
index
9a93efb330a757fff7e261e272adc909ed0ba724..2c0e086ba12c99f80ae2c522cc5e681ecea7649f
100644
--- a/chrome/browser/search/hotword_service.h
+++ b/chrome/browser/search/hotword_service.h
@@ -60,7 +60,8 @@ class HotwordService : public
extensions::ExtensionRegistryObserver,
// Returns true if the hotword supports the current system language.
static bool DoesHotwordSupportLanguage(Profile* profile);

- // Returns true if the "enable-experimental-hotwording" flag is set.
+ // Always returns true.
+ // TODO(amistry): Remove this.
static bool IsExperimentalHotwordingEnabled();

explicit HotwordService(Profile* profile);
Index: chrome/browser/search/hotword_service_unittest.cc
diff --git a/chrome/browser/search/hotword_service_unittest.cc
b/chrome/browser/search/hotword_service_unittest.cc
index
ee68d692030749f7eb3695ad5696a3666808d4e5..491bb7fd623a26a1fbe67768da5fcfa76e0f5b81
100644
--- a/chrome/browser/search/hotword_service_unittest.cc
+++ b/chrome/browser/search/hotword_service_unittest.cc
@@ -133,10 +133,6 @@ class HotwordServiceTest :

void SetUp() override {
extension_id_ = GetParam();
- if (extension_id_ == extension_misc::kHotwordExtensionId) {
- base::CommandLine::ForCurrentProcess()->AppendSwitch(
- switches::kDisableExperimentalHotwording);
- }
#if defined(OS_CHROMEOS)
// Tests on chromeos need to have the handler initialized.
chromeos::CrasAudioHandler::InitializeForTesting();
@@ -159,7 +155,6 @@ class HotwordServiceTest :
INSTANTIATE_TEST_CASE_P(HotwordServiceTests,
HotwordServiceTest,
::testing::Values(
- extension_misc::kHotwordExtensionId,
extension_misc::kHotwordSharedModuleId));

TEST_P(HotwordServiceTest, IsHotwordAllowedBadFieldTrial) {
Index: chrome/common/chrome_switches.cc
diff --git a/chrome/common/chrome_switches.cc
b/chrome/common/chrome_switches.cc
index
083dcc15fd2dad5cb31e848baf2b2044a0ad5c62..7569745f36676f54693e62ba001fede6e004e6c3
100644
--- a/chrome/common/chrome_switches.cc
+++ b/chrome/common/chrome_switches.cc
@@ -426,11 +426,6 @@ const char kEnhancedBookmarksExperiment[]
= "enhanced-bookmarks-experiment";
const char kEnableEphemeralAppsInWebstore[] =
"enable-ephemeral-apps-in-webstore";

-// Disables v2 hotword detection features. These features include
-// using a new component extension for performing hotword detection, new UI
-// flows, and always-on detection.
-const char kDisableExperimentalHotwording[]
= "disable-experimental-hotwording";
-
// Enables experimental hotword features specific to always-on.
const char kEnableExperimentalHotwordHardware[]
= "enable-hotword-hardware";

Index: chrome/common/chrome_switches.h
diff --git a/chrome/common/chrome_switches.h
b/chrome/common/chrome_switches.h
index
ec699414ef0e4f9cfdb8c44cb9a36b68224d2704..18e0d5b2ab5ead159a5f66eb9790f2fe8d2cbb15
100644
--- a/chrome/common/chrome_switches.h
+++ b/chrome/common/chrome_switches.h
@@ -126,7 +126,6 @@ extern const char kEnhancedBookmarksExperiment[];
extern const char kEnableDomainReliability[];
extern const char kEnableDownloadNotification[];
extern const char kEnableEphemeralAppsInWebstore[];
-extern const char kDisableExperimentalHotwording[];
extern const char kEnableExperimentalHotwordHardware[];
extern const char kEnableExtensionActivityLogging[];
extern const char kEnableExtensionActivityLogTesting[];

[mgi...@chromium.org]

As discussed, if you are planning to remove all of the other code for M43, I
would just wait until you are sure you don't need it, and delete everything
in
one CL (or two CLs close to each other, if you really want to).

I don't think it's a good idea to leave all of these TODOs and disabled
tests
and dangling unreachable code around for an indefinite period of time.

https://codereview.chromium.org/981773003/

[ami...@chromium.org]

On 2015/03/10 06:18:16, Matt Giuca wrote:
> As discussed, if you are planning to remove all of the other code for
> M43, I
> would just wait until you are sure you don't need it, and delete
> everything in
> one CL (or two CLs close to each other, if you really want to).

> I don't think it's a good idea to leave all of these TODOs and disabled
> tests
> and dangling unreachable code around for an indefinite period of time.

Well, the follow-up CL which deletes everything is ready to go
(https://codereview.chromium.org/992173002/). I just figured it would be
better
on reviewers to split into two logical parts.

https://codereview.chromium.org/981773003/

[mgi...@chromium.org]

OK, lgtm, assuming the next one is going to land in the next few days (I
will
look at it shortly).

https://codereview.chromium.org/981773003/

[ami...@chromium.org]

the...@chromium.org: For OWNERS.



https://codereview.chromium.org/981773003/

[the...@chromium.org]

lgtm



https://codereview.chromium.org/981773003/

[kcara...@chromium.org]

On 2015/03/11 23:53:21, Lei Zhang wrote:
> lgtm

This cl is also removing the field trial check. Don't you have that in a
separate cl?

https://codereview.chromium.org/981773003/

[ami...@chromium.org]

On 2015/03/12 03:44:22, kcarattini wrote:
> On 2015/03/11 23:53:21, Lei Zhang wrote:
> > lgtm

> This cl is also removing the field trial check. Don't you have that in a
> separate cl?

Good point. Then again, if I submit the other one first, the merge conflict
is
trivial and semantically irrelevant.

https://codereview.chromium.org/981773003/

[kcara...@chromium.org]

lgtm

I'd just like the cl description to accurately reflect which cl it's in. If
the
other change is already submitted, then no worries.

https://codereview.chromium.org/981773003/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/981773003/60001


https://codereview.chromium.org/981773003/

[commi...@chromium.org]

Committed patchset #4 (id:60001)

https://codereview.chromium.org/981773003/

[commi...@chromium.org]

Patchset 4 (id:??) landed as
https://crrev.com/5ded0c90a0d0b68a5b467cc9abcf3a0396d45b9c
Cr-Commit-Position: refs/heads/master@{#320843}

https://codereview.chromium.org/981773003/