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 28f6f18f60a53e9b2830449f39eee84aeee3c10a..19b9630aac3a1de57c9b0bd6a0c802205096149a 100644
--- a/android_webview/native/aw_contents.cc
+++ b/android_webview/native/aw_contents.cc
@@ -215,6 +215,7 @@ AwContents::~AwContents() {
     find_helper_->SetListener(NULL);
   if (icon_helper_.get())
     icon_helper_->SetListener(NULL);
+  geolocation_callbacks_.clear();

[boliu, 2013/02/06 20:29:54]

is this necessary?

[Kristian Monsen, 2013/02/08 00:07:44]

When thinking about it, no. It happens automatically when the list goes out of
scope.

 }
 
 void AwContents::DrawGL(AwDrawGLInfo* draw_info) {
@@ -711,20 +712,59 @@ 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()));
-  Java_AwContents_onGeolocationPermissionsShowPrompt(env,
-      java_ref_.get(env).obj(), render_process_id, render_view_id, bridge_id,
-      j_requesting_frame.obj());
+  ScopedJavaLocalRef<jstring> j_origin(
+      ConvertUTF8ToJavaString(env,origin.spec()));
+  Java_AwContents_onGeolocationPermissionsShowPrompt(
+      env, java_ref.get(env).obj(), j_origin.obj());
+}
+
 }

[boliu, 2013/02/06 20:29:54]

// namespace

[Kristian Monsen, 2013/02/08 00:07:44]

Done.

 
-void AwContents::OnGeolocationHidePrompt() {
-  // TODO(kristianm): Implement this
+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));

[boliu, 2013/02/06 20:29:54]

So using a list means if there are multiple requests for the same origin,
they'll get shown multiple times?

Can we make it shown once and if the decision is permanent, then not show again?

[joth, 2013/02/06 22:09:51]

onGeolocationPermissionsShowPrompt on java side already does this. (called from
GeolocationShowPromp for the item at front of the queue)

+  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);
+    }
+  }
+}
+
+void AwContents::OnGeolocationHidePrompt(const GURL& origin) {
+  std::list<OriginCallback>::iterator it = geolocation_callbacks_.begin();
+  bool removed_current_outstanding_callback = false;
+  for ( ; it != geolocation_callbacks_.end(); ++it) {
+    if ((*it).first == origin.GetOrigin()) {
+      it = geolocation_callbacks_.erase(it);
+      if (it == geolocation_callbacks_.begin()) {

[joth, 2013/02/06 22:09:51]

'it' will be invalidated by the erase() call, so you can't compare it to begin()
here nor can you call ++it when looping around.

feels like we're missing test coverage...
(maybe cleanest answer is to factor out the geolocation piece into its own
dedicated class in aw/browser and can then write a native only unit test for it.

[Kristian Monsen, 2013/02/08 00:07:44]

seting it to the next element after the erase, but you are right it was
incremented twice in that case. Updated.

And yes more test coverage would be nice, as it is hard to make the cts test
cover the corner cases.

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

Right - We have upstream instrumentation tests and unit tests for just this sort
of thing. Certainly don't want to abuse CTS for implementation specific
(white-box) code paths like this.

This is an argument for extracting geolocation_callbacks_ and functionality that
uses it into its own clearly defined (and testable) class rather than add more
bloat to AwContents.

+        removed_current_outstanding_callback = true;
+      }
+    }
+  }
+  if (removed_current_outstanding_callback) {

[boliu, 2013/02/06 20:29:54]

Can removed_current_outstanding_callback ever be false here? If not, then might
be better to use a DCHECK.

[Kristian Monsen, 2013/02/08 00:07:44]

Yes, if it was cancelled before the callback came back here.

+    JNIEnv* env = AttachCurrentThread();
+    Java_AwContents_onGeolocationPermissionsHidePrompt(
+        env, java_ref_.get(env).obj());
+    GeolocationShowPrompt(
+        java_ref_, geolocation_callbacks_.front().first);

[boliu, 2013/02/06 20:29:54]

check front() is not null?

[Kristian Monsen, 2013/02/08 00:07:44]

Good catch! Adding an empty list check.

+  }
 }
 
 jint AwContents::FindAllSync(JNIEnv* env, jobject obj, jstring search_string) {