Fetch snippets from ChromeReader and show them on the NTP

chrome/browser/android/ntp_snippets_bridge.cc

+// Copyright 2016 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/android/ntp_snippets_bridge.h"
+
+#include <jni.h>
+
+#include "base/android/jni_android.h"
+#include "base/android/jni_array.h"
+#include "base/android/scoped_java_ref.h"
+#include "chrome/browser/ntp_snippets/ntp_snippets_service_factory.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_android.h"
+#include "components/ntp_snippets/ntp_snippets_service.h"
+#include "jni/NTPSnippetsBridge_jni.h"
+
+using base::android::JavaParamRef;
+using base::android::ToJavaArrayOfStrings;
+
+namespace ntp_snippets {
+static jlong Init(JNIEnv* env,
+                  const JavaParamRef<jobject>& obj,
+                  const JavaParamRef<jobject>& j_profile) {
+  NTPSnippetsBridge* snippets_bridge = new NTPSnippetsBridge(j_profile);
+  return reinterpret_cast<intptr_t>(snippets_bridge);
+}
+
+NTPSnippetsBridge::NTPSnippetsBridge(jobject j_profile) {
+  Profile* profile = ProfileAndroid::FromProfileAndroid(j_profile);
+  if (profile) {
+    ntp_snippets_service_ = NTPSnippetsServiceFactory::GetForProfile(profile);
+    ntp_snippets_service_->AddObserver(this);

[Bernhard Bauer, 2016/02/08 18:19:26]

When are you going to unregister yourself as an observer?

(FTR, ScopedObserver can be very helpful for that.)

[May, 2016/02/09 17:38:53]

Thanks for the hint. Done.

+  }
+}
+
+NTPSnippetsBridge::~NTPSnippetsBridge() {}
+
+void NTPSnippetsBridge::Destroy(JNIEnv* env, const JavaParamRef<jobject>& obj) {

[Bernhard Bauer, 2016/02/08 18:19:26]

This method isn't called anywhere (or declared in Java, for that matter).

[May, 2016/02/09 17:38:53]

Fixed (by actually calling it on cleanup)

+  delete this;
+}
+
+void NTPSnippetsBridge::SetSnippetsObserver(
+    JNIEnv* env,
+    const JavaParamRef<jobject>& obj,
+    const JavaParamRef<jobject>& j_observer) {
+  observer_.Reset(env, j_observer);
+  if (ntp_snippets_service_->is_loaded()) {

[Bernhard Bauer, 2016/02/08 18:19:26]

Nit: no braces for single-line bodies.

[May, 2016/02/09 17:38:52]

Done.

+    NTPSnippetsServiceLoaded(ntp_snippets_service_);
+  }
+}
+
+void NTPSnippetsBridge::NTPSnippetsServiceLoaded(NTPSnippetsService* service) {

[Bernhard Bauer, 2016/02/08 18:19:26]

You could just directly access |ntp_snippets_service_| instead of passing it.

[May, 2016/02/09 17:38:53]

It's an override, that's the signature of the NTPSnippetsServiceObserver.

[Bernhard Bauer, 2016/02/10 10:48:44]

Oh, I see.

Hm, could we only register as an NTPSnippetsServiceObserver in
SetSnippetsObserver() instead of in the constructor? Before we have a Java
observer, we're not interested in the snippets anyway, and that way this method
will only be called through the interface.

Or we could even merge SetSnippetsObserver() and the constructor -- the observer
is set right after construction anyway, so we wouldn't have to deal with
|observer_| being null.

[May, 2016/02/10 18:16:46]

Done.

+  if (observer_.is_null())
+    return;
+
+  std::vector<std::string> titles;
+  std::vector<std::string> urls;
+  std::vector<std::string> thumbnail_urls;
+  std::vector<std::string> snippets;
+  for (auto& snippet : *service) {

[Bernhard Bauer, 2016/02/08 18:19:26]

Can you use an actual type here instead of auto? And maybe const?

[May, 2016/02/09 17:38:53]

I used auto because it was the recommended way of accessing NTPSnippetsService
(written by noyau@). I'll change it if you feel strongly. Added the const.

[Bernhard Bauer, 2016/02/10 10:48:44]

Re: type, it's very complicated to find out the actual type of |snippet| here,
in particular as |service| isn't directly a collection of some type. My rule is
roughly, if you wouldn't use auto outside of the for-loop (and we usually don't
in Chrome unless the type is very complicated), I wouldn't use it in the for
loop.

Re: const, it turns out that the type that is replaced with auto already is
const, so the additional const isn't actually necessary. I think this really
just underscores my point though that it would be preferable to use the real
type, because this is extremely hard to notice unless you happen to look at the
iterator method in NTPSnippetsService, and if you don't, it either looks wrong
(missing const) or is wrong (double const).

[May, 2016/02/10 18:16:46]

Done.

+    titles.push_back(snippet.title());
+    urls.push_back(snippet.url().spec());
+    thumbnail_urls.push_back(snippet.salient_image_url().spec());
+    snippets.push_back(snippet.snippet());
+  }
+
+  JNIEnv* env = base::android::AttachCurrentThread();
+  Java_SnippetsObserver_onSnippetAvailable(
+      env, observer_.obj(), ToJavaArrayOfStrings(env, titles).obj(),
+      ToJavaArrayOfStrings(env, urls).obj(),
+      ToJavaArrayOfStrings(env, thumbnail_urls).obj(),
+      ToJavaArrayOfStrings(env, snippets).obj());
+}
+
+void NTPSnippetsBridge::NTPSnippetsServiceShutdown(
+    NTPSnippetsService* service) {
+  observer_.Reset();
+}
+
+// static
+bool RegisterNTPSnippetsBridge(JNIEnv* env) {
+  return RegisterNativesImpl(env);
+}
+}  // namespace ntp_snippets