Implementing geolocation for the Android Webview

android_webview/native/aw_contents.cc

Index: android_webview/native/aw_contents.cc
diff --git a/android_webview/native/aw_contents.cc b/android_webview/native/aw_contents.cc
index a0438a31d714525a6d7f573ffb22b04e4d3edef4..8e017bc41b212157088fd999da06c099596263a9 100644
--- a/android_webview/native/aw_contents.cc
+++ b/android_webview/native/aw_contents.cc
@@ -727,20 +727,80 @@ bool RegisterAwContents(JNIEnv* env) {
   return RegisterNativesImpl(env) >= 0;
 }
 
-void AwContents::OnGeolocationShowPrompt(int render_process_id,
-                                       int render_view_id,
-                                       int bridge_id,
-                                       const GURL& requesting_frame) {
+namespace {
+
+void GeolocationShowPromptTask(JavaObjectWeakGlobalRef java_ref,
+                               const GURL& origin) {
+  LOG(INFO) << "Running task geo";

[benm (inactive), 2013/02/12 10:46:30]

nit: remove logging?

[Kristian Monsen, 2013/02/12 18:53:03]

Done.

   JNIEnv* env = AttachCurrentThread();
-  ScopedJavaLocalRef<jstring> j_requesting_frame(
-      ConvertUTF8ToJavaString(env, requesting_frame.spec()));
+  ScopedJavaLocalRef<jstring> j_origin(
+      ConvertUTF8ToJavaString(env, origin.spec()));
   Java_AwContents_onGeolocationPermissionsShowPrompt(env,
-      java_ref_.get(env).obj(), render_process_id, render_view_id, bridge_id,
-      j_requesting_frame.obj());
+                                                     java_ref.get(env).obj(),

[benm (inactive), 2013/02/12 10:46:30]

This will NPE if java_ref has been collected.

[Kristian Monsen, 2013/02/12 18:53:03]

Done.

+                                                     j_origin.obj());
+}
+
+void GeolocationShowPrompt(JavaObjectWeakGlobalRef java_ref,
+                           const GURL& origin) {
+    content::BrowserThread::PostTask(content::BrowserThread::UI,
+                                     FROM_HERE,
+                                     base::Bind(&GeolocationShowPromptTask,
+                                                java_ref,
+                                                origin));
+}
+
+} // anonymous namespace
+
+void AwContents::OnGeolocationShowPrompt(const GURL& requesting_frame,
+                                         base::Callback<void(bool)> callback) {
+  GURL origin = requesting_frame.GetOrigin();
+  bool show_prompt = pending_geolocation_prompts_.empty();
+  pending_geolocation_prompts_.push_back(OriginCallback(origin, callback));
+  if (show_prompt) {
+    GeolocationShowPrompt(java_ref_, origin);

[benm (inactive), 2013/02/12 10:46:30]

I think we should take a strong ref to the java_ref_ or make
GeolocationShowPromptTask robust to receiving a null weak ref.

I'd be inclined to take a Global ref here, early out if it's null to avoid
posting unnecessary tasks, and then clear up the exta ref when we return from
java with the answer to the prompt. You can then use const JavaRef<jobject>& as
your parameter type for the helpers.

Same in Invoke and Hide below.

[Kristian Monsen, 2013/02/12 18:53:03]

I have checked if the weak pointer is not null before using. I don't think it
makes sense to take the global ref as it doesn't make sense to call the prompt
if the ref is going away.

[benm (inactive), 2013/02/12 19:03:21]

I don't think checking for null is enough, it may get collected after you've
checked for null. The only way to ensure the GC doesn't collect is if there is a
strong ref to the object.

[Kristian Monsen, 2013/02/12 19:17:20]

Right, but this is happening when you call get() (I think?), and it creates a
local ref. Updated to keep the ref inside the method.

[boliu, 2013/02/12 19:18:47]

Sorry, I got confused this morning, I confused WeakPtr with WeakRef. Do what Ben
says.

[Ben Murdoch, 2013/02/12 19:21:05]

Yes, get() will return a ScopedJavaLocalRef. I just looked at the patch again
and see that you are taking one of those (only spotted the null check last
time), so I think it should be fine now.

[Kristian Monsen, 2013/02/12 19:23:38]

I updated after your comment as I realized it could go bad between the if and
the use :-) Sorry if that was not clear.

+  }
 }
 
-void AwContents::OnGeolocationHidePrompt() {
-  // TODO(kristianm): Implement this
+void AwContents::InvokeGeolocationCallback(JNIEnv* env,
+                                           jobject obj,
+                                           jboolean value,
+                                           jstring origin) {
+  GURL callback_origin(base::android::ConvertJavaStringToUTF16(env, origin));
+  if (callback_origin.GetOrigin() ==
+      pending_geolocation_prompts_.front().first) {
+    pending_geolocation_prompts_.front().second.Run(value);
+    pending_geolocation_prompts_.pop_front();
+    if (!pending_geolocation_prompts_.empty()) {
+      GeolocationShowPrompt(java_ref_,
+                            pending_geolocation_prompts_.front().first);
+    }
+  }
+}
+
+void AwContents::OnGeolocationHidePrompt(const GURL& origin) {
+  bool removed_current_outstanding_callback = false;
+  std::list<OriginCallback>::iterator it = pending_geolocation_prompts_.begin();
+  while (it != pending_geolocation_prompts_.end()) {
+    if ((*it).first == origin.GetOrigin()) {
+      if (it == pending_geolocation_prompts_.begin()) {
+        removed_current_outstanding_callback = true;
+      }
+      it = pending_geolocation_prompts_.erase(it);
+    } else {
+      it++;

[benm (inactive), 2013/02/12 10:46:30]

nit: style guide says to pre-increment.

[Kristian Monsen, 2013/02/12 18:53:03]

Done.

+    }
+  }
+
+  if (removed_current_outstanding_callback) {
+    JNIEnv* env = AttachCurrentThread();
+    Java_AwContents_onGeolocationPermissionsHidePrompt(
+        env,
+        java_ref_.get(env).obj());
+    if (!pending_geolocation_prompts_.empty()) {
+      GeolocationShowPrompt(java_ref_,
+                            pending_geolocation_prompts_.front().first);
+    }
+  }
 }
 
 jint AwContents::FindAllSync(JNIEnv* env, jobject obj, jstring search_string) {