Implementing geolocation for the Android Webview

android_webview/native/aw_contents.cc

 // Copyright (c) 2012 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 "android_webview/native/aw_contents.h"
 
 #include <android/bitmap.h>
 #include <sys/system_properties.h>
 
 #include "android_webview/browser/aw_browser_context.h"
@@ skipping 197 matching lines @@
   SetWebContents(reinterpret_cast<content::WebContents*>(new_wc));
 }
 
 AwContents::~AwContents() {
   DCHECK(AwContents::FromWebContents(web_contents_.get()) == this);
   web_contents_->RemoveUserData(kAwContentsUserDataKey);
   if (find_helper_.get())
     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) {
 
   TRACE_EVENT0("AwContents", "AwContents::DrawGL");
 
   if (view_size_.IsEmpty() || !scissor_clip_layer_ ||
       draw_info->mode == AwDrawGLInfo::kModeProcess)
     return;
 
@@ skipping 476 matching lines @@
                  jobject obj,
                  jobject web_contents_delegate) {
   AwContents* tab = new AwContents(env, obj, web_contents_delegate);
   return reinterpret_cast<jint>(tab);
 }
 
 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());
 }
 
-void AwContents::OnGeolocationHidePrompt() {
-  // TODO(kristianm): Implement this
+}

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

// namespace

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

Done.

+
+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) {
   return GetFindHelper()->FindAllSync(
       ConvertJavaStringToUTF16(env, search_string));
 }
 
 void AwContents::FindAllAsync(JNIEnv* env, jobject obj, jstring search_string) {
   GetFindHelper()->FindAllAsync(ConvertJavaStringToUTF16(env, search_string));
 }
@@ skipping 338 matching lines @@
   if (!picture) {
     render_view_host_ext_->CapturePictureSync();
     picture = RendererPictureMap::GetInstance()->GetRendererPicture(
         web_contents_->GetRoutingID());
   }
 
   return picture;
 }
 
 }  // namespace android_webview