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

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

Index: chrome/browser/ui/webui/options/browser_options_handler.cc
diff --git a/chrome/browser/ui/webui/options/browser_options_handler.cc b/chrome/browser/ui/webui/options/browser_options_handler.cc
index 277d5d35a6445ad7a22814177d13af8c5990eef2..8aad0702c64f1444ddbc9c2b5e67450b690aafd3 100644
--- a/chrome/browser/ui/webui/options/browser_options_handler.cc
+++ b/chrome/browser/ui/webui/options/browser_options_handler.cc
@@ -1244,7 +1244,9 @@ void BrowserOptionsHandler::OnTemplateURLServiceChanged() {
   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;