Implementing geolocation for the Android Webview

android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java

Index: android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java
diff --git a/android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java b/android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java
index 133f5970eb5a12a778701aba59b3f329e39941e0..f0afdca89d7c165d5e41375d47e815c8a4de873f 100644
--- a/android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java
+++ b/android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java
@@ -24,10 +24,39 @@ public final class AwGeolocationPermissions {
             AwGeolocationPermissions.class.getCanonicalName() + "%";
     private final SharedPreferences mSharedPreferences;
 
+    private static AwGeolocationPermissions sInstance;

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

crumbs more statics. I'll make a patch to avoid the need for this. I plan to add
new class AwBrowserContext, and a getGeolocationPermissions method there on.

in meantime, you can just leave the glue layer using the public c'tor. 

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

OK.

+
+    // TODO(kristianm): Make this private once framework is updated to use getInstance

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

Sorry I don't understand this comment. What framework are you referring to? This
is an aw class so doesn't that mean all instances of creating this should be in
chromium and can be fixed in this patch?

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

Android -> framework/webview. That is where this is created. I need to be able
to get to the instance from AwContents.java.

     public AwGeolocationPermissions(SharedPreferences sharedPreferences) {
         mSharedPreferences = sharedPreferences;
+        sInstance = this;
+    }
+
+    /**
+     * Get the static instance after it has been created
+     */
+    public static synchronized AwGeolocationPermissions getInstance() {

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

Can we use synchronized blocks instead of having the whole function be
synchornized? Just not wanting to commit the api to it. Same below

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

Done.

+        if (sInstance == null) {
+            throw new IllegalStateException("This should only be called after creteInstance.");
+        }
+        return sInstance;
+    }
+
+    /**
+     * Create the static instance of this class
+     */
+    public static synchronized AwGeolocationPermissions createInstance(

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

getInstance vs createInstance? I'm confused if this is a singleton or not

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

It needs a SharedPreference when creating the static instance, but that is hard
to get to later, so that is why there is a getInstance() with parameters. I
thought it was clearer if one is named getInstance and the other
createInstance(SharedPreferences)

+            SharedPreferences sharedPreferences) {
+        if (sInstance != null) {
+            throw new IllegalStateException("This should only be called once.");
+        }
+        sInstance = new AwGeolocationPermissions(sharedPreferences);

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

sInstance is set here and in the constructor, duplicated?

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

Yes, done.

I wanted it here, but had to be in the ctor as well since that is the once
called by android framework/webview. Removed it from here from now.

+        return sInstance;
     }
 
+    /**
+     * Set one origin to be allowed.
+     */
     public void allow(String origin) {
         String key = getOriginKey(origin);
         if (key != null) {
@@ -35,6 +64,9 @@ public final class AwGeolocationPermissions {
         }
     }
 
+    /**
+     * Set one origin to be denied.
+     */
     public void deny(String origin) {
         String key = getOriginKey(origin);
         if (key != null) {
@@ -42,6 +74,9 @@ public final class AwGeolocationPermissions {
         }
     }
 
+    /**
+     * Clear one origin from being allowed and denied.
+     */
     public void clear(String origin) {
         String key = getOriginKey(origin);
         if (key != null) {
@@ -49,6 +84,9 @@ public final class AwGeolocationPermissions {
         }
     }
 
+    /**
+     * Clear all origins set to allowed or denied.
+     */
     public void clearAll() {
         SharedPreferences.Editor editor = null;
         for (String name : mSharedPreferences.getAll().keySet()) {
@@ -64,7 +102,10 @@ public final class AwGeolocationPermissions {
         }
     }
 
-    public void getAllowed(String origin, final ValueCallback<Boolean> callback) {
+    /**
+     * Sync method to get if an origin set to be allowed.
+     */
+    public boolean getAllowed(String origin) {

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

I think isOriginAllowed / isOriginDenied would be better for both these.

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

Done.

         boolean allowed = false;
         try {
             String key = getOriginKey(origin);
@@ -74,7 +115,30 @@ public final class AwGeolocationPermissions {
         } catch (ClassCastException e) {
             // Want to return false in this case, do nothing here
         }
-        final boolean finalAllowed = allowed;
+        return allowed;

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

AIUI passing key == null to getFoo() is fine so you can just say:


try {
    return mSharedPreferences.getBoolean(getOriginKey(origin), false);
} catch (ClassCastException e) {
  return false;
}

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

Done.

+    }
+
+    /**
+     * Sync method to get if an origin set to be denied.
+     */
+    public boolean getDenied(String origin) {
+        boolean allowed = true;
+        try {
+            String key = getOriginKey(origin);
+            if (key != null) {
+                allowed = mSharedPreferences.getBoolean(key, true);

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

the 'true' here & above looks very odd.

This comes about because  getDenied() returning false means "unknown", not
"allowed". i.e. not the most obvious API.


Would it be simpler to have a pair of methods hasOrgin() and isOriginAllowed() ?

that makes the callsite much simpler too:

if (permissions.hasOrigin(origin)) {
  nativeInvokeGeolocationCallback(mNativeAwContents,
          permissions.isOriginAllowed(origin), origin);
  return;
}

mClient.showPrompt(..);

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

Updated as it makes the only callsite clearer, but I think is still as unclear
as before. isOriginAllowed() will return true or unknown. It could return a
Boolean with null being the doesn't exist state but I am not sure if that would
be clearer.

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

agreed. we might as well invent an enum (int consts) if we went that route...

I like the code as you have it now  (Of course, I suggested it...:) And I don't
think it's unclear. isOriginAllowed() returning false is very unambiguous: if
the origin has not been explicitly allowed, then it is not allowed. In most
contexts your don't need to care if it was explicitly denied or is just
defaulting to false, and in the odd case you do need to know, just use
hasOrigin().

+            }
+        } catch (ClassCastException e) {
+            // Want to return false in this case, do nothing here
+        }
+        return !allowed;
+    }
+
+    /**
+     * ASync method to get if an origin set to be allowed.
+     */
+    public void getAllowed(String origin, final ValueCallback<Boolean> callback) {
+        final boolean finalAllowed = getAllowed(origin);

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

given this is async, should this be more async that getAllowed be called in the
Runnable below? Similar below.

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

I felt this was better as the result will reflect what is true at calling time.
Using SharedPreferences on the UI thread is fine I think.

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

Seems reasonable. Otherwise, calling this sequence on non-ui thread:
geo.clearAll().
geo.getAllowed(...)
geo.allow(foo)

could be racy (foo may or may not be in the set), but as you have it the answer
is always deterministic and as expected.

         ThreadUtils.postOnUiThread(new Runnable() {
             @Override
             public void run() {
@@ -83,6 +147,9 @@ public final class AwGeolocationPermissions {
         });
     }
 
+    /**
+     * ASync method to get the domains currently allowed or denied.
+     */
     public void getOrigins(final ValueCallback<Set<String>> callback) {
         final Set<String> origins = new HashSet<String>();
         for (String name : mSharedPreferences.getAll().keySet()) {
@@ -98,6 +165,9 @@ public final class AwGeolocationPermissions {
         });
     }
 
+    /**
+     * Get the domain of an URL using the GURL library.
+     */
     private String getOriginKey(String url) {
         String origin = GURLUtils.getOrigin(url);
         if (origin.isEmpty()) {