Implement remaining chrome.searchEnginesPrivate methods.

chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc

Index: chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc
diff --git a/chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc b/chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc
index 6fc7e43bd37986da7b7e0778f079fdc4a1fc09a1..dae85e7c0753ee244d352a201ecd54f9a8937c69 100644
--- a/chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc
+++ b/chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc
@@ -4,17 +4,28 @@
 
 #include "chrome/browser/extensions/api/search_engines_private/search_engines_private_api.h"
 
+#include "base/prefs/pref_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/chrome_extension_function.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/search/hotword_audio_history_handler.h"
+#include "chrome/browser/search/hotword_service.h"
+#include "chrome/browser/search/hotword_service_factory.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
-#include "chrome/common/extensions/api/search_engines_private.h"
+#include "chrome/browser/signin/signin_manager_factory.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/common/url_constants.h"
+#include "chrome/grit/generated_resources.h"
 #include "components/search_engines/template_url_service.h"
+#include "components/signin/core/browser/signin_manager_base.h"
 #include "extensions/browser/extension_function_registry.h"
+#include "ui/base/l10n/l10n_util.h"
 
 namespace extensions {
 
+namespace search_engines_private = api::search_engines_private;
+
 ////////////////////////////////////////////////////////////////////////////////
 // SearchEnginesPrivateGetDefaultSearchEnginesFunction
 
@@ -37,9 +48,14 @@ SearchEnginesPrivateGetDefaultSearchEnginesFunction::Run() {
       template_url_service->GetDefaultSearchProvider();
   std::vector<TemplateURL*> urls = template_url_service->GetTemplateURLs();
   for (size_t i = 0; i < urls.size(); i++) {
-    api::search_engines_private::SearchEngine engine;
+    if (!urls[i]->show_in_default_list())
+      continue;

[stevenjb, 2015/04/28 17:16:38]

If the data model differentiates between "default" and "other" with a boolean
(rather than using separate lists), it might make sense for the Extension API to
do the same thing, i.e. put the list separation in the UI. That way if the UX
folks decide to combine the lists we don't have to re-merge the lists. Also it
makes the API a bit simpler.

This is just a thought/suggestion. I don't want to slow things down, but I do
prefer simpler APIs that follow the data model (instead of a particular UX
model).

[Oren Blasberg, 2015/04/28 21:58:30]

Done.

+
+    search_engines_private::SearchEngine engine;
     engine.guid = urls[i]->sync_guid();
     engine.name = base::UTF16ToASCII(urls[i]->short_name());
+    engine.keyword = base::UTF16ToASCII(urls[i]->keyword());
+    engine.url = urls[i]->url();
     if (urls[i] == default_url)
       engine.is_selected = scoped_ptr<bool>(new bool(true));
 
@@ -63,9 +79,9 @@ SearchEnginesPrivateSetSelectedSearchEngineFunction::
 
 ExtensionFunction::ResponseAction
 SearchEnginesPrivateSetSelectedSearchEngineFunction::Run() {
-  scoped_ptr<api::search_engines_private::SetSelectedSearchEngine::Params>
+  scoped_ptr<search_engines_private::SetSelectedSearchEngine::Params>
       parameters =
-          api::search_engines_private::SetSelectedSearchEngine::Params::Create(
+          search_engines_private::SetSelectedSearchEngine::Params::Create(
               *args_);
   EXTENSION_FUNCTION_VALIDATE(parameters.get());
 
@@ -76,4 +92,283 @@ SearchEnginesPrivateSetSelectedSearchEngineFunction::Run() {
   return RespondNow(NoArguments());
 }
 
+////////////////////////////////////////////////////////////////////////////////
+// SearchEnginesPrivateGetOtherSearchEnginesFunction
+
+SearchEnginesPrivateGetOtherSearchEnginesFunction::
+    SearchEnginesPrivateGetOtherSearchEnginesFunction()
+    : chrome_details_(this) {
+}
+
+SearchEnginesPrivateGetOtherSearchEnginesFunction::
+    ~SearchEnginesPrivateGetOtherSearchEnginesFunction() {
+}
+
+ExtensionFunction::ResponseAction
+SearchEnginesPrivateGetOtherSearchEnginesFunction::Run() {
+  TemplateURLService* template_url_service =
+      TemplateURLServiceFactory::GetForProfile(chrome_details_.GetProfile());
+  base::ListValue* engines = new base::ListValue();
+
+  std::vector<TemplateURL*> urls = template_url_service->GetTemplateURLs();
+  for (size_t i = 0; i < urls.size(); i++) {
+    if (urls[i]->show_in_default_list())
+      continue;

[stevenjb, 2015/04/28 17:16:39]

If we do decide to keep this as two separate APIs we should at least move the
copy/pasted code into a common helper method that takes 'get_default' as a
boolean.

[Oren Blasberg, 2015/04/28 21:58:30]

Ended up combining them.

+
+    search_engines_private::SearchEngine engine;
+    engine.guid = urls[i]->sync_guid();
+    engine.name = base::UTF16ToASCII(urls[i]->short_name());
+    engine.keyword = base::UTF16ToASCII(urls[i]->keyword());
+    engine.url = urls[i]->url();
+
+    engines->Append(engine.ToValue().release());
+  }
+
+  return RespondNow(OneArgument(engines));
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// SearchEnginesPrivateAddOtherSearchEngineFunction
+
+SearchEnginesPrivateAddOtherSearchEngineFunction::
+    SearchEnginesPrivateAddOtherSearchEngineFunction()
+    : chrome_details_(this) {
+}
+
+SearchEnginesPrivateAddOtherSearchEngineFunction::
+    ~SearchEnginesPrivateAddOtherSearchEngineFunction() {
+}
+
+ExtensionFunction::ResponseAction
+SearchEnginesPrivateAddOtherSearchEngineFunction::Run() {
+  scoped_ptr<search_engines_private::AddOtherSearchEngine::Params> parameters =
+      search_engines_private::AddOtherSearchEngine::Params::Create(*args_);
+  EXTENSION_FUNCTION_VALIDATE(parameters.get());
+
+  TemplateURLData data;
+  data.short_name = base::ASCIIToUTF16(parameters->name);
+  data.SetKeyword(base::ASCIIToUTF16(parameters->keyword));
+  data.SetURL(parameters->url);
+  TemplateURL* turl = new TemplateURL(data);
+
+  TemplateURLService* template_url_service =
+      TemplateURLServiceFactory::GetForProfile(chrome_details_.GetProfile());
+  template_url_service->Add(turl);
+  return RespondNow(NoArguments());
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// SearchEnginesPrivateUpdateSearchEngineFunction
+
+SearchEnginesPrivateUpdateSearchEngineFunction::
+    SearchEnginesPrivateUpdateSearchEngineFunction()
+    : chrome_details_(this) {
+}
+
+SearchEnginesPrivateUpdateSearchEngineFunction::
+    ~SearchEnginesPrivateUpdateSearchEngineFunction() {
+}
+
+ExtensionFunction::ResponseAction
+SearchEnginesPrivateUpdateSearchEngineFunction::Run() {
+  scoped_ptr<search_engines_private::UpdateSearchEngine::Params> parameters =
+      search_engines_private::UpdateSearchEngine::Params::Create(*args_);
+  EXTENSION_FUNCTION_VALIDATE(parameters.get());
+
+  TemplateURLService* template_url_service =
+      TemplateURLServiceFactory::GetForProfile(chrome_details_.GetProfile());
+  TemplateURL* turl =
+      template_url_service->GetTemplateURLForGUID(parameters->guid);
+
+  template_url_service->ResetTemplateURL(
+      turl, base::ASCIIToUTF16(parameters->name),
+      base::ASCIIToUTF16(parameters->keyword), parameters->url);
+
+  return RespondNow(NoArguments());
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// SearchEnginesPrivateRemoveSearchEngineFunction
+
+SearchEnginesPrivateRemoveSearchEngineFunction::
+    SearchEnginesPrivateRemoveSearchEngineFunction()
+    : chrome_details_(this) {
+}
+
+SearchEnginesPrivateRemoveSearchEngineFunction::
+    ~SearchEnginesPrivateRemoveSearchEngineFunction() {
+}
+
+ExtensionFunction::ResponseAction
+SearchEnginesPrivateRemoveSearchEngineFunction::Run() {
+  scoped_ptr<search_engines_private::RemoveSearchEngine::Params> parameters =
+      search_engines_private::RemoveSearchEngine::Params::Create(*args_);
+  EXTENSION_FUNCTION_VALIDATE(parameters.get());
+
+  TemplateURLService* template_url_service =
+      TemplateURLServiceFactory::GetForProfile(chrome_details_.GetProfile());
+  TemplateURL* turl =
+      template_url_service->GetTemplateURLForGUID(parameters->guid);
+  template_url_service->Remove(turl);
+  return RespondNow(NoArguments());
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// SearchEnginesPrivateGetHotwordStateFunction
+
+SearchEnginesPrivateGetHotwordStateFunction::
+    SearchEnginesPrivateGetHotwordStateFunction()
+    : chrome_details_(this), weak_ptr_factory_(this) {
+}
+
+SearchEnginesPrivateGetHotwordStateFunction::
+    ~SearchEnginesPrivateGetHotwordStateFunction() {
+}
+
+ExtensionFunction::ResponseAction
+SearchEnginesPrivateGetHotwordStateFunction::Run() {
+  Profile* profile = chrome_details_.GetProfile();
+  TemplateURLService* template_url_service =
+      TemplateURLServiceFactory::GetForProfile(profile);
+
+  scoped_ptr<search_engines_private::HotwordState> state(
+      new search_engines_private::HotwordState());
+
+  bool is_search_provider_google = false;
+  if (template_url_service && template_url_service->loaded()) {
+    const TemplateURL* default_url =
+        template_url_service->GetDefaultSearchProvider();
+    if (default_url &&
+        default_url->HasGoogleBaseURLs(
+            template_url_service->search_terms_data())) {
+      is_search_provider_google = true;
+    } else {
+      // If the user has chosen a default search provide other than Google, turn
+      // off hotwording since other providers don't provide that functionality.
+      HotwordService* hotword_service =
+          HotwordServiceFactory::GetForProfile(profile);
+      if (hotword_service)
+        hotword_service->DisableHotwordPreferences();

[stevenjb, 2015/04/28 17:16:38]

This seems like a strange place to do this; effectively we can end up disabling
hotwording as a side-effect of a GetHotwordState call.

Either this should be removed from here if it isn't necessary, or this entire
clause should be wrapped into a helper function somewhere. You may need to get
in touch with the maintainer(s) of the HotwordService code.

[Oren Blasberg, 2015/04/28 18:54:26]

disclaimer: Lots of this code is copied/adapted from BrowserOptionsHandler.

Yeah, I can see why the getter having mutation effects would be weird.
FWIW, My intent here was that we would call GetHotwordState whenever any of the
default search engines changes. That way if it changes from google to something
else, this code will disable the hotword prefs for us neatly.

Does that seem like something that should happen in the JS instead? We'd need a
method for disabling hotword preferences (it's not quite as straightforward as
"turn off these 2 prefs"; else we'd just use settings private. Check out [1]  )

We'd also need a js method for determining if the current search engine 'has
google base urls' (we could add an API method for that too)

Thoughts?


[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sea...

+    }
+  }
+
+  if (!HotwordServiceFactory::IsHotwordAllowed(profile) ||
+      !is_search_provider_google) {
+    return RespondNow(OneArgument(state->ToValue().release()));

[stevenjb, 2015/04/28 17:16:38]

It looks like we could replace the clause above with:

if (!default_url || !default_url->HasGoogleBasedURLs(...) ||
!HotwordServiceFactory::IsHotwordAllowed(profile))
  return RespondNow(OneArgument(state->ToValue().release()));
}

and eliminate lines 245-252?

[Oren Blasberg, 2015/04/28 21:58:30]

I don't quite see how this is equivalent. Which clause are you proposing to
replace? I think maybe I'm confused because having a bunch of !'s in a row make
it a bit harder to reasona about the logic.

In any case I removed the "else" above because we decided to pull disabling
hotword preferences out of the purview of the extension api code. 

+  }
+
+  state->availability.push_back(
+      search_engines_private::HotwordFeature::HOTWORD_FEATURE_SEARCH);
+
+  SigninManagerBase* signin = SigninManagerFactory::GetForProfile(profile);
+  bool authenticated = signin && signin->IsAuthenticated();
+  bool always_on =
+      HotwordServiceFactory::IsAlwaysOnAvailable() && authenticated;
+
+  if (always_on) {
+    state->availability.push_back(
+        search_engines_private::HotwordFeature::HOTWORD_FEATURE_ALWAYS_ON);
+    if (profile->GetPrefs()->GetBoolean(prefs::kHotwordAlwaysOnSearchEnabled)) {
+      state->availability.push_back(
+          search_engines_private::HotwordFeature::HOTWORD_FEATURE_RETRAIN_LINK);
+    }
+  }
+
+  int error = HotwordServiceFactory::GetCurrentError(profile);
+  if (error) {
+    base::string16 hotword_help_url =
+        base::ASCIIToUTF16(chrome::kHotwordLearnMoreURL);
+    std::string error_message(l10n_util::GetStringUTF8(error));
+    if (error == IDS_HOTWORD_GENERIC_ERROR_MESSAGE) {
+      error_message = l10n_util::GetStringFUTF8(error, hotword_help_url);
+    }
+    state->error_msg = scoped_ptr<std::string>(&error_message);

[stevenjb, 2015/04/28 17:16:38]

state->error_msg.reset(new std::string(error_message));

As written... you're passing the address of a local variable to a scoped_ptr.

Alternatively you could do:
scoped_ptr<std::string> error_message(...(error));
...
state->error_msg = error_message.Pass();

[Oren Blasberg, 2015/04/28 18:44:00]

Done.

+  }
+
+  // Audio history should be displayed if it's enabled regardless of the
+  // hotword error state if the user is signed in. If the user is not signed
+  // in, audio history is meaningless. This is only displayed if always-on
+  // hotwording is available.
+  if (authenticated && always_on) {
+    std::string user_display_name = signin->GetAuthenticatedUsername();
+    DCHECK(!user_display_name.empty());
+    base::string16 audio_history_state =
+        l10n_util::GetStringFUTF16(IDS_HOTWORD_AUDIO_HISTORY_ENABLED,
+                                   base::ASCIIToUTF16(user_display_name));
+    HotwordService* hotword_service =
+        HotwordServiceFactory::GetForProfile(profile);
+    if (hotword_service) {
+      hotword_service->GetAudioHistoryHandler()->GetAudioHistoryEnabled(
+          base::Bind(&SearchEnginesPrivateGetHotwordStateFunction::
+                         OnAudioHistoryChecked,
+                     weak_ptr_factory_.GetWeakPtr(), base::Passed(&state),
+                     audio_history_state));
+      return RespondLater();
+    }
+  }
+
+  return RespondNow(OneArgument(state->ToValue().release()));
+}
+
+void SearchEnginesPrivateGetHotwordStateFunction::OnAudioHistoryChecked(
+    scoped_ptr<search_engines_private::HotwordState> state,
+    const base::string16& audio_history_state,
+    bool success,
+    bool logging_enabled) {
+  if (success && logging_enabled) {
+    state->availability.push_back(
+        search_engines_private::HotwordFeature::HOTWORD_FEATURE_AUDIO_HISTORY);
+    std::string audio_history_state_str(
+        base::UTF16ToASCII(audio_history_state));
+    state->audio_history_state = make_scoped_ptr(&audio_history_state_str);
+  }
+  Respond(OneArgument(state->ToValue().release()));
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// SearchEnginesPrivateOptIntoHotwordingFunction
+
+SearchEnginesPrivateOptIntoHotwordingFunction::
+    SearchEnginesPrivateOptIntoHotwordingFunction()
+    : chrome_details_(this) {
+}
+
+SearchEnginesPrivateOptIntoHotwordingFunction::
+    ~SearchEnginesPrivateOptIntoHotwordingFunction() {
+}
+
+ExtensionFunction::ResponseAction
+SearchEnginesPrivateOptIntoHotwordingFunction::Run() {
+  scoped_ptr<search_engines_private::OptIntoHotwording::Params> parameters =
+      search_engines_private::OptIntoHotwording::Params::Create(*args_);
+  EXTENSION_FUNCTION_VALIDATE(parameters.get());
+
+  Profile* profile = chrome_details_.GetProfile();
+
+  HotwordService::LaunchMode launch_mode =
+      HotwordService::HOTWORD_AND_AUDIO_HISTORY;
+
+  if (parameters->retrain) {
+    DCHECK(
+        profile->GetPrefs()->GetBoolean(prefs::kHotwordAlwaysOnSearchEnabled));
+    DCHECK(profile->GetPrefs()->GetBoolean(prefs::kHotwordAudioLoggingEnabled));

[stevenjb, 2015/04/28 17:16:39]

Since this is an API we should set an error and fail if either of these are
false, not crash on Debug and proceed on Release.

[Oren Blasberg, 2015/04/28 18:44:00]

Done (setting error) -- Q: How do I "fail"? I can't return false since this is a
UIThreadExtensionFunction not an Async one. Also, this function doesn't take a
success or failure callback as argument.

[stevenjb, 2015/04/28 19:06:03]

I think you just set 'error_ = errordesc' then SendResponse(false). That's what
we do for async methods in networking_private_api.cc. You might need to look
around for the right synchronous pattern.

[Oren Blasberg, 2015/04/28 21:58:30]

Done.

+
+    launch_mode = HotwordService::RETRAIN;
+  } else if (profile->GetPrefs()->GetBoolean(
+                 prefs::kHotwordAudioLoggingEnabled)) {
+    DCHECK(
+        !profile->GetPrefs()->GetBoolean(prefs::kHotwordAlwaysOnSearchEnabled));

[stevenjb, 2015/04/28 17:16:38]

Sane here.

[Oren Blasberg, 2015/04/28 21:58:30]

Done.

+    launch_mode = HotwordService::HOTWORD_ONLY;
+  } else {
+    DCHECK(
+        !profile->GetPrefs()->GetBoolean(prefs::kHotwordAlwaysOnSearchEnabled));

[stevenjb, 2015/04/28 17:16:38]

And here.

[Oren Blasberg, 2015/04/28 21:58:30]

Done.

+  }
+
+  HotwordService* hotword_service =
+      HotwordServiceFactory::GetForProfile(profile);
+  if (hotword_service)
+    hotword_service->OptIntoHotwording(launch_mode);
+  return RespondNow(NoArguments());
+}
+
 }  // namespace extensions