Allow C++ iteration over Java object arrays.

Introduce JavaObjectArrayReader, which provides an input iterator that
returns each element of a Java object in turn, wrapped as a
ScopedJavaLocalRef. Also add a utility function
JavaRef<jobjectArray>::ReadElements<T>() which creates a reader for the
array pointed to by the reference, for ease of use.

Convert a number of simple cases in the Chromium sources to use the
newly introduced mechanisms to demonstrate their use.

Change-Id: I2b9aefe45a50885d96c074d5904a0ed3e22334be
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1283836
Reviewed-by: Joe Downing <joedow@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Commit-Queue: Richard Coles <torne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#655028}

base/android/child_process_service.cc

@@ -27,13 +27,11 @@ void JNI_ChildProcessService_RegisterFileDescriptors(
     const JavaParamRef<jlongArray>& j_offsets,
     const JavaParamRef<jlongArray>& j_sizes) {
   std::vector<base::Optional<std::string>> keys;
-  jsize keys_size = env->GetArrayLength(j_keys);
-  keys.reserve(keys_size);
-  for (jsize i = 0; i < keys_size; i++) {
-    base::android::ScopedJavaLocalRef<jstring> str(
-        env, static_cast<jstring>(env->GetObjectArrayElement(j_keys, i)));
+  JavaObjectArrayReader<jstring> keys_array(j_keys);
+  keys.reserve(keys_array.size());
+  for (auto str : keys_array) {
     base::Optional<std::string> key;
-    if (!str.is_null()) {
+    if (str) {
       key = base::android::ConvertJavaStringToUTF8(env, str);
     }
     keys.push_back(std::move(key));

base/android/scoped_java_ref.h

@@ -96,6 +96,10 @@ class BASE_EXPORT JavaRef<jobject> {
   DISALLOW_COPY_AND_ASSIGN(JavaRef);
 };
 
+// Forward declare the object array reader for the convenience function.
+template <typename T>
+class JavaObjectArrayReader;
+
 // Generic base class for ScopedJavaLocalRef and ScopedJavaGlobalRef. Useful
 // for allowing functions to accept a reference without having to mandate
 // whether it is a local or global type.
@@ -108,6 +112,17 @@ class JavaRef : public JavaRef<jobject> {
 
   T obj() const { return static_cast<T>(JavaRef<jobject>::obj()); }
 
+  // Get a JavaObjectArrayReader for the array pointed to by this reference.
+  // Only defined for JavaRef<jobjectArray>.
+  // You must pass the type of the array elements (usually jobject) as the
+  // template parameter.
+  template <typename ElementType,
+            typename T_ = T,
+            typename = std::enable_if_t<std::is_same<T_, jobjectArray>::value>>
+  JavaObjectArrayReader<ElementType> ReadElements() const {
+    return JavaObjectArrayReader<ElementType>(*this);
+  }
+
  protected:
   JavaRef(JNIEnv* env, T obj) : JavaRef<jobject>(env, obj) {}
 
@@ -283,6 +298,10 @@ class ScopedJavaLocalRef : public JavaRef<T> {
   // Friend required to get env_ from conversions.
   template <typename U>
   friend class ScopedJavaLocalRef;
+
+  // Avoids JavaObjectArrayReader having to accept and store its own env.
+  template <typename U>
+  friend class JavaObjectArrayReader;
 };
 
 // Holds a global reference to a Java object. The global reference is scoped
@@ -387,6 +406,135 @@ class ScopedJavaGlobalRef : public JavaRef<T> {
   T Release() { return static_cast<T>(JavaRef<T>::ReleaseInternal()); }
 };
 
+// Wrapper for a jobjectArray which supports input iteration, allowing Java
+// arrays to be iterated over with a range-based for loop, or used with
+// <algorithm> functions that accept input iterators.
+//
+// The iterator returns each object in the array in turn, wrapped in a
+// ScopedJavaLocalRef<T>. T will usually be jobject, but if you know that the
+// array contains a more specific type (such as jstring) you can use that
+// instead. This does not check the type at runtime!
+//
+// The wrapper holds a local reference to the array and only queries the size of
+// the array once, so must only be used as a stack-based object from the current
+// thread.
+//
+// Note that this does *not* update the contents of the array if you mutate the
+// returned ScopedJavaLocalRef.
+template <typename T>
+class JavaObjectArrayReader {
+ public:
+  class iterator {
+   public:
+    // We can only be an input iterator, as all richer iterator types must
+    // implement the multipass guarantee (always returning the same object for
+    // the same iterator position), which is not practical when returning
+    // temporary objects.
+    using iterator_category = std::input_iterator_tag;
+
+    using difference_type = ptrdiff_t;
+    using value_type = ScopedJavaLocalRef<T>;
+
+    // It doesn't make sense to return a reference type as the iterator creates
+    // temporary wrapper objects when dereferenced. Fortunately, it's not
+    // required that input iterators actually use references, and defining it
+    // as value_type is valid.
+    using reference = value_type;
+
+    // This exists to make operator-> work as expected: its return value must
+    // resolve to an actual pointer (otherwise the compiler just keeps calling
+    // operator-> on the return value until it does), so we need an extra level
+    // of indirection. This is sometimes called an "arrow proxy" or similar, and
+    // this version is adapted from base/value_iterators.h.
+    class pointer {
+     public:
+      explicit pointer(const reference& ref) : ref_(ref) {}
+      pointer(const pointer& ptr) = default;
+      pointer& operator=(const pointer& ptr) = delete;
+      reference* operator->() { return &ref_; }
+
+     private:
+      reference ref_;
+    };
+
+    iterator(const iterator&) = default;
+    ~iterator() = default;
+
+    iterator& operator=(const iterator&) = default;
+
+    bool operator==(const iterator& other) const {
+      DCHECK(reader_ == other.reader_);
+      return i_ == other.i_;
+    }
+
+    bool operator!=(const iterator& other) const {
+      DCHECK(reader_ == other.reader_);
+      return i_ != other.i_;
+    }
+
+    reference operator*() const {
+      DCHECK(i_ < reader_->size_);
+      // JNIEnv functions return unowned local references; take ownership with
+      // Adopt so that ~ScopedJavaLocalRef will release it automatically later.
+      return value_type::Adopt(
+          reader_->array_.env_,
+          static_cast<T>(reader_->array_.env_->GetObjectArrayElement(
+              reader_->array_.obj(), i_)));
+    }
+
+    pointer operator->() const { return pointer(operator*()); }
+
+    iterator& operator++() {
+      DCHECK(i_ < reader_->size_);
+      ++i_;
+      return *this;
+    }
+
+    iterator operator++(int) {
+      iterator old = *this;
+      ++*this;
+      return old;
+    }
+
+   private:
+    iterator(const JavaObjectArrayReader* reader, jsize i)
+        : reader_(reader), i_(i) {}
+    const JavaObjectArrayReader* reader_;
+    jsize i_;
+
+    friend JavaObjectArrayReader;
+  };
+
+  JavaObjectArrayReader(const JavaRef<jobjectArray>& array) : array_(array) {
+    size_ = array_.env_->GetArrayLength(array_.obj());
+  }
+
+  // Copy constructor to allow returning it from JavaRef::ReadElements().
+  JavaObjectArrayReader(const JavaObjectArrayReader& other) = default;
+
+  // Assignment operator for consistency with copy constructor.
+  JavaObjectArrayReader& operator=(const JavaObjectArrayReader& other) =
+      default;
+
+  // Allow move constructor and assignment since this owns a local ref.
+  JavaObjectArrayReader(JavaObjectArrayReader&& other) = default;
+  JavaObjectArrayReader& operator=(JavaObjectArrayReader&& other) = default;
+
+  bool empty() const { return size_ == 0; }
+
+  jsize size() const { return size_; }
+
+  iterator begin() const { return iterator(this, 0); }
+
+  iterator end() const { return iterator(this, size_); }
+
+ private:
+  ScopedJavaLocalRef<jobjectArray> array_;
+  jsize size_;
+
+  friend iterator;
+};
+
 }  // namespace android
 }  // namespace base
 

base/android/scoped_java_ref_unittest.cc

@@ -4,12 +4,15 @@
 
 #include "base/android/scoped_java_ref.h"
 
+#include <iterator>
+#include <type_traits>
+
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 #define EXPECT_SAME_OBJECT(a, b) \
-  EXPECT_TRUE(env->IsSameObject(a.obj(), b.obj()))
+  EXPECT_TRUE(env->IsSameObject((a).obj(), (b).obj()))
 
 namespace base {
 namespace android {
@@ -229,5 +232,112 @@ TEST_F(ScopedJavaRefTest, RefCounts) {
   EXPECT_EQ(0, g_global_refs);
 }
 
+class JavaObjectArrayReaderTest : public testing::Test {
+ protected:
+  void SetUp() override {
+    JNIEnv* env = AttachCurrentThread();
+    int_class_ = GetClass(env, "java/lang/Integer");
+    int_constructor_ = MethodID::Get<MethodID::TYPE_INSTANCE>(
+        env, int_class_.obj(), "<init>", "(I)V");
+    array_ = MakeArray(array_len_);
+
+    // Make array_len_ different Integer objects, keep a reference to each,
+    // and add them to the array.
+    for (jint i = 0; i < array_len_; ++i) {
+      jobject member = env->NewObject(int_class_.obj(), int_constructor_, i);
+      ASSERT_NE(member, nullptr);
+      array_members_[i] = ScopedJavaLocalRef<jobject>::Adopt(env, member);
+      env->SetObjectArrayElement(array_.obj(), i, member);
+    }
+  }
+
+  // Make an Integer[] with len elements, all initialized to null.
+  ScopedJavaLocalRef<jobjectArray> MakeArray(jsize len) {
+    JNIEnv* env = AttachCurrentThread();
+    jobjectArray array = env->NewObjectArray(len, int_class_.obj(), nullptr);
+    EXPECT_NE(array, nullptr);
+    return ScopedJavaLocalRef<jobjectArray>::Adopt(env, array);
+  }
+
+  static constexpr jsize array_len_ = 10;
+  ScopedJavaLocalRef<jclass> int_class_;
+  jmethodID int_constructor_;
+  ScopedJavaLocalRef<jobject> array_members_[array_len_];
+  ScopedJavaLocalRef<jobjectArray> array_;
+};
+
+// Must actually define the variable until C++17 :(
+constexpr jsize JavaObjectArrayReaderTest::array_len_;
+
+TEST_F(JavaObjectArrayReaderTest, ZeroLengthArray) {
+  JavaObjectArrayReader<jobject> zero_length(MakeArray(0));
+  EXPECT_TRUE(zero_length.empty());
+  EXPECT_EQ(zero_length.size(), 0);
+  EXPECT_EQ(zero_length.begin(), zero_length.end());
+  for (auto element : zero_length) {
+    FAIL() << "Loop body should not execute";
+  }
+}
+
+// Verify that we satisfy the C++ "InputIterator" named requirements.
+TEST_F(JavaObjectArrayReaderTest, InputIteratorRequirements) {
+  typedef JavaObjectArrayReader<jobject>::iterator It;
+
+  JNIEnv* env = AttachCurrentThread();
+  JavaObjectArrayReader<jobject> reader(array_);
+  It i = reader.begin();
+
+  EXPECT_TRUE(std::is_copy_constructible<It>::value);
+  It copy = i;
+  EXPECT_EQ(copy, i);
+  EXPECT_EQ(It(i), i);
+
+  EXPECT_TRUE(std::is_copy_assignable<It>::value);
+  It assign = reader.end();
+  It& assign2 = (assign = i);
+  EXPECT_EQ(assign, i);
+  EXPECT_EQ(assign2, assign);
+
+  EXPECT_TRUE(std::is_destructible<It>::value);
+
+  // Swappable
+  It left = reader.begin(), right = reader.end();
+  std::swap(left, right);
+  EXPECT_EQ(left, reader.end());
+  EXPECT_EQ(right, reader.begin());
+
+  // Basic check that iterator_traits works
+  bool same_type = std::is_same<std::iterator_traits<It>::iterator_category,
+                                std::input_iterator_tag>::value;
+  EXPECT_TRUE(same_type);
+
+  // Comparisons
+  EXPECT_EQ(reader.begin(), reader.begin());
+  EXPECT_NE(reader.begin(), reader.end());
+
+  // Dereferencing
+  ScopedJavaLocalRef<jobject> o = *(reader.begin());
+  EXPECT_SAME_OBJECT(o, array_members_[0]);
+  EXPECT_TRUE(env->IsSameObject(o.obj(), reader.begin()->obj()));
+
+  // Incrementing
+  It preinc = ++(reader.begin());
+  EXPECT_SAME_OBJECT(*preinc, array_members_[1]);
+  It postinc = reader.begin();
+  EXPECT_SAME_OBJECT(*postinc++, array_members_[0]);
+  EXPECT_SAME_OBJECT(*postinc, array_members_[1]);
+}
+
+// Check that range-based for and the convenience function work as expected.
+TEST_F(JavaObjectArrayReaderTest, RangeBasedFor) {
+  JNIEnv* env = AttachCurrentThread();
+
+  int i = 0;
+  for (ScopedJavaLocalRef<jobject> element : array_.ReadElements<jobject>()) {
+    EXPECT_SAME_OBJECT(element, array_members_[i++]);
+  }
+  EXPECT_EQ(i, array_len_);
+}
+
 }  // namespace android
 }  // namespace base

chrome/browser/android/payments/service_worker_payment_app_bridge.cc

@@ -191,9 +191,7 @@ std::vector<PaymentMethodDataPtr> ConvertPaymentMethodDataFromJavaToNative(
     JNIEnv* env,
     const JavaParamRef<jobjectArray>& jmethod_data) {
   std::vector<PaymentMethodDataPtr> result;
-  for (jsize i = 0; i < env->GetArrayLength(jmethod_data); i++) {
-    ScopedJavaLocalRef<jobject> element(
-        env, env->GetObjectArrayElement(jmethod_data, i));
+  for (auto element : jmethod_data.ReadElements<jobject>()) {
     PaymentMethodDataPtr method_data_item = PaymentMethodData::New();
     method_data_item->supported_method = ConvertJavaStringToUTF8(
         env,
@@ -253,9 +251,7 @@ PaymentRequestEventDataPtr ConvertPaymentRequestEventDataFromJavaToNative(
       env,
       Java_ServiceWorkerPaymentAppBridge_getValueFromPaymentItem(env, jtotal));
 
-  for (jsize i = 0; i < env->GetArrayLength(jmodifiers); i++) {
-    ScopedJavaLocalRef<jobject> jmodifier(
-        env, env->GetObjectArrayElement(jmodifiers, i));
+  for (auto jmodifier : jmodifiers.ReadElements<jobject>()) {
     PaymentDetailsModifierPtr modifier = PaymentDetailsModifier::New();
 
     ScopedJavaLocalRef<jobject> jmodifier_total =
@@ -360,9 +356,7 @@ static void JNI_ServiceWorkerPaymentAppBridge_CanMakePayment(
   event_data->method_data =
       ConvertPaymentMethodDataFromJavaToNative(env, jmethod_data);
 
-  for (jsize i = 0; i < env->GetArrayLength(jmodifiers); i++) {
-    ScopedJavaLocalRef<jobject> jmodifier(
-        env, env->GetObjectArrayElement(jmodifiers, i));
+  for (auto jmodifier : jmodifiers.ReadElements<jobject>()) {
     PaymentDetailsModifierPtr modifier = PaymentDetailsModifier::New();
 
     ScopedJavaLocalRef<jobject> jmodifier_total =

chrome/browser/android/provider/chrome_browser_provider.cc

@@ -917,10 +917,7 @@ ScopedJavaLocalRef<jobject> ChromeBrowserProvider::QueryBookmarkFromAPI(
   // Used to store the projection column names according their sequence.
   std::vector<std::string> columns_name;
   if (projection) {
-    jsize len = env->GetArrayLength(projection);
-    for (int i = 0; i < len; i++) {
-      ScopedJavaLocalRef<jstring> j_name(
-          env, static_cast<jstring>(env->GetObjectArrayElement(projection, i)));
+    for (auto j_name : projection.ReadElements<jstring>()) {
       std::string name = ConvertJavaStringToUTF8(env, j_name);
       history::HistoryAndBookmarkRow::ColumnID id =
           history::HistoryAndBookmarkRow::GetColumnID(name);
@@ -1059,10 +1056,7 @@ ScopedJavaLocalRef<jobject> ChromeBrowserProvider::QuerySearchTermFromAPI(
   // Used to store the projection column names according their sequence.
   std::vector<std::string> columns_name;
   if (projection) {
-    jsize len = env->GetArrayLength(projection);
-    for (int i = 0; i < len; i++) {
-      ScopedJavaLocalRef<jstring> j_name(
-          env, static_cast<jstring>(env->GetObjectArrayElement(projection, i)));
+    for (auto j_name : projection.ReadElements<jstring>()) {
       std::string name = ConvertJavaStringToUTF8(env, j_name);
       history::SearchRow::ColumnID id =
           history::SearchRow::GetColumnID(name);

chrome/browser/installable/installed_webapp_bridge.cc

@@ -26,13 +26,9 @@ InstalledWebappBridge::GetInstalledWebappNotificationPermissions() {
   JNIEnv* env = base::android::AttachCurrentThread();
   ScopedJavaLocalRef<jobjectArray> j_permissions =
       Java_InstalledWebappBridge_getNotificationPermissions(env);
-  jsize size = env->GetArrayLength(j_permissions.obj());
 
   InstalledWebappProvider::RuleList rules;
-  for (jsize i = 0; i < size; i++) {
-    ScopedJavaLocalRef<jobject> j_permission(
-        env, env->GetObjectArrayElement(j_permissions.obj(), i));
-
+  for (auto j_permission : j_permissions.ReadElements<jobject>()) {
     GURL origin(ConvertJavaStringToUTF8(
         Java_InstalledWebappBridge_getOriginFromPermission(env, j_permission)));
     ContentSetting setting = IntToContentSetting(