[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_map.h"
+#include "base/android/jni_string.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/TemplateUrlService_jni.h"
+
+using base::android::CheckException;

[Peter Kasting, 2013/02/20 05:31:07]

Nit: Avoid using statements except where they save a number of lines

[Yaron, 2013/02/21 21:39:15]

Done.

+using base::android::ConvertMapToJavaMap;
+using base::android::ConvertUTF16ToJavaString;
+using base::android::GetClass;
+using base::android::ScopedJavaLocalRef;
+
+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();
+}
+}  // namespace
+
+TemplateUrlServiceAndroid::TemplateUrlServiceAndroid(
+    JNIEnv* env, jobject obj)

[Peter Kasting, 2013/02/20 05:31:07]

Nit: One arg per line

[Yaron, 2013/02/21 21:39:15]

Done.

+    : weak_java_obj_(env, obj),
+      template_url_service_(
+        TemplateURLServiceFactory::GetForProfile(GetOriginalProfile())) {

[Peter Kasting, 2013/02/20 05:31:07]

Nit: Indent 4, not 2

[Yaron, 2013/02/21 21:39:15]

Done.

+  registrar_.Add(this, chrome::NOTIFICATION_TEMPLATE_URL_SERVICE_LOADED,
+      content::Source<TemplateURLService>(template_url_service_));

[Peter Kasting, 2013/02/20 05:31:07]

Nit: Lines of arguments should be horizontally aligned (several places)

[Yaron, 2013/02/21 21:39:15]

Sorry, I thought that was only for function declarations. 

+}
+
+TemplateUrlServiceAndroid::~TemplateUrlServiceAndroid() {
+}
+
+void TemplateUrlServiceAndroid::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  DCHECK(type == chrome::NOTIFICATION_TEMPLATE_URL_SERVICE_LOADED);

[Peter Kasting, 2013/02/20 05:31:07]

Nit: DCHECK_EQ

[Yaron, 2013/02/21 21:39:15]

Done.

+  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) {
+  if (!template_url_service_->loaded())

[Peter Kasting, 2013/02/20 05:31:07]

Nit: Remove conditional; Load() will check this and other conditions and
short-circuit internally

[Yaron, 2013/02/21 21:39:15]

Done.

+     template_url_service_->Load();
+}
+
+void TemplateUrlServiceAndroid::SetSearchEngine(JNIEnv* env,
+                                                jobject obj,
+                                                jint selected_index) {
+  std::vector<TemplateURL*> template_urls =
+      template_url_service_->GetTemplateURLs();
+  if (selected_index < 0 ||
+      selected_index >= static_cast<int>(template_urls.size())) {

[Peter Kasting, 2013/02/20 05:31:07]

Nit: Given that you use selected_index as a size_t below (by using []), I
suggest casting it to a size_t and storing that in a temp.

This should also eliminate the need to check for "< 0" since that will turn into
a failure of the ">= .size()" check in most cases; if you're worried about the
risk of treating a very large negative number as a valid small positive number,
however, you could do the check at conversion time.

[Yaron, 2013/02/21 21:39:15]

Done.

+    DLOG(ERROR) << "Wrong index for search engine";

[Peter Kasting, 2013/02/20 05:31:07]

Nit: In general we try to avoid logging statements, since it's hard to collect
logs from most users' machines; there's no rule against them, but consider
whether they could just be eliminated, or else turned into a DCHECK() or similar
if this case could never happen.

[Yaron, 2013/02/21 21:39:15]

Converted to DCHECK.

+    return;
+  }
+
+  TemplateURL* template_url = template_urls[selected_index];
+  if (template_url->prepopulate_id() > 0) {
+    template_url_service_->SetDefaultSearchProvider(template_url);
+  } else {
+    DLOG(ERROR) << "Tried to select non-prepopulated search engine";
+  }
+}
+
+jint TemplateUrlServiceAndroid::GetSearchEngine(JNIEnv* env, jobject obj) {
+  std::vector<TemplateURL*> template_urls =
+      template_url_service_->GetTemplateURLs();
+  for (size_t i = 0; i < template_urls.size(); ++i) {
+    if (template_url_service_->GetDefaultSearchProvider() == template_urls[i]) {

[Peter Kasting, 2013/02/20 05:31:07]

Nit: Move this call outside the loop, since it's not dirt-cheap

[Yaron, 2013/02/21 21:39:15]

Done.

+      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) {

[Peter Kasting, 2013/02/20 05:31:07]

Nit: Indent 4

[Yaron, 2013/02/21 21:39:15]

Done.

+  TemplateURLService::TemplateURLVector template_urls =
+      template_url_service_->GetTemplateURLs();
+
+  int prepopulated_urls_count = 0;
+  for (size_t i = 0; i < template_urls.size(); i++) {

[bulach, 2013/02/20 14:41:25]

nit: ++i (and 118 too)

[Yaron, 2013/02/21 21:39:15]

Done.

+    if (template_urls[i]->prepopulate_id() > 0)
+      prepopulated_urls_count++;
+  }
+
+  ScopedJavaLocalRef<jclass> hashmap_clazz = GetClass(env, "java/util/HashMap");

[Peter Kasting, 2013/02/20 05:31:07]

Nit: clazz?

[Yaron, 2013/02/21 21:39:15]

Customary in java but your'e right that it's wrong here.

+  jobjectArray search_engine_array =
+      env->NewObjectArray(prepopulated_urls_count, hashmap_clazz.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::stringstream ss;

[Peter Kasting, 2013/02/20 05:31:07]

The Google style guide bans stringstreams outside of logging code.

Here you're just converting an int to a string, so consider using something from
string_number_conversions.h.  If you want to avoid casting the size_t to
something else, you could use StringPrintf() instead with the ("%" PRIuS)
argument as the format string.

[Yaron, 2013/02/21 21:39:15]

Done.

+    ss << i;
+    std::map<std::string, string16> search_engine_map;
+    search_engine_map[kSearchEngineId] = UTF8ToUTF16(ss.str());
+    search_engine_map[kSearchEngineShortName] = template_url->short_name();
+    search_engine_map[kSearchEngineKeyword] = template_url->keyword();
+    search_engine_map[kSearchEngineUrl] = template_url->url_ref().DisplayURL();
+
+    DCHECK(prepopulated_url_index < prepopulated_urls_count);
+    env->SetObjectArrayElement(search_engine_array, prepopulated_url_index,
+        ConvertMapToJavaMap(env, search_engine_map).obj());
+    CheckException(env);
+    prepopulated_url_index++;
+  }
+  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 RegisterNativesImpl(env);
+}