Implement SmartText selection.

content/browser/android/context_selection_client.cc

+// Copyright 2017 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 "content/browser/android/context_selection_client.h"
+
+#include "base/android/jni_android.h"
+#include "base/android/jni_string.h"
+#include "base/bind.h"
+#include "content/public/browser/render_frame_host.h"
+#include "jni/ContextSelectionClient_jni.h"
+
+using base::android::AttachCurrentThread;
+using base::android::ConvertUTF16ToJavaString;
+using base::android::JavaParamRef;
+using base::android::ScopedJavaLocalRef;
+
+namespace content {
+
+namespace {
+const void* const kContextSelectionClientUDKey = &kContextSelectionClientUDKey;
+}
+
+// This class deletes ContextSelectionClient when WebContents is destroyed.
+class ContextSelectionClient::UserData : public base::SupportsUserData::Data {
+ public:
+  explicit UserData(ContextSelectionClient* client) : client_(client) {}
+
+ private:
+  std::unique_ptr<ContextSelectionClient> client_;
+
+  DISALLOW_IMPLICIT_CONSTRUCTORS(UserData);
+};
+
+jlong Init(JNIEnv* env,
+           const JavaParamRef<jobject>& obj,
+           const JavaParamRef<jobject>& jweb_contents) {
+  WebContents* web_contents = WebContents::FromJavaWebContents(jweb_contents);
+  CHECK(web_contents)
+      << "A ContextSelectionClient should be created with a valid WebContents.";
+
+  return reinterpret_cast<intptr_t>(

[Ted C, 2017/03/29 17:43:50]

since this is using UserData, what is the expected behavior if multiple clients
attempt to create one of these with the same web contents?  Is the expected
behavior that it should clobber?  Should we only allow one?

I'm wondering if this should first check for the existence of a user data and
return it before attempting to create a new one.  (then Init would need to be
something like GetOrCreate or just Get with the idea that it will lazily create
on the first attempt).

[Tima Vaisburd, 2017/03/29 22:56:36]

I wanted to allow only one, and I thought the DCHECK() at l.53 would do that.
Or, do you envision having multiple such clients in the future?

[Ted C, 2017/03/30 00:12:39]

The API still seems quite weird.  Having the java api just be named create#, it
doesn't seem clear to me that this should and can only be called a single time.

I guess I just don't understand why we are using supports user data at all.  To
me, that is useful to get and fetch the same object over time.  Here we are
using it purely as a signal for when the WebContents gets destroyed (which to me
should be a WebContentsObserver).

Nothing about this is really broken though, so this is fine for now and we can
change it later if needed.

[Tima Vaisburd, 2017/03/30 01:03:13]

Acknowledged.

+      new ContextSelectionClient(env, obj, web_contents));
+}
+
+ContextSelectionClient::ContextSelectionClient(

[Ted C, 2017/03/29 17:43:50]

actually, this is only created via init, so should it be private as well?

[Tima Vaisburd, 2017/03/29 22:56:36]

Init() is not a class member, it is a stand-alone function, hence I need a
public constructor.

[Ted C, 2017/03/30 00:12:39]

Acknowledged.

+    JNIEnv* env,
+    const base::android::JavaRef<jobject>& obj,
+    WebContents* web_contents)
+    : java_ref_(env, obj),
+      web_contents_(web_contents),
+      weak_ptr_factory_(this) {
+  DCHECK(!web_contents_->GetUserData(kContextSelectionClientUDKey));
+  web_contents_->SetUserData(kContextSelectionClientUDKey, new UserData(this));
+}
+
+ContextSelectionClient::~ContextSelectionClient() {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> j_obj = java_ref_.get(env);
+  java_ref_.reset();

[Ted C, 2017/03/29 17:43:50]

do you need to do this?  wouldn't this happen as part of the natural destruction
order?

[Tima Vaisburd, 2017/03/29 22:56:36]

Removed.

+  if (!j_obj.is_null()) {
+    Java_ContextSelectionClient_onNativeSideDestroyed(
+        env, j_obj, reinterpret_cast<intptr_t>(this));
+  }
+}
+
+void ContextSelectionClient::RequestSurroundingText(
+    JNIEnv* env,
+    const JavaParamRef<jobject>& obj,
+    int callback_data) {
+  RenderFrameHost* focused_frame = web_contents_->GetFocusedFrame();
+  if (!focused_frame) {
+    OnSurroundingTextReceived(callback_data, base::string16(), 0, 0);
+    return;
+  }
+
+  constexpr int EXTRA_CHARS = 100;

[Ted C, 2017/03/29 17:43:50]

should this be configurable w/ a upper bounds limit?  more for the contextual
search side of things

[Tima Vaisburd, 2017/03/29 22:56:36]

I changed to pass this value with every call to RequestSurroundingText(), in
Java it is still a constant 100.
Please let me know what did you have in mind for contextual search.

+  focused_frame->RequestTextSurroundingSelection(
+      base::Bind(&ContextSelectionClient::OnSurroundingTextReceived,
+                 weak_ptr_factory_.GetWeakPtr(), callback_data),
+      EXTRA_CHARS);
+}
+
+void ContextSelectionClient::CancelAllRequests(
+    JNIEnv* env,
+    const base::android::JavaParamRef<jobject>& obj) {
+  weak_ptr_factory_.InvalidateWeakPtrs();
+}
+
+void ContextSelectionClient::OnSurroundingTextReceived(
+    int callback_data,
+    const base::string16& text,
+    int start,
+    int end) {
+  JNIEnv* env = AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> obj = java_ref_.get(env);
+  if (!obj.is_null()) {
+    ScopedJavaLocalRef<jstring> j_text = ConvertUTF16ToJavaString(env, text);
+    Java_ContextSelectionClient_onSurroundingTextReceived(
+        env, obj, callback_data, j_text, start, end);
+  }
+}
+
+bool RegisterContextSelectionClient(JNIEnv* env) {
+  return RegisterNativesImpl(env);
+}
+
+}  // namespace content