Change behaivor to decide whether a search engine should be shown in the default list

chrome/browser/ui/webui/options/browser_options_handler.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/webui/options/browser_options_handler.h"
 
 #include <stddef.h>
 
 #include <set>
 #include <utility>
@@ skipping 1226 matching lines @@
     return;
 
   const TemplateURL* default_url =
       template_url_service_->GetDefaultSearchProvider();
 
   int default_index = -1;
   base::ListValue search_engines;
   TemplateURLService::TemplateURLVector model_urls(
       template_url_service_->GetTemplateURLs());
   for (size_t i = 0; i < model_urls.size(); ++i) {
-    if (!model_urls[i]->ShowInDefaultList(
+    TemplateURL* t_url = model_urls[i];

[Peter Kasting, 2016/11/10 06:41:06]

Nit: If you're going to declare this, use it everywhere below that refers to
model_urls[i] (2 more places).

[ltian, 2016/11/11 03:52:13]

Done.

+    if (!template_url_service_->ShowInDefaultList(t_url) ||
+        !model_urls[i]->SupportsReplacement(

[Peter Kasting, 2016/11/10 06:41:06]

I'm concerned that I don't understand why this was the only place previously
that was explicitly checking SupportsReplacement() (implicitly, by calling
ShowInDefaultList()).

It seems like either this can just check the new ShowInDefaultList() and not
also SupportsReplacement(), or else ShowInDefaultList() itself should check
that.

The former would be a relatively small behavior change and thus hopefully easy
to understand the ramifications of.  The latter would not.

Can you help me understand the effect removing this SupportsReplacement() call
would have?

[ltian, 2016/11/11 03:52:13]

If I understand correctly, browser_options_handler.cc is only for desktop
version. And for this onTemplateURLServiceChanged(), it controls content shown
in the drop down list aside "Manage search engines..." button (the button to
display the dialog of all search engines).  However, for the dialog showing all
search engines, it is controlled by template_url_table_model.cc and inside it,
it only check  show_in_default_list field to decide the default list
(https://codesearch.chromium.org/chromium/src/chrome/browser/ui/search_engines...).
So previous code uses slightly different logic for displaying default list in
different places.

For SupportsReplacement() itself, it checks whether the TemplateURLRef of a
template url has placeholders in it, for which callers can substitute values to
get a "real" URL using ReplaceSearchTerms().
(https://codesearch.chromium.org/chromium/src/components/search_engines/templa...).

So I will update the ShowInDefaultList() to also check SupportsReplacement().

             template_url_service_->search_terms_data()))
       continue;
 
     std::unique_ptr<base::DictionaryValue> entry(new base::DictionaryValue());
     entry->SetString("name", model_urls[i]->short_name());
     entry->SetInteger("index", i);
     search_engines.Append(std::move(entry));
     if (model_urls[i] == default_url)
       default_index = i;
   }
@@ skipping 971 matching lines @@
 
 bool BrowserOptionsHandler::IsDeviceOwnerProfile() {
 #if defined(OS_CHROMEOS)
   return chromeos::ProfileHelper::IsOwnerProfile(Profile::FromWebUI(web_ui()));
 #else
   return true;
 #endif
 }
 
 }  // namespace options