[Android] Allow setting recently visited search engines as default search engine

chrome/browser/search_engines/template_url_service_android.h

 // Copyright (c) 2013 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.
 
 #ifndef CHROME_BROWSER_SEARCH_ENGINES_TEMPLATE_URL_SERVICE_ANDROID_H_
 #define CHROME_BROWSER_SEARCH_ENGINES_TEMPLATE_URL_SERVICE_ANDROID_H_
 
 #include <memory>
 
 #include "base/android/jni_weak_ref.h"
 #include "base/android/scoped_java_ref.h"
 #include "base/macros.h"
 #include "components/search_engines/template_url_service.h"
 #include "components/search_engines/template_url_service_observer.h"
 
 class TemplateURL;
 
 
 // Android wrapper of the TemplateUrlService which provides access from the Java
 // layer. Note that on Android, there's only a single profile, and therefore
 // a single instance of this wrapper.
 class TemplateUrlServiceAndroid : public TemplateURLServiceObserver {
  public:
   TemplateUrlServiceAndroid(JNIEnv* env, jobject obj);
 
   void Load(JNIEnv* env, const base::android::JavaParamRef<jobject>& obj);
   void SetUserSelectedDefaultSearchProvider(
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj,
-      jint selected_index);
+      const base::android::JavaParamRef<jstring>& jkeyword);
+  // Update template_urls and return the index of default search provider in the

[Peter Kasting, 2016/11/02 00:34:03]

Nit: Function comments should be descriptive ("Updates") rather than imperative
("Update"); see
http://google.github.io/styleguide/cppguide.html#Function_Comments .

I would place a blank line above this comment and below the function, to
separate it from the other functions this comment is not talking about.  Bonus
points if you add similar descriptive comments to any other functions which
wouldn't be instantly clear to readers, or have surprising side effects.

This comment refers to "template_urls", which isn't a parameter.  Did you mean
|template_urls_|?  It says "updates"; updates how?  In a way different from the
last sentence of the comment (which is what "Note" kind of implies")?  Or does
that cover this?  If so make this clearer, e.g.:

Returns the index of the default search provider within |template_urls_|.  If
this is a custom search engine (as opposed to a prepopulated one), the list will
be reordered so it comes before all other custom engines.

However, I still think it's confusing and error-prone that this getter has
obviously-non-const behavior on the class, and this reordering should be
something you do elsewhere (see comment on previous patch set, why you didn't
reply to).  A caller that did this seemingly-benign pseudocode would go badly
wrong:

  for (i = 0; i < GetTemplateUrlCount(); ++i)
    AddItemToMenu(GetTemplateUrlAt(i));
  SelectMenuItem(GetDefaultSearchProvider());

In fact it's even worse than it looks, because before calling
GetDefaultSearchProvider() the first time, |template_urls_| is totally empty;
this function doesn't just reorder the list, it populates it.

I really think you need to change this design.  You can already observe when
things in the base TemplateURLService change, so you can keep a parallel list in
sync.

[ltian, 2016/11/22 21:51:41]

I will wrap the operations about populating and sorting |template_urls_| in
another function and in this function, it first calls that function to reload
all data and then returns the index of default provider. Does this sound a
better design?

[Peter Kasting, 2016/11/22 21:57:10]

Are you planning to call these methods from every function that can directly or
indirectly access this data?  For example, GetTemplateUrlCount() needs this too.
 Then will you make the relevant methods const and |template_urls_| mutable, so
it's clear that, from an API perspective, calling this does not change
observable state?

If so, then what you're describing is a pull-based system that uses
|template_urls_| as a cache which is updated on each attempted access.  One of
my core questions is whether that's a better design than a push-based system
which updates this on every change to the underlying URLs.

[ltian, 2016/11/22 23:25:10]

Yes, it is a pull-based system and I think it is fine because the Android side
is also a pull-based system. The Android UI will not be updated once it is
initiated(By initEntries() in SearchEngineAdapter.java). 

And I think LoadTemplateURLs() is not needed to be called from every function
accessing this data. It only need to be called by
GetDefaultSearchProviderIndex() because that is the very first function to get
data from TemplateURLService once the service is loaded.

So basically the workflow is when Android side tries to populate UI, it calls
the initEntries() in SearchEngineAdapter.java, the initEntries() first calls
GetDefaultSearchProviderIndex() and GetDefaultSearchProviderIndex() loads
|template_urls_| and |default_search_provider_index_|. Then later every Android
side call will directly retrieves data from those two fields. The two field will
only be reloaded once Android side loads UI again.  

[Peter Kasting, 2016/11/22 23:36:20]

Then why not force the caller to use that access pattern?  Eliminate most of the
functions here and replace them with some kind of thing that just returns the
full list of relevant TemplateURLs, in order, plus the index of the default
search URL in that list.

Then there's no need to cache anything; you just compute this each time.  The
API becomes simpler and safer.  And the caller can cache this list or do
whatever they want with it.
 
But can you guarantee that won't change in the future?  What if someone adds a
call to one of these other functions from elsewhere later, since the API looks
like that's a reasonable thing to do?
Right, the problem is that this code cannot guarantee that's how all callers for
all time will use it, and the API does not make it hard or impossible to use it
in some other way.

+  // list. Note that this function will also reorder the list so that if any
+  // custom search engine is set as default, it will be placed before the rest
+  // of

[Peter Kasting, 2016/11/02 00:34:03]

Nit: Rewrap

[ltian, 2016/11/22 21:51:41]

Done.

+  // custom ones and after all the prepopulated ones.
   jint GetDefaultSearchProvider(

[Peter Kasting, 2016/11/02 00:34:03]

Nit: GetDefaultSearchProviderIndex()?

[ltian, 2016/11/22 21:51:41]

Done.

       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj);
   jint GetTemplateUrlCount(JNIEnv* env,
                            const base::android::JavaParamRef<jobject>& obj);
   jboolean IsLoaded(JNIEnv* env,
                     const base::android::JavaParamRef<jobject>& obj);
   base::android::ScopedJavaLocalRef<jobject> GetTemplateUrlAt(
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj,
       jint index);
@@ skipping 22 matching lines @@
   base::android::ScopedJavaLocalRef<jstring> GetUrlForContextualSearchQuery(
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj,
       const base::android::JavaParamRef<jstring>& jquery,
       const base::android::JavaParamRef<jstring>& jalternate_term,
       jboolean jshould_prefetch,
       const base::android::JavaParamRef<jstring>& jprotocol_version);
   base::android::ScopedJavaLocalRef<jstring> GetSearchEngineUrlFromTemplateUrl(
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj,
-      jint index);
+      const base::android::JavaParamRef<jstring>& jkeyword);
 
   static bool Register(JNIEnv* env);
 
  private:
   ~TemplateUrlServiceAndroid() override;
 
-  bool IsPrepopulatedTemplate(TemplateURL* url);
-
   void OnTemplateURLServiceLoaded();
 
   // TemplateUrlServiceObserver:
   void OnTemplateURLServiceChanged() override;
 
   JavaObjectWeakGlobalRef weak_java_obj_;
 
   // Pointer to the TemplateUrlService for the main profile.
   TemplateURLService* template_url_service_;
 
   std::unique_ptr<TemplateURLService::Subscription> template_url_subscription_;
 
+  std::vector<TemplateURL*> template_urls_;
+
   DISALLOW_COPY_AND_ASSIGN(TemplateUrlServiceAndroid);
 };
 
 #endif  // CHROME_BROWSER_SEARCH_ENGINES_TEMPLATE_URL_SERVICE_ANDROID_H_