[Android] Introduce a Java wrapper for TemplateUrlService.

chrome/browser/search_engines/template_url_service_android.cc

+// 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.
+
+#include "chrome/browser/search_engines/template_url_service_android.h"
+
+#include "base/android/jni_string.h"
+#include "base/format_macros.h"
+#include "base/stringprintf.h"
+#include "base/utf_string_conversions.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/search_engines/template_url.h"
+#include "chrome/browser/search_engines/template_url_prepopulate_data.h"
+#include "chrome/browser/search_engines/template_url_service.h"
+#include "chrome/browser/search_engines/template_url_service_factory.h"
+#include "chrome/common/chrome_notification_types.h"
+#include "jni/HashMap_jni.h"
+#include "jni/TemplateUrlService_jni.h"
+
+namespace {
+
+const char kSearchEngineId[] = "searchEngineId";
+const char kSearchEngineShortName[] = "searchEngineShortName";
+const char kSearchEngineKeyword[] = "searchEngineKeyword";
+const char kSearchEngineUrl[] = "searchEngineUrl";
+
+Profile* GetOriginalProfile() {
+  return g_browser_process->profile_manager()->GetDefaultProfile()->
+      GetOriginalProfile();
+}
+
+base::android::ScopedJavaLocalRef<jobject> ConvertMapToJavaMap(
+    JNIEnv* env,
+    const std::map<std::string, string16>& cmap) {
+  base::android::ScopedJavaLocalRef<jobject> jmap(
+      JNI_HashMap::Java_HashMap_ConstructorJUHM(env));
+
+  for (std::map<std::string, string16>::const_iterator i = cmap.begin();
+       i != cmap.end(); ++i) {
+    base::android::ScopedJavaLocalRef<jstring> key =
+        base::android::ConvertUTF8ToJavaString(env, i->first);
+    base::android::ScopedJavaLocalRef<jstring> value =
+        base::android::ConvertUTF16ToJavaString(env,
+                                                                 i->second);

[bulach, 2013/02/25 12:51:19]

nit: fits previous line

[Yaron, 2013/02/25 18:45:10]

Done.

+    JNI_HashMap::Java_HashMap_put(env, jmap.obj(), key.obj(), value.obj());
+  }
+  return jmap;
+}
+}  // namespace
+
+TemplateUrlServiceAndroid::TemplateUrlServiceAndroid(JNIEnv* env,
+                                                     jobject obj)
+    : weak_java_obj_(env, obj),
+      template_url_service_(
+          TemplateURLServiceFactory::GetForProfile(GetOriginalProfile())) {
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_TEMPLATE_URL_SERVICE_LOADED,
+                 content::Source<TemplateURLService>(template_url_service_));
+}
+
+TemplateUrlServiceAndroid::~TemplateUrlServiceAndroid() {
+}
+
+void TemplateUrlServiceAndroid::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  DCHECK_EQ(chrome::NOTIFICATION_TEMPLATE_URL_SERVICE_LOADED, type);
+  JNIEnv* env = base::android::AttachCurrentThread();
+  if (weak_java_obj_.get(env).is_null())
+    return;
+
+  Java_TemplateUrlService_templateUrlServiceLoaded(env,
+      weak_java_obj_.get(env).obj());
+}
+
+void TemplateUrlServiceAndroid::Load(JNIEnv* env, jobject obj) {
+  template_url_service_->Load();
+}
+
+void TemplateUrlServiceAndroid::SetSearchEngine(JNIEnv* env,
+                                                jobject obj,
+                                                jint selected_index) {
+  std::vector<TemplateURL*> template_urls =
+      template_url_service_->GetTemplateURLs();
+  size_t selected_index_size_t = static_cast<size_t>(selected_index);
+  DCHECK_LT(selected_index_size_t, template_urls.size()) <<
+      "Wrong index for search engine";
+
+  TemplateURL* template_url = template_urls[selected_index_size_t];
+  DCHECK_GT(template_url->prepopulate_id(), 0) <<
+      "Tried to select non-prepopulated search engine";
+  template_url_service_->SetDefaultSearchProvider(template_url);
+}
+
+jint TemplateUrlServiceAndroid::GetSearchEngine(JNIEnv* env, jobject obj) {
+  std::vector<TemplateURL*> template_urls =
+      template_url_service_->GetTemplateURLs();
+  TemplateURL* default_search_provider =
+      template_url_service_->GetDefaultSearchProvider();
+  for (size_t i = 0; i < template_urls.size(); ++i) {
+    if (default_search_provider == template_urls[i]) {
+      return i;
+    }
+  }
+  return -1;
+}
+
+jboolean TemplateUrlServiceAndroid::IsLoaded(JNIEnv* env, jobject obj) {
+  return template_url_service_->loaded();
+}
+
+base::android::ScopedJavaLocalRef<jobjectArray>
+    TemplateUrlServiceAndroid::GetLocalizedSearchEngines(JNIEnv* env,
+                                                         jobject obj) {
+  TemplateURLService::TemplateURLVector template_urls =
+      template_url_service_->GetTemplateURLs();
+
+  int prepopulated_urls_count = 0;
+  for (size_t i = 0; i < template_urls.size(); ++i) {
+    if (template_urls[i]->prepopulate_id() > 0)
+      prepopulated_urls_count++;

[bulach, 2013/02/25 12:51:19]

nit: ++prepopulated_urls_count

[Yaron, 2013/02/25 18:45:10]

Done.

+  }
+
+  base::android::ScopedJavaLocalRef<jclass> hashmap_class =
+      base::android::GetClass(env, "java/util/HashMap");

[bulach, 2013/02/25 12:51:19]

I think there's a handy JNI_HashMap::g_HashMap_clazz

[Yaron, 2013/02/25 18:45:10]

it's in anon namesapce but sure.

+  jobjectArray search_engine_array =
+      env->NewObjectArray(prepopulated_urls_count, hashmap_class.obj(), NULL);
+
+  int prepopulated_url_index = 0;
+  for (size_t i = 0; i < template_urls.size(); ++i) {
+    TemplateURL* template_url = template_urls[i];
+    // Only show prepopulated template URLs.
+    if (template_url->prepopulate_id() <= 0)
+      continue;
+
+    std::map<std::string, string16> search_engine_map;
+    search_engine_map[kSearchEngineId] = UTF8ToUTF16(
+        base::StringPrintf("%" PRIuS, i));
+    search_engine_map[kSearchEngineShortName] = template_url->short_name();
+    search_engine_map[kSearchEngineKeyword] = template_url->keyword();
+    search_engine_map[kSearchEngineUrl] = template_url->url_ref().DisplayURL();

[bulach, 2013/02/25 12:51:19]

damn ocean apart... :) sorry, I think I'm not exemplifying my thoughts as clear
as I should...

this looks fine, but let me try to sketch out something, let me know if the
following is clearer:

java:
class Foo {
  class Item {
    String mA;
    String mB;
    @CalledByNative("Item")
    create(String a, String b, String c) {
      ...
    }
  }
  native int nativeGetItemCount();
  native Item nativeGetItemAt(int n);
  ...
  Item[] doSomething() {
    int count = nativeGetItemCount();
    Item[] ret = new Item[count];
    for (int i = 0; i < count; ++count)
      ret[i] = nativeGetItemAt(i);
    return ret;
  }
}


c++:
jint GetCount(...) {
  return template_url_service_->GetTemplateURLs()->size();
}

jobject GetItemAt(..., jint i) {
  TemplateURL* t =
    template_url_service_->GetTemplateURLs()[i];
  if (t->prepopulate_id() <= 0)
    return NULL;
  return Java_Foo_createItem(
    t->short_name, t->keyword, t->url_ref().DisplayURL());  
}


my summary would then be:
pros:
+ type safety guaranteed by Java_Foo_createItem, and explicitly documented in
code by the "Item" class.
+ no need to duplicate these consts across the board
+ no need to know anything about java map in c++.. it'd only know about the
"Item" class, which is the only thing it cares about.
+ less indirections / boxing: it's clear from java code that it requires an
"Item" object, and it's clearer from c++ that it provides one...
+ if the java-side needs a map rather than Item, then let java side do this
massaging / conversion, and keep c++ only using "opaque" datatypes (i.e., it
doesn't know what an item is, but it has a clean and safe API to create one)..
the map is too transparent here.

cons:
- the JNI api now contains a "getCount()" and "getItemAt()" rather than
"getAllItems()".. however, as above, I think these two APIs are simpler and
self-documented... 


would this make any sense, specially in the context of setting this as a pattern
for other classes in the future?

wdyt? :)

[Yaron, 2013/02/25 18:45:10]

I'm convinced. I didn't realize you meant to get rid of the map entirely and
create another type instead of marshalling a key-value across the border. I like
it much better now, downstream code is easier to read, and no more magic
constants.

Thanks!

+
+    DCHECK_LT(prepopulated_url_index, prepopulated_urls_count);
+    env->SetObjectArrayElement(search_engine_array, prepopulated_url_index,
+        ConvertMapToJavaMap(env, search_engine_map).obj());
+    base::android::CheckException(env);
+    prepopulated_url_index++;

[bulach, 2013/02/25 12:51:19]

nit: ++prepopulated_url_index

[Yaron, 2013/02/25 18:45:10]

Done.

+  }
+  return ScopedJavaLocalRef<jobjectArray>(env, search_engine_array);
+}
+
+
+static jint Init(JNIEnv* env, jobject obj) {
+  TemplateUrlServiceAndroid* template_url_service_android =
+      new TemplateUrlServiceAndroid(env, obj);
+  return reinterpret_cast<jint>(template_url_service_android);
+}
+
+// static
+bool TemplateUrlServiceAndroid::Register(JNIEnv* env) {
+  return JNI_HashMap::RegisterNativesImpl(env) &&
+      RegisterNativesImpl(env);
+}