Separate SwipeRefreshHandler and ContentViewCore

chrome/browser/android/swipe_refresh_handler.cc

Index: chrome/browser/android/swipe_refresh_handler.cc
diff --git a/chrome/browser/android/swipe_refresh_handler.cc b/chrome/browser/android/swipe_refresh_handler.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0a8d191fb108738962acbf5068ad1e971054ab43
--- /dev/null
+++ b/chrome/browser/android/swipe_refresh_handler.cc
@@ -0,0 +1,65 @@
+// 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/swipe_refresh_handler.h"
+
+#include "jni/SwipeRefreshHandler_jni.h"
+
+using base::android::AttachCurrentThread;
+using base::android::JavaParamRef;
+using base::android::JavaRef;
+using base::android::ScopedJavaLocalRef;
+
+SwipeRefreshHandler::SwipeRefreshHandler(JNIEnv* env,
+                                         const JavaRef<jobject>& obj)
+    : java_ref_(env, obj) {}
+
+bool SwipeRefreshHandler::PullStart() {
+  JNIEnv* env = AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> obj = java_ref_.get(env);
+  if (!obj.is_null()) {
+    return Java_SwipeRefreshHandler_start(env, obj);
+  }
+  return false;
+}
+
+void SwipeRefreshHandler::PullUpdate(float delta) {
+  JNIEnv* env = AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> obj = java_ref_.get(env);
+  if (!obj.is_null()) {
+    Java_SwipeRefreshHandler_pull(env, obj, delta);
+  }
+}
+
+void SwipeRefreshHandler::PullRelease(bool allow_refresh) {
+  JNIEnv* env = AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> obj = java_ref_.get(env);
+  if (!obj.is_null()) {
+    Java_SwipeRefreshHandler_release(env, obj, allow_refresh);
+  }
+}
+
+void SwipeRefreshHandler::PullReset() {
+  JNIEnv* env = AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> obj = java_ref_.get(env);
+  if (!obj.is_null()) {
+    Java_SwipeRefreshHandler_reset(env, obj);
+  }
+}
+
+static jlong Init(JNIEnv* env, const JavaParamRef<jobject>& obj) {
+  SwipeRefreshHandler* swipe_refresh_handler =
+      new SwipeRefreshHandler(env, obj);
+  return reinterpret_cast<intptr_t>(swipe_refresh_handler);

[Jinsuk Kim, 2016/11/25 01:49:18]

Alternatively, you can set the created object to WebContentViewAndroid here. You
can get the native WebContents by passing the java WC to this method. 

Then the api SetORH needs to be defined in native rather than in Java. I think
the advantage of this approach is that Java ORH interface can be removed
entirely. It was sort of redundant since we already have its native counterpart.

[rlanday, 2016/11/28 19:42:01]

This seems potentially problematic to me; inside Tab, we have access to a
WebContents object, not a WebContentsImpl (which I think we need to get the
native pointer). I'm not super familiar with this code but my gut feeling is
that it seems cleaner to keep most of the logic in Java and perform the
appropriate native actions in parallel vs. doing stuff like directly calling
methods on the native WebContents from here.

If we do want to set the created object to WebContentViewAndroid when the
SwipeRefreshHandler is created, would it make sense to expose this functionality
on the Java WebContents?

[Jinsuk Kim, 2016/11/28 23:06:40]

YOu can add the api to native WebContents/WebContentsImpl instead of Java. I
kind of prefer doing things in native if possible. The big advantage is, again,
that then we can go without Java interface but only use native one.

+}
+
+void SwipeRefreshHandler::Destroy(JNIEnv* env,
+                                  const JavaParamRef<jobject>& obj) {
+  delete this;

[Jinsuk Kim, 2016/11/25 01:49:18]

With the refactoring I think this object better be owned by OverscrollRefresh
since OR is the only user of the instance, and ideally OverscrollRefreshHandler
should be deleted together with OR. Then Java SRH doesn't have to know/keep the
pointer to the native object. Let OR use unique_ptr to manage ORH.  

[rlanday, 2016/11/28 19:42:01]

This seems weird to me; I think the native SwipeRefreshHandler should live as
long as the Java SwipeRefreshHandler, otherwise we're introducing an additional
state to have to reason about (Java SwipeRefreshHandler exists but the native
SwipeRefreshHandler has been destructed). This strongly suggests having the Java
instance own the C++ instance.

[Jinsuk Kim, 2016/11/28 23:06:40]

You don't have to worry about the destruction order if OR owns the object and
delete it when it's done with it. Once it is deleted the flow ever reaches here,
and Java instance can be gc'ed any time. What if Destroy is called before OR
still needs to use the ORH...? one less thing to worry about. The reason it was
not owned by OR I think is just because the actual object was CVC whose lifetime
is not supposed to be in charge of OR.

+}
+
+// static
+bool SwipeRefreshHandler::RegisterSwipeRefreshHandler(JNIEnv* env) {
+  return RegisterNativesImpl(env);
+}