Commit 32ee967a authored by Mark Pearson's avatar Mark Pearson Committed by Commit Bot

Revert "assistant: fix race condition on showing hotword options"

This reverts commit b3bb875d.

Reason for revert:

Causes deterministic failures on bot
https://ci.chromium.org/p/chromium/builders/ci/linux-chromeos-dbg
of test
CrSettingsGoogleAssistantPageTest.All

More details at http://crbug.com/983568


Original change's description:
> assistant: fix race condition on showing hotword options
> 
> Hotword device detection may take some time for cras. Assistant
> settings page maybe showing before cras has fully discovered all
> audio devices.  This change makes sure we handle cras audio
> device node changes to update with latest information.
> 
> Bug: b/135137356
> Test: locally build and test on device.
> Change-Id: Iea84cd3a9e7e25b324014242799ac88f372a099e
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1687363
> Reviewed-by: Yue Li <updowndota@chromium.org>
> Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
> Commit-Queue: Xiaohui Chen <xiaohuic@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#675922}

TBR=stevenjb@chromium.org,xiaohuic@chromium.org,updowndota@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: b/135137356
Change-Id: I2dd5ee9af4b777d71e3954365796421010525c99
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1699509Reviewed-by: default avatarMark Pearson <mpearson@chromium.org>
Commit-Queue: Mark Pearson <mpearson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#677036}
parent 301fe942
...@@ -41,7 +41,7 @@ const ConsentStatus = { ...@@ -41,7 +41,7 @@ const ConsentStatus = {
Polymer({ Polymer({
is: 'settings-google-assistant-page', is: 'settings-google-assistant-page',
behaviors: [I18nBehavior, PrefsBehavior, WebUIListenerBehavior], behaviors: [I18nBehavior, PrefsBehavior],
properties: { properties: {
/** @private */ /** @private */
...@@ -124,15 +124,6 @@ Polymer({ ...@@ -124,15 +124,6 @@ Polymer({
this.browserProxy_ = settings.GoogleAssistantBrowserProxyImpl.getInstance(); this.browserProxy_ = settings.GoogleAssistantBrowserProxyImpl.getInstance();
}, },
/** @override */
ready: function() {
this.addWebUIListener('hotwordDeviceUpdated', (hasHotword) => {
this.hotwordDspAvailable_ = hasHotword;
});
chrome.send('initializeGoogleAssistantPage');
},
/** /**
* @param {boolean} toggleValue * @param {boolean} toggleValue
* @return {string} * @return {string}
......
...@@ -203,7 +203,7 @@ void AssistantOptInFlowScreenHandler::OnSpeakerIdEnrollmentFailure() { ...@@ -203,7 +203,7 @@ void AssistantOptInFlowScreenHandler::OnSpeakerIdEnrollmentFailure() {
RecordAssistantOptInStatus(VOICE_MATCH_ENROLLMENT_ERROR); RecordAssistantOptInStatus(VOICE_MATCH_ENROLLMENT_ERROR);
CallJS("login.AssistantOptInFlowScreen.onVoiceMatchUpdate", CallJS("login.AssistantOptInFlowScreen.onVoiceMatchUpdate",
base::Value("failure")); base::Value("failure"));
LOG(ERROR) << "Speaker ID enrollment failure."; LOG(ERROR) << "Speaker ID enrollmend failure.";
} }
void AssistantOptInFlowScreenHandler::SetupAssistantConnection() { void AssistantOptInFlowScreenHandler::SetupAssistantConnection() {
......
...@@ -13,7 +13,6 @@ ...@@ -13,7 +13,6 @@
#include "base/values.h" #include "base/values.h"
#include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/webui/chromeos/assistant_optin/assistant_optin_ui.h" #include "chrome/browser/ui/webui/chromeos/assistant_optin/assistant_optin_ui.h"
#include "chromeos/audio/cras_audio_handler.h"
#include "chromeos/constants/chromeos_switches.h" #include "chromeos/constants/chromeos_switches.h"
#include "chromeos/services/assistant/public/mojom/constants.mojom.h" #include "chromeos/services/assistant/public/mojom/constants.mojom.h"
#include "components/arc/arc_prefs.h" #include "components/arc/arc_prefs.h"
...@@ -26,34 +25,13 @@ namespace chromeos { ...@@ -26,34 +25,13 @@ namespace chromeos {
namespace settings { namespace settings {
GoogleAssistantHandler::GoogleAssistantHandler(Profile* profile) GoogleAssistantHandler::GoogleAssistantHandler(Profile* profile)
: profile_(profile), weak_factory_(this) { : profile_(profile), weak_factory_(this) {}
chromeos::CrasAudioHandler::Get()->AddAudioObserver(this);
}
GoogleAssistantHandler::~GoogleAssistantHandler() {
chromeos::CrasAudioHandler::Get()->RemoveAudioObserver(this);
}
void GoogleAssistantHandler::OnJavascriptAllowed() { GoogleAssistantHandler::~GoogleAssistantHandler() {}
if (pending_hotword_update_) {
OnAudioNodesChanged();
}
}
void GoogleAssistantHandler::OnJavascriptAllowed() {}
void GoogleAssistantHandler::OnJavascriptDisallowed() {} void GoogleAssistantHandler::OnJavascriptDisallowed() {}
void GoogleAssistantHandler::OnAudioNodesChanged() {
if (!IsJavascriptAllowed()) {
pending_hotword_update_ = true;
return;
}
pending_hotword_update_ = false;
FireWebUIListener(
"hotwordDeviceUpdated",
base::Value(chromeos::CrasAudioHandler::Get()->HasHotwordDevice()));
}
void GoogleAssistantHandler::RegisterMessages() { void GoogleAssistantHandler::RegisterMessages() {
web_ui()->RegisterMessageCallback( web_ui()->RegisterMessageCallback(
"showGoogleAssistantSettings", "showGoogleAssistantSettings",
...@@ -68,10 +46,6 @@ void GoogleAssistantHandler::RegisterMessages() { ...@@ -68,10 +46,6 @@ void GoogleAssistantHandler::RegisterMessages() {
"syncVoiceModelStatus", "syncVoiceModelStatus",
base::BindRepeating(&GoogleAssistantHandler::HandleSyncVoiceModelStatus, base::BindRepeating(&GoogleAssistantHandler::HandleSyncVoiceModelStatus,
base::Unretained(this))); base::Unretained(this)));
web_ui()->RegisterMessageCallback(
"initializeGoogleAssistantPage",
base::BindRepeating(&GoogleAssistantHandler::HandleInitialized,
base::Unretained(this)));
} }
void GoogleAssistantHandler::HandleShowGoogleAssistantSettings( void GoogleAssistantHandler::HandleShowGoogleAssistantSettings(
...@@ -97,11 +71,6 @@ void GoogleAssistantHandler::HandleSyncVoiceModelStatus( ...@@ -97,11 +71,6 @@ void GoogleAssistantHandler::HandleSyncVoiceModelStatus(
settings_manager_->SyncSpeakerIdEnrollmentStatus(); settings_manager_->SyncSpeakerIdEnrollmentStatus();
} }
void GoogleAssistantHandler::HandleInitialized(const base::ListValue* args) {
CHECK_EQ(0U, args->GetSize());
AllowJavascript();
}
void GoogleAssistantHandler::BindAssistantSettingsManager() { void GoogleAssistantHandler::BindAssistantSettingsManager() {
DCHECK(!settings_manager_.is_bound()); DCHECK(!settings_manager_.is_bound());
......
...@@ -6,9 +6,7 @@ ...@@ -6,9 +6,7 @@
#define CHROME_BROWSER_UI_WEBUI_SETTINGS_CHROMEOS_GOOGLE_ASSISTANT_HANDLER_H_ #define CHROME_BROWSER_UI_WEBUI_SETTINGS_CHROMEOS_GOOGLE_ASSISTANT_HANDLER_H_
#include "base/macros.h" #include "base/macros.h"
#include "base/optional.h"
#include "chrome/browser/ui/webui/settings/settings_page_ui_handler.h" #include "chrome/browser/ui/webui/settings/settings_page_ui_handler.h"
#include "chromeos/audio/cras_audio_handler.h"
#include "chromeos/services/assistant/public/mojom/settings.mojom.h" #include "chromeos/services/assistant/public/mojom/settings.mojom.h"
#include "mojo/public/cpp/bindings/binding.h" #include "mojo/public/cpp/bindings/binding.h"
...@@ -17,8 +15,7 @@ class Profile; ...@@ -17,8 +15,7 @@ class Profile;
namespace chromeos { namespace chromeos {
namespace settings { namespace settings {
class GoogleAssistantHandler : public ::settings::SettingsPageUIHandler, class GoogleAssistantHandler : public ::settings::SettingsPageUIHandler {
chromeos::CrasAudioHandler::AudioObserver {
public: public:
explicit GoogleAssistantHandler(Profile* profile); explicit GoogleAssistantHandler(Profile* profile);
~GoogleAssistantHandler() override; ~GoogleAssistantHandler() override;
...@@ -27,9 +24,6 @@ class GoogleAssistantHandler : public ::settings::SettingsPageUIHandler, ...@@ -27,9 +24,6 @@ class GoogleAssistantHandler : public ::settings::SettingsPageUIHandler,
void OnJavascriptAllowed() override; void OnJavascriptAllowed() override;
void OnJavascriptDisallowed() override; void OnJavascriptDisallowed() override;
// chromeos::CrasAudioHandler::AudioObserver overrides
void OnAudioNodesChanged() override;
private: private:
// WebUI call to launch into the Google Assistant app settings. // WebUI call to launch into the Google Assistant app settings.
void HandleShowGoogleAssistantSettings(const base::ListValue* args); void HandleShowGoogleAssistantSettings(const base::ListValue* args);
...@@ -37,8 +31,6 @@ class GoogleAssistantHandler : public ::settings::SettingsPageUIHandler, ...@@ -37,8 +31,6 @@ class GoogleAssistantHandler : public ::settings::SettingsPageUIHandler,
void HandleRetrainVoiceModel(const base::ListValue* args); void HandleRetrainVoiceModel(const base::ListValue* args);
// WebUI call to sync Assistant voice model status. // WebUI call to sync Assistant voice model status.
void HandleSyncVoiceModelStatus(const base::ListValue* args); void HandleSyncVoiceModelStatus(const base::ListValue* args);
// WebUI call to signal js side is ready.
void HandleInitialized(const base::ListValue* args);
// Bind to assistant settings manager. // Bind to assistant settings manager.
void BindAssistantSettingsManager(); void BindAssistantSettingsManager();
...@@ -47,8 +39,6 @@ class GoogleAssistantHandler : public ::settings::SettingsPageUIHandler, ...@@ -47,8 +39,6 @@ class GoogleAssistantHandler : public ::settings::SettingsPageUIHandler,
assistant::mojom::AssistantSettingsManagerPtr settings_manager_; assistant::mojom::AssistantSettingsManagerPtr settings_manager_;
bool pending_hotword_update_ = false;
base::WeakPtrFactory<GoogleAssistantHandler> weak_factory_; base::WeakPtrFactory<GoogleAssistantHandler> weak_factory_;
DISALLOW_COPY_AND_ASSIGN(GoogleAssistantHandler); DISALLOW_COPY_AND_ASSIGN(GoogleAssistantHandler);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment