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..14bb861a015fb3f955d2fece73a928ef9e3ad7fc 100644
--- a/android_webview/native/aw_contents.cc
+++ b/android_webview/native/aw_contents.cc
@@ -727,20 +727,68 @@ 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 GeolocationShowPrompt(JavaObjectWeakGlobalRef java_ref,
+                           const GURL& origin) {
   JNIEnv* env = AttachCurrentThread();
-  ScopedJavaLocalRef<jstring> j_requesting_frame(
-      ConvertUTF8ToJavaString(env, requesting_frame.spec()));
+  ScopedJavaLocalRef<jstring> j_origin(
+      ConvertUTF8ToJavaString(env,origin.spec()));

[joth, 2013/02/08 02:15:01]

nit: space after comma

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

Done.

   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(),
+                                                     j_origin.obj());
 }
 
-void AwContents::OnGeolocationHidePrompt() {
-  // TODO(kristianm): Implement this
+} // anonymous namespace
+
+void AwContents::OnGeolocationShowPrompt(const GURL& requesting_frame,
+                                         base::Callback<void(bool)> callback) {
+  const GURL& origin = requesting_frame.GetOrigin();
+  bool show_prompt = geolocation_callbacks_.empty();
+  geolocation_callbacks_.push_back(OriginCallback(origin, callback));
+  if (show_prompt) {
+    GeolocationShowPrompt(java_ref_, origin);
+  }
+}
+
+void AwContents::InvokeGeolocationCallback(JNIEnv* env,
+                                           jobject obj,
+                                           jboolean value,
+                                           jstring origin) {
+  GURL callback_origin(base::android::ConvertJavaStringToUTF16(env, origin));
+  if (callback_origin.GetOrigin() == geolocation_callbacks_.front().first) {
+    geolocation_callbacks_.front().second.Run(value);
+    geolocation_callbacks_.pop_front();
+    if (!geolocation_callbacks_.empty()) {
+      GeolocationShowPrompt(java_ref_, geolocation_callbacks_.front().first);

[joth, 2013/02/08 02:15:01]

this could create a callstack as deep as the number of items in the queue (i.e.
if the app synchronously completes the ShowPrompt it will call right back into
InvokeGeolocationCallback and process the next list item, until the list is
empty) - this will also happen if the origin already has a retained result, or
if geolocation was disabled in AwSettings.


this is sufficiently corner case I don't think it matters, but if we cared, we
could turn GeolocationShowPrompt into an async method (call via PostTask)
or... switch the java side to use ThreadUtils.postOnUiThread (i.e. to always
yield callstack) but that means we'd also always need to push the result via the
AwGeolocationCallback instance (even when the result from a retained answer or
AwSettings disabled geolocation)
... hmmm that would have other marginal benefits, such as mean in the call to
Java_AwContents_onGeolocationPermissionsHidePrompt below that app could not
possibly synchronously call into InvokeGeolocationCallback and mess up the list
status either.

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

Converted showprompt into a task, that actually found a bug that would have
showed up in real apps, so well worth doing :-)

+    }
+  }
+}
+
+void AwContents::OnGeolocationHidePrompt(const GURL& origin) {
+  bool removed_current_outstanding_callback = false;
+  std::list<OriginCallback>::iterator it = geolocation_callbacks_.begin();
+  for ( ; it != geolocation_callbacks_.end(); ) {

[joth, 2013/02/08 02:15:01]

This construct has it's very own special syntax:

while (it != geolocation_callbacks_.end()) {


:-)

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

Done.

+    if ((*it).first == origin.GetOrigin()) {
+      it = geolocation_callbacks_.erase(it);
+      if (it == geolocation_callbacks_.begin()) {

[joth, 2013/02/08 02:15:01]

you still need to make this check *prior* to updating |it| though?

something like:

it = list_.begin();
while (it != list_.end()) {
  if (*it == origin.GetOrigin()) {
    removed_current_outstanding_callback |= it == list_.begin();
    it = list_.erase(it);
  } else {
    ++it;
  }
}

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

I think it should fine, I did think of that. For the next iterator to be == to
list.begin(), you must have called erase on the first element (possibly several
new first elements, but that is ok).

The fact that you were not sure means it is not readable code, rewritten.

I don't think:
removed_current_outstanding_callback |= it == list_.begin();
can be used. What if the first two elements are removed (assuming after you
delete the front of the list the next call to begin() will point to the iterator
to the new front) ? 

Another thing here is that this all could be simplified if we knew if there can
be several request for a specific origin. I thought no, but wrote the code as if
yes since I was not sure.

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