Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11416347:
Adding java implementation for Geolocation (Closed)
|
Created:
8 years ago by Kristian Monsen Modified:
8 years ago CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding java implementation for Geolocation
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173328
Patch Set 1 #
Total comments: 36
Patch Set 2 : Updated to use apply and shared prefs #
Total comments: 13
Patch Set 3 : Updated ThreadUtils and got results before moving to UI thread #
Total comments: 9
Patch Set 4 : Code dereplication #Patch Set 5 : Boolean pref edition, I think I like it better. I think the lock can be removed as well? #Patch Set 6 : Removed not needed if #
Total comments: 10
Patch Set 7 : Removed the lock #
Total comments: 4
Patch Set 8 : Add prefix in origin conversion, changed class name to have an Aw prefix to avoid name collision #Patch Set 9 : Removed not needed imports #Patch Set 10 : Finding origin with native code and Gurl #Patch Set 11 : Removed non existent imports #Patch Set 12 : Moved the JNI to net/ and renamed to GurlUtils #
Total comments: 12
Patch Set 13 : Renamed GurlUtils -> GURLUtils, other minor edits #Patch Set 14 : Updated comment and copyright date #
Total comments: 6
Messages
Total messages: 41 (0 generated)
https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... File android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java (right): https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:23: * All methods in this class are async. What is the requirement for them being async? Is it to avoid the IO when writing SharedPreferences to disk? Or a legacy compatibility requirement? https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:33: private static final Object lock = new Object(); Can be a member? https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:35: public GeolocationPermissions(Context appContext) { nit: sp/appContext/context/ - there's no requirement that the user provide an application context instead of any other type of context, is there? https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:47: boolean added = copiedOrigins.add(origin); nit: don't think the local adds much, could just inline in the if condition https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:51: editor.commit(); If we used editor.apply() instead, would we need to start a new thread for this (and other functions) https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:74: editor.commit(); nit: could have a single commit outside both ifs https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:110: callback.onReceiveValue(true); callback.onReceiveValue(allowedSet.contains(origin)) :) https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:132: // Method to get the origin in a standarized form, mostlt taken from sp mostly https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:133: // android.net.WebAddress.toString, except not using the path element. nit: "except the path element is not required" is a bit clearer i think.
https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... File android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java (right): https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:25: @JNINamespace("android_webview") not needed as there's no JNI here... except, see my comment on getOrigin below. (and if we extract that to a URL util class, we'd still have no JNI here) https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:36: mSharedPreferences = appContext.getSharedPreferences(PREF_NAME, Context.MODE_PRIVATE); as discussed, lets do this in the factory and pass the sharedprefs instance in here. the |key| names need to be more unique then. perhaps PREF_ALLOWED_ORIGINS = "geolocation.AllowedOrgins"; https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:40: Thread t = new Thread() { instead of these 5 places you make a short-lived thread, instead try: AsyncTask.THREAD_POOL_EXECUTOR.execute(new Runnable() { ... }); https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:105: public void run() { do we actually need to make this async? reading from SharedPrefs is a memory only operation, so I'd be tempted to just do it right here without the new Thread, and then use Looper.myLooper().post() to complete the callback asynchronously. https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:108: PREF_ALLOWED_ORIGINS, new HashSet<String>()); you can pass null rather than 'new HashSet<String>' but just be aware the return value may be null then. https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:112: callback.onReceiveValue(false); do you know what thread legacy implementation called |callback| on? I feel it should either be the caller's thread or the UI thread, rather than random short lived thread. https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:120: Thread t = new Thread() { again probably no need to spawn a thread https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:134: private String getOrigin(String domain) { please make this into a native method, and use GURL::GetOrigin() ? chromium makes a big deal about having a single URL parsing library. we may still want to pull out a common URL java-side wrapper of utils. as you need to plumb up the JNI for this, maybe now's the time. Maybe net/android/java/src/org/chromium/net/URLUtils.java net/android/url_utils.h/cc (if you want to leave this for a follow up CL I'm OK with that too)
https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... File android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java (right): https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:40: Thread t = new Thread() { On 2012/12/06 01:52:33, joth wrote: > instead of these 5 places you make a short-lived thread, instead try: > > AsyncTask.THREAD_POOL_EXECUTOR.execute(new Runnable() { > ... > }); actually...... couple other things to consider here: - if we used AsyncTask.SERIAL_EXECUTOR it would avoid the need for the lock object - if we post all tasks to that executor (contrary to my previous comments about doing the reads on caller thread) then it means we'd have well defined sequencing (e.g. write(x) follow by read(x), the read is sure to read the value written) That way, no child threads needed at all. Although! I just saw ben's comment about apply(). I like that even better (I thought there was something like this so was confused before when I found commit() would block the caller)
Thank you for the reviews, I think I have addressed all comments. There code has changed a bit, so some comments did not apply anymore. https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... File android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java (right): https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:23: * All methods in this class are async. On 2012/12/05 11:35:05, benm wrote: > What is the requirement for them being async? Is it to avoid the IO when writing > SharedPreferences to disk? Or a legacy compatibility requirement? Both, might get called on the main thread so better to not write on it. Updated to say that callbacks are posted on the main thread. https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:25: @JNINamespace("android_webview") On 2012/12/06 01:52:33, joth wrote: > not needed as there's no JNI here... except, see my comment on getOrigin below. > (and if we extract that to a URL util class, we'd still have no JNI here) Done. https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:33: private static final Object lock = new Object(); On 2012/12/05 11:35:05, benm wrote: > Can be a member? It is static by design, this way if you have two instances they will be synchronized (they would use the same prefs). I am not sure how likely this is to cause problems, but thought it was best to do it correct. https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:35: public GeolocationPermissions(Context appContext) { On 2012/12/05 11:35:05, benm wrote: > nit: sp/appContext/context/ - there's no requirement that the user provide an > application context instead of any other type of context, is there? Done. It is not required, and it probably doesn't matter since we just use it right away. https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:36: mSharedPreferences = appContext.getSharedPreferences(PREF_NAME, Context.MODE_PRIVATE); On 2012/12/06 01:52:33, joth wrote: > as discussed, lets do this in the factory and pass the sharedprefs instance in > here. > > the |key| names need to be more unique then. perhaps > PREF_ALLOWED_ORIGINS = "geolocation.AllowedOrgins"; Done. https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:40: Thread t = new Thread() { On 2012/12/06 02:08:11, joth wrote: > On 2012/12/06 01:52:33, joth wrote: > > instead of these 5 places you make a short-lived thread, instead try: > > > > AsyncTask.THREAD_POOL_EXECUTOR.execute(new Runnable() { > > ... > > }); > > actually...... couple other things to consider here: > - if we used AsyncTask.SERIAL_EXECUTOR it would avoid the need for the lock > object > - if we post all tasks to that executor (contrary to my previous comments about > doing the reads on caller thread) then it means we'd have well defined > sequencing (e.g. write(x) follow by read(x), the read is sure to read the value > written) > That way, no child threads needed at all. > > Although! I just saw ben's comment about apply(). I like that even better (I > thought there was something like this so was confused before when I found > commit() would block the caller) Changed to apply after I found out it can be used for putStringSet https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:47: boolean added = copiedOrigins.add(origin); On 2012/12/05 11:35:05, benm wrote: > nit: don't think the local adds much, could just inline in the if condition Done. https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:51: editor.commit(); On 2012/12/05 11:35:05, benm wrote: > If we used editor.apply() instead, would we need to start a new thread for this > (and other functions) Yes, but from the documentation it doesn't look like apply() can be used for putStringSet(). It is a bit unclear: http://developer.android.com/reference/android/content/SharedPreferences.Edit... I asked around, and it is a documentation typo, so using apply. https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:74: editor.commit(); On 2012/12/05 11:35:05, benm wrote: > nit: could have a single commit outside both ifs Commit can be very expensive. Changed to apply now, but keeping the single apply. https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:105: public void run() { On 2012/12/06 01:52:33, joth wrote: > do we actually need to make this async? reading from SharedPrefs is a memory > only operation, so I'd be tempted to just do it right here without the new > Thread, and then use Looper.myLooper().post() to complete the callback > asynchronously. Calling it on the UI thread. https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:108: PREF_ALLOWED_ORIGINS, new HashSet<String>()); On 2012/12/06 01:52:33, joth wrote: > you can pass null rather than 'new HashSet<String>' but just be aware the return > value may be null then. Used it here, but in most other cases, there would be a lot of null checks. https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:110: callback.onReceiveValue(true); On 2012/12/05 11:35:05, benm wrote: > callback.onReceiveValue(allowedSet.contains(origin)) :) Done. https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:112: callback.onReceiveValue(false); On 2012/12/06 01:52:33, joth wrote: > do you know what thread legacy implementation called |callback| on? I feel it > should either be the caller's thread or the UI thread, rather than random short > lived thread. On the UI thread, updated to post a message there. https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:120: Thread t = new Thread() { On 2012/12/06 01:52:33, joth wrote: > again probably no need to spawn a thread Also on the UI thread. https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:132: // Method to get the origin in a standarized form, mostlt taken from On 2012/12/05 11:35:05, benm wrote: > sp mostly Done. https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:133: // android.net.WebAddress.toString, except not using the path element. On 2012/12/05 11:35:05, benm wrote: > nit: "except the path element is not required" is a bit clearer i think. Done. https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:134: private String getOrigin(String domain) { On 2012/12/06 01:52:33, joth wrote: > please make this into a native method, and use GURL::GetOrigin() ? chromium > makes a big deal about having a single URL parsing library. > > we may still want to pull out a common URL java-side wrapper of utils. as you > need to plumb up the JNI for this, maybe now's the time. > Maybe > net/android/java/src/org/chromium/net/URLUtils.java > net/android/url_utils.h/cc > > (if you want to leave this for a follow up CL I'm OK with that too) Created a TODO, will do in a follow up CL.
https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... File android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java (right): https://codereview.chromium.org/11416347/diff/1/android_webview/java/src/org/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:33: private static final Object lock = new Object(); On 2012/12/06 23:07:35, kristianm1 wrote: > On 2012/12/05 11:35:05, benm wrote: > > Can be a member? > It is static by design, this way if you have two instances they will be > synchronized (they would use the same prefs). I am not sure how likely this is > to cause problems, but thought it was best to do it correct. I think it would be incorrect to have two instances reading/writing the same keys. if we did support multiple instances, we should inject into each a unique key prefix to partition them. https://codereview.chromium.org/11416347/diff/5001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java (right): https://codereview.chromium.org/11416347/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:49: hmmm with the API here, there's no way to ever populate PREF_DENIED_ORIGINS? it's also slightly tricky as there's nothing stopping an origin appearing in both the allow and deny set. (in clear() below, you do correctly handle that 'impossible' state.). did you consider one prefs key per origin? (storing the state as a Boolean value). that makes the getOrigins() method a little more awkward. (but SharedPreferences.getAll does at least make it possible). https://codereview.chromium.org/11416347/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:88: public void getAllowed(final String userOrigin, final ValueCallback<Boolean> callback) { I think it would be more correct to do the lookup first, then just post the result to the UI thread. Otherwise someone who did a read followed by a write on one (non UI) thread may get the callback with the result of the write. String origin = getOrigin(userOrigin); Set allowedSet = mSharedPreferences.getStringSet(PREF_ALLOWED_ORIGINS, null); final boolean result = allowedSet != null ? allowedSet.contains(origin) : false; ThreadUtils.postOnUiThread(new FutureTask<Void>(new Runnable() { @Override public void run() { callback.onReceiveValue(result); } (ThreadUtils.postOnUiThread should have an overload that takes a Runnable and just posts that)
https://codereview.chromium.org/11416347/diff/5001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java (right): https://codereview.chromium.org/11416347/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:26: private static final String PREF_ALLOWED_ORIGINS = "geolocation.AllowedOrgins"; nit: Might be wise to use the package name to qualify these keys? e.g. "org.chromium.android_webview.GeolocationPermissions.AllowedOrigins"? https://codereview.chromium.org/11416347/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:30: private static final Object lock = new Object(); sLock https://codereview.chromium.org/11416347/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:36: public void allow(final String userOrigin) { nit: can remove final now? https://codereview.chromium.org/11416347/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:89: ThreadUtils.postOnUiThread(new FutureTask<Void>(new Runnable() { Why did you decide to callback on the UI thread? Is that what the legacy implementation did? Unless that's the case I think that intuitively it makes sense to callback on the thread making the request (you can still do that asynchronously by posting to Looper.myLooper()), hmm, actually that's only if the calling thread has started a Looper (which it probably didn't if it's just a short lived thread). Maybe best to keep the UI thread if the callback must be made asynchronously.
https://codereview.chromium.org/11416347/diff/5001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java (right): https://codereview.chromium.org/11416347/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:26: private static final String PREF_ALLOWED_ORIGINS = "geolocation.AllowedOrgins"; On 2012/12/07 14:07:56, benm wrote: > nit: Might be wise to use the package name to qualify these keys? > e.g. > "org.chromium.android_webview.GeolocationPermissions.AllowedOrigins"? Done. https://codereview.chromium.org/11416347/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:30: private static final Object lock = new Object(); On 2012/12/07 14:07:56, benm wrote: > sLock Done. https://codereview.chromium.org/11416347/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:36: public void allow(final String userOrigin) { On 2012/12/07 14:07:56, benm wrote: > nit: can remove final now? Done. https://codereview.chromium.org/11416347/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:49: On 2012/12/06 23:37:05, joth wrote: > hmmm with the API here, there's no way to ever populate PREF_DENIED_ORIGINS? > > > it's also slightly tricky as there's nothing stopping an origin appearing in > both the allow and deny set. (in clear() below, you do correctly handle that > 'impossible' state.). > > did you consider one prefs key per origin? (storing the state as a Boolean > value). that makes the getOrigins() method a little more awkward. (but > SharedPreferences.getAll does at least make it possible). > > I did think of this, but felt this was less clunky. I think allow should remove it from denied domains. The api is not wonderful, to find out if an origin is denied you have to check if it is in one of the lists with getOrigins(), and if it is, try if getAllowed() returns false. If the origin is not in any of the lists getAllowed() should return false. There is no way to set denied, other than loading a webpage from that origin, and replying permanent deny when asked. https://codereview.chromium.org/11416347/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:88: public void getAllowed(final String userOrigin, final ValueCallback<Boolean> callback) { On 2012/12/06 23:37:05, joth wrote: > I think it would be more correct to do the lookup first, then just post the > result to the UI thread. Otherwise someone who did a read followed by a write on > one (non UI) thread may get the callback with the result of the write. > > String origin = getOrigin(userOrigin); > Set allowedSet = mSharedPreferences.getStringSet(PREF_ALLOWED_ORIGINS, null); > final boolean result = allowedSet != null ? allowedSet.contains(origin) : false; > ThreadUtils.postOnUiThread(new FutureTask<Void>(new Runnable() { > @Override > public void run() { > callback.onReceiveValue(result); > } > > > (ThreadUtils.postOnUiThread should have an overload that takes a Runnable and > just posts that) Done, and added the overload. https://codereview.chromium.org/11416347/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:89: ThreadUtils.postOnUiThread(new FutureTask<Void>(new Runnable() { On 2012/12/07 14:07:56, benm wrote: > Why did you decide to callback on the UI thread? Is that what the legacy > implementation did? > > Unless that's the case I think that intuitively it makes sense to callback on > the thread making the request (you can still do that asynchronously by posting > to Looper.myLooper()), hmm, actually that's only if the calling thread has > started a Looper (which it probably didn't if it's just a short lived thread). > Maybe best to keep the UI thread if the callback must be made asynchronously. That is what the classic implementation does. Also as you mention the calling thread might not have a Looper.
https://codereview.chromium.org/11416347/diff/5001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java (right): https://codereview.chromium.org/11416347/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:49: On 2012/12/07 21:25:29, kristianm1 wrote: > On 2012/12/06 23:37:05, joth wrote: > > hmmm with the API here, there's no way to ever populate PREF_DENIED_ORIGINS? > > > > > > it's also slightly tricky as there's nothing stopping an origin appearing in > > both the allow and deny set. (in clear() below, you do correctly handle that > > 'impossible' state.). > > > > did you consider one prefs key per origin? (storing the state as a Boolean > > value). that makes the getOrigins() method a little more awkward. (but > > SharedPreferences.getAll does at least make it possible). > > > > > > I did think of this, but felt this was less clunky. I think allow should remove > it from denied domains. > Yeah that was my feeling too at first, but more I see the restrictions on using putStringSet, e.g. the way you need to repeatedly make deep copies of the full HashSet in order to make a single item mod to it, the more I think that just having one key per origin would make the code a lot simpler. > The api is not wonderful, to find out if an origin is denied you have to check > if it is in one of the lists with getOrigins(), and if it is, try if > getAllowed() returns false. If the origin is not in any of the lists > getAllowed() should return false. > > There is no way to set denied, other than loading a webpage from that origin, > and replying permanent deny when asked. Even if there's no public way to set denied() we'll still need the method on this implementation class in order to implement the permanent deny webpage flow. https://codereview.chromium.org/11416347/diff/11001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java (right): https://codereview.chromium.org/11416347/diff/11001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:27: "org.chromium.android_webview.GeolocationPermissions.AllowedOrgins"; nit: GeolocationPermissions.class.getCanonicalName() + ".AllowedOrigins"; https://codereview.chromium.org/11416347/diff/11001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:32: private static final Object sLock = new Object(); see my other comment: it would be incorrect usage to have two instances of this class working on the same shared pref keys, so no need for this to be static. (and generally, I'm trying to not have statics unless it's strictly needed) https://codereview.chromium.org/11416347/diff/11001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:52: editor.putStringSet(PREF_ALLOWED_ORIGINS, copiedOrigins); this should be PREF_DENIED_ORIGINS. An instrumentation test for this class wouldn't hurt. (I think there's a ram-only shared prefs subclass we can use to avoid it needing to hit disk). https://codereview.chromium.org/11416347/diff/11001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:55: } this function is starting to duplicate a lot of clear(). Also you're going to have to write a deny(origin) method, in order to implement the onGeolocationPermissionsShowPrompt flow, and deny() is also going to be very familiar. Can you factor out a single method: private void setOriginStateLocked(String origin, String key, boolean state) { assert Thread.holdsLock(mLock); Set<String> origins = mSharedPreferences.getStringSet(key, null); if (state && (origins == null || !origins.contains(origin)) { origins = origins == null ? new HashSet<String>() : new HashSet(origins); origins.add(origin); mSharedPreferences.edit().putStringSet(key, origins).apply(); } else if (!state && origins != null && origins.contains(origin)) { origins = new HashSet(origins); origins.remove(origin); mSharedPreferences.edit().putStringSet(key, origins).apply(); } } then we have public void allow(String origin) { origin = GetOrigin(origin); syncrhonized(lock) { SetOriginStateLocked(origin, PREF_ALLOWED_ORIGINS, true); SetOriginStateLocked(origin, PREF_DENIED_ORIGINS, false); } } public void deny(String origin) { origin = GetOrigin(origin); syncrhonized(lock) { SetOriginStateLocked(origin, PREF_ALLOWED_ORIGINS, false); SetOriginStateLocked(origin, PREF_DENIED_ORIGINS, true); } } public void clear(String origin) { origin = GetOrigin(origin); syncrhonized(lock) { SetOriginStateLocked(origin, PREF_ALLOWED_ORIGINS, false); SetOriginStateLocked(origin, PREF_DENIED_ORIGINS, false); } }
Fixed these, will look into using boolean prefs. https://codereview.chromium.org/11416347/diff/11001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java (right): https://codereview.chromium.org/11416347/diff/11001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:27: "org.chromium.android_webview.GeolocationPermissions.AllowedOrgins"; On 2012/12/07 23:47:33, joth wrote: > nit: GeolocationPermissions.class.getCanonicalName() + ".AllowedOrigins"; Done. https://codereview.chromium.org/11416347/diff/11001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:32: private static final Object sLock = new Object(); On 2012/12/07 23:47:33, joth wrote: > see my other comment: it would be incorrect usage to have two instances of this > class working on the same shared pref keys, so no need for this to be static. > (and generally, I'm trying to not have statics unless it's strictly needed) Done. https://codereview.chromium.org/11416347/diff/11001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:32: private static final Object sLock = new Object(); On 2012/12/07 23:47:33, joth wrote: > see my other comment: it would be incorrect usage to have two instances of this > class working on the same shared pref keys, so no need for this to be static. > (and generally, I'm trying to not have statics unless it's strictly needed) Done. https://codereview.chromium.org/11416347/diff/11001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:52: editor.putStringSet(PREF_ALLOWED_ORIGINS, copiedOrigins); On 2012/12/07 23:47:33, joth wrote: > this should be PREF_DENIED_ORIGINS. > > An instrumentation test for this class wouldn't hurt. (I think there's a > ram-only shared prefs subclass we can use to avoid it needing to hit disk). Will look into it. https://codereview.chromium.org/11416347/diff/11001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:55: } On 2012/12/07 23:47:33, joth wrote: > this function is starting to duplicate a lot of clear(). Also you're going to > have to write a deny(origin) method, in order to implement the > onGeolocationPermissionsShowPrompt flow, and deny() is also going to be very > familiar. > > Can you factor out a single method: > > private void setOriginStateLocked(String origin, String key, boolean state) { > assert Thread.holdsLock(mLock); > Set<String> origins = mSharedPreferences.getStringSet(key, null); > if (state && (origins == null || !origins.contains(origin)) { > origins = origins == null ? new HashSet<String>() : new HashSet(origins); > origins.add(origin); > mSharedPreferences.edit().putStringSet(key, origins).apply(); > } else if (!state && origins != null && origins.contains(origin)) { > origins = new HashSet(origins); > origins.remove(origin); > mSharedPreferences.edit().putStringSet(key, origins).apply(); > } > } > > > then we have > > public void allow(String origin) { > origin = GetOrigin(origin); > syncrhonized(lock) { > SetOriginStateLocked(origin, PREF_ALLOWED_ORIGINS, true); > SetOriginStateLocked(origin, PREF_DENIED_ORIGINS, false); > } > } > > public void deny(String origin) { > origin = GetOrigin(origin); > syncrhonized(lock) { > SetOriginStateLocked(origin, PREF_ALLOWED_ORIGINS, false); > SetOriginStateLocked(origin, PREF_DENIED_ORIGINS, true); > } > } > > > public void clear(String origin) { > origin = GetOrigin(origin); > syncrhonized(lock) { > SetOriginStateLocked(origin, PREF_ALLOWED_ORIGINS, false); > SetOriginStateLocked(origin, PREF_DENIED_ORIGINS, false); > } > } > > Done.
New version with boolean prefs, I think it is simpler. I think the lock can be removed now?
Ace! Only nits https://codereview.chromium.org/11416347/diff/15002/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java (right): https://codereview.chromium.org/11416347/diff/15002/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:15: import java.util.concurrent.FutureTask; don't think these are needed now? (but odd -- Runnable should be. We get that for free for some reason?) https://codereview.chromium.org/11416347/diff/15002/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:29: private static final Object mLock = new Object(); yep, agree mLock is spurious now. Only place it could help is if allow() called while another thread is doing a clearAll() or getOrigins(), but they're both inherently racy anyway so trying to push the behavior one way isn't going to help. https://codereview.chromium.org/11416347/diff/15002/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:59: if (name.indexOf(PREF_PREFIX) == 0) { nit: if (name.startsWith(PREF_PREFIX) https://codereview.chromium.org/11416347/diff/15002/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:81: if (name.indexOf(PREF_PREFIX) == 0) { ditto https://codereview.chromium.org/11416347/diff/15002/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:85: final Set<String> response = new HashSet<String>(origins); nit: no need to copy here. Just declare |origins| as final up top of method.
2 more minor comments. https://codereview.chromium.org/11416347/diff/10003/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java (right): https://codereview.chromium.org/11416347/diff/10003/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:26: private static final String PREF_PREFIX = GeolocationPermissions.class.getCanonicalName() + "."; I'd be tempted to use a DNS invalid char to force a hard separator. e.g. "%" https://codereview.chromium.org/11416347/diff/10003/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:84: private String getOrigin(String domain) { Oh possible improvement: rename to getOriginKey() and include the PREF_PREFIX on the result? Although, the ParseException case is interesting. Whatever we return there (null, empty or PREF_PREFIX) then invalid URLs will all end up sharing one entry in the pref table. PREF_PREFIX at least means ClearAll will find them. The 'correct' solution would be to move the catch up to the outer functions and have them deal with it correctly, but it's basically just mapping to a no-op, so returning null would let them do this just as easily.
Addressed all comments, PTAL. https://codereview.chromium.org/11416347/diff/15002/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java (right): https://codereview.chromium.org/11416347/diff/15002/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:15: import java.util.concurrent.FutureTask; On 2012/12/08 01:39:24, joth wrote: > don't think these are needed now? > > (but odd -- Runnable should be. We get that for free for some reason?) Right, will remove. Runnable is in java.lang so get that one for free like string. https://codereview.chromium.org/11416347/diff/15002/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:29: private static final Object mLock = new Object(); On 2012/12/08 01:39:24, joth wrote: > yep, agree mLock is spurious now. Only place it could help is if allow() called > while another thread is doing a clearAll() or getOrigins(), but they're both > inherently racy anyway so trying to push the behavior one way isn't going to > help. Already removed. Some of it is racy no matter what we do here as you noted. https://codereview.chromium.org/11416347/diff/15002/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:59: if (name.indexOf(PREF_PREFIX) == 0) { On 2012/12/08 01:39:24, joth wrote: > nit: if (name.startsWith(PREF_PREFIX) Done. https://codereview.chromium.org/11416347/diff/15002/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:81: if (name.indexOf(PREF_PREFIX) == 0) { On 2012/12/08 01:39:24, joth wrote: > ditto Done. https://codereview.chromium.org/11416347/diff/15002/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:85: final Set<String> response = new HashSet<String>(origins); On 2012/12/08 01:39:24, joth wrote: > nit: no need to copy here. Just declare |origins| as final up top of method. Done. https://codereview.chromium.org/11416347/diff/10003/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java (right): https://codereview.chromium.org/11416347/diff/10003/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:26: private static final String PREF_PREFIX = GeolocationPermissions.class.getCanonicalName() + "."; On 2012/12/08 01:51:33, joth wrote: > I'd be tempted to use a DNS invalid char to force a hard separator. e.g. "%" Since it is the start of the string, I don't think it matters, but shouldn't harm. Done. https://codereview.chromium.org/11416347/diff/10003/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/GeolocationPermissions.java:84: private String getOrigin(String domain) { On 2012/12/08 01:51:33, joth wrote: > Oh possible improvement: rename to getOriginKey() and include the PREF_PREFIX on > the result? > > Although, the ParseException case is interesting. Whatever we return there > (null, empty or PREF_PREFIX) then invalid URLs will all end up sharing one entry > in the pref table. PREF_PREFIX at least means ClearAll will find them. > The 'correct' solution would be to move the catch up to the outer functions and > have them deal with it correctly, but it's basically just mapping to a no-op, so > returning null would let them do this just as easily. > Thanks! Had forgotten about this, I guess returning null and handling that is the simplest.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kristianm@chromium.org/11416347/22001
On 2012/12/11 18:35:15, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/kristianm%40chromium.org/11416347/22001 WebAddress is hidden, will implement the Gurl approach right away.
Sorry for I got bad news for ya. Compile failed with a clobber build on android_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
Getting the origin from GURL, make a new general java class in net. Please take a look, if it looks OK I will add a net/ owner.
https://codereview.chromium.org/11416347/diff/34005/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java (right): https://codereview.chromium.org/11416347/diff/34005/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:95: if (origin.isEmpty()) { origin will be null for invalid input. ? https://codereview.chromium.org/11416347/diff/34005/net/android/java/src/org/... File net/android/java/src/org/chromium/net/GurlUtils.java (right): https://codereview.chromium.org/11416347/diff/34005/net/android/java/src/org/... net/android/java/src/org/chromium/net/GurlUtils.java:12: * Callbacks are posted on the UI thread. wrong comment https://codereview.chromium.org/11416347/diff/34005/net/android/java/src/org/... net/android/java/src/org/chromium/net/GurlUtils.java:15: public final class GurlUtils { ya I think this is the correct capitalization by our normal uses.. but both java uses URL and google & chrome have GURL so I think GURLUtils would be the consistent name. http://developer.android.com/reference/java/net/URL.html etc https://codereview.chromium.org/11416347/diff/34005/net/android/java/src/org/... net/android/java/src/org/chromium/net/GurlUtils.java:19: * would return "http://www.example.com:8080". It will assert in native code and return null for nit: I don't think it will assert - just give you the empty string as you say.
https://codereview.chromium.org/11416347/diff/34005/net/android/java/src/org/... File net/android/java/src/org/chromium/net/GurlUtils.java (right): https://codereview.chromium.org/11416347/diff/34005/net/android/java/src/org/... net/android/java/src/org/chromium/net/GurlUtils.java:26: if (origin.isEmpty()) { use this for concise NPE safe checking: if ("".equals(origin))
https://codereview.chromium.org/11416347/diff/34005/net/android/java/src/org/... File net/android/java/src/org/chromium/net/GurlUtils.java (right): https://codereview.chromium.org/11416347/diff/34005/net/android/java/src/org/... net/android/java/src/org/chromium/net/GurlUtils.java:26: if (origin.isEmpty()) { On 2012/12/13 18:30:54, joth wrote: > use this for concise NPE safe checking: > if ("".equals(origin)) opps - I meant that comment for getOriginKey not here :-) although... you could move this empty -> null conversation to the .cc side. Or else just remove it altogether to make the caller not have to deal with NPE!
https://codereview.chromium.org/11416347/diff/34005/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java (right): https://codereview.chromium.org/11416347/diff/34005/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:95: if (origin.isEmpty()) { On 2012/12/13 04:59:54, joth wrote: > origin will be null for invalid input. ? Changing to empty string in gurlutils https://codereview.chromium.org/11416347/diff/34005/net/android/java/src/org/... File net/android/java/src/org/chromium/net/GurlUtils.java (right): https://codereview.chromium.org/11416347/diff/34005/net/android/java/src/org/... net/android/java/src/org/chromium/net/GurlUtils.java:12: * Callbacks are posted on the UI thread. On 2012/12/13 04:59:54, joth wrote: > wrong comment Done. https://codereview.chromium.org/11416347/diff/34005/net/android/java/src/org/... net/android/java/src/org/chromium/net/GurlUtils.java:15: public final class GurlUtils { On 2012/12/13 04:59:54, joth wrote: > ya I think this is the correct capitalization by our normal uses.. but both java > uses URL and google & chrome have GURL so I think GURLUtils would be the > consistent name. > http://developer.android.com/reference/java/net/URL.html etc Done. https://codereview.chromium.org/11416347/diff/34005/net/android/java/src/org/... net/android/java/src/org/chromium/net/GurlUtils.java:19: * would return "http://www.example.com:8080". It will assert in native code and return null for On 2012/12/13 04:59:54, joth wrote: > nit: I don't think it will assert - just give you the empty string as you say. From gurl.h, documenting spec(): // Returns the raw spec, i.e., the full text of the URL, in canonical UTF-8, // if the URL is valid. If the URL is not valid, this will assert and return // the empty string (for safety in release builds, to keep them from being // misused which might be a security problem). https://codereview.chromium.org/11416347/diff/34005/net/android/java/src/org/... net/android/java/src/org/chromium/net/GurlUtils.java:26: if (origin.isEmpty()) { On 2012/12/13 18:33:37, joth wrote: > On 2012/12/13 18:30:54, joth wrote: > > use this for concise NPE safe checking: > > if ("".equals(origin)) > > opps - I meant that comment for getOriginKey not here :-) > > although... you could move this empty -> null conversation to the .cc side. Or > else just remove it altogether to make the caller not have to deal with NPE! Removed the check.
lgtm modulo this function comment. https://codereview.chromium.org/11416347/diff/34005/net/android/java/src/org/... File net/android/java/src/org/chromium/net/GurlUtils.java (right): https://codereview.chromium.org/11416347/diff/34005/net/android/java/src/org/... net/android/java/src/org/chromium/net/GurlUtils.java:19: * would return "http://www.example.com:8080". It will assert in native code and return null for On 2012/12/13 20:41:27, kristianm1 wrote: > On 2012/12/13 04:59:54, joth wrote: > > nit: I don't think it will assert - just give you the empty string as you say. > > From gurl.h, documenting spec(): > // Returns the raw spec, i.e., the full text of the URL, in canonical UTF-8, > // if the URL is valid. If the URL is not valid, this will assert and return > // the empty string (for safety in release builds, to keep them from being > // misused which might be a security problem). But you call spec() on the result of a GetOrigin() call. GetOrigin() promises to return either a valid URL or empty, and neither of those would cause spec() to assert. ergo, an assert should be impossible from a call to this function. If you're not sure the answer is probably to add net/android/javatests/src/org/chromium/net/GURLUtilsTest.java to clarify it :-) We will rely on this never asserting, to handle any legacy app that does pass us garbage.
On 2012/12/13 21:50:19, joth wrote: > lgtm modulo this function comment. > > https://codereview.chromium.org/11416347/diff/34005/net/android/java/src/org/... > File net/android/java/src/org/chromium/net/GurlUtils.java (right): > > https://codereview.chromium.org/11416347/diff/34005/net/android/java/src/org/... > net/android/java/src/org/chromium/net/GurlUtils.java:19: * would return > "http://www.example.com:8080". It will assert in native code and return null for > On 2012/12/13 20:41:27, kristianm1 wrote: > > On 2012/12/13 04:59:54, joth wrote: > > > nit: I don't think it will assert - just give you the empty string as you > say. > > > > From gurl.h, documenting spec(): > > // Returns the raw spec, i.e., the full text of the URL, in canonical UTF-8, > > // if the URL is valid. If the URL is not valid, this will assert and return > > // the empty string (for safety in release builds, to keep them from being > > // misused which might be a security problem). > > But you call spec() on the result of a GetOrigin() call. GetOrigin() promises to > return either a valid URL or empty, and neither of those would cause spec() to > assert. ergo, an assert should be impossible from a call to this function. If > you're not sure the answer is probably to add > net/android/javatests/src/org/chromium/net/GURLUtilsTest.java to clarify it :-) > > We will rely on this never asserting, to handle any legacy app that does pass us > garbage. Removed.
lgtm. looks like yfriedman is the only west-coast net/android reviewer.
Added Yaron and Will from the owners files since they are west coast based.
Sorry, I'm going to defer to Yaron. I don't know this well enough :) https://codereview.chromium.org/11416347/diff/44009/net/android/gurl_utils.h File net/android/gurl_utils.h (left): https://codereview.chromium.org/11416347/diff/44009/net/android/gurl_utils.h#... net/android/gurl_utils.h:5: #include "net/quic/crypto/crypto_protocol.h" Hehe, I don't think branching is really what you want here :) Unless you want their rev history shared. https://codereview.chromium.org/11416347/diff/44009/net/android/gurl_utils.h File net/android/gurl_utils.h (right): https://codereview.chromium.org/11416347/diff/44009/net/android/gurl_utils.h#... net/android/gurl_utils.h:8: #include <jni.h> Can we just forward declare JNIEnv?
On 13 December 2012 14:44, <willchan@chromium.org> wrote: > Sorry, I'm going to defer to Yaron. I don't know this well enough :) > > I think kristian just meant you to review net/net.gyp<https://codereview.chromium.org/11416347/patch/44009/41012> :-) > > https://codereview.chromium.**org/11416347/diff/44009/net/** > android/gurl_utils.h<https://codereview.chromium.org/11416347/diff/44009/net/android/gurl_utils.h> > File net/android/gurl_utils.h (left): > > https://codereview.chromium.**org/11416347/diff/44009/net/** > android/gurl_utils.h#oldcode5<https://codereview.chromium.org/11416347/diff/44009/net/android/gurl_utils.h#oldcode5> > net/android/gurl_utils.h:5: #include "net/quic/crypto/crypto_**protocol.h" > Hehe, I don't think branching is really what you want here :) Unless you > want their rev history shared. > > https://codereview.chromium.**org/11416347/diff/44009/net/** > android/gurl_utils.h<https://codereview.chromium.org/11416347/diff/44009/net/android/gurl_utils.h> > File net/android/gurl_utils.h (right): > > https://codereview.chromium.**org/11416347/diff/44009/net/** > android/gurl_utils.h#newcode8<https://codereview.chromium.org/11416347/diff/44009/net/android/gurl_utils.h#newcode8> > net/android/gurl_utils.h:8: #include <jni.h> > Can we just forward declare JNIEnv? > > https://codereview.chromium.**org/11416347/<https://codereview.chromium.org/1... >
Thanks for clarifying. net/net.gyp lgtm.
https://codereview.chromium.org/11416347/diff/44009/net/android/gurl_utils.h File net/android/gurl_utils.h (left): https://codereview.chromium.org/11416347/diff/44009/net/android/gurl_utils.h#... net/android/gurl_utils.h:5: #include "net/quic/crypto/crypto_protocol.h" On 2012/12/13 22:44:29, willchan wrote: > Hehe, I don't think branching is really what you want here :) Unless you want > their rev history shared. I was wondering about this as well, I think it is just git trying to be clever. Not sure what I can do about it. https://codereview.chromium.org/11416347/diff/44009/net/android/gurl_utils.h File net/android/gurl_utils.h (right): https://codereview.chromium.org/11416347/diff/44009/net/android/gurl_utils.h#... net/android/gurl_utils.h:8: #include <jni.h> On 2012/12/13 22:44:29, willchan wrote: > Can we just forward declare JNIEnv? It doesn't actually need to be (read can't I think) forward declared since it is magic included. This makes this do nothing since it is always included. Do you think it is better without this include?
https://codereview.chromium.org/11416347/diff/44009/net/android/gurl_utils.h File net/android/gurl_utils.h (right): https://codereview.chromium.org/11416347/diff/44009/net/android/gurl_utils.h#... net/android/gurl_utils.h:8: #include <jni.h> On 2012/12/13 23:32:32, kristianm1 wrote: > On 2012/12/13 22:44:29, willchan wrote: > > Can we just forward declare JNIEnv? > > It doesn't actually need to be (read can't I think) forward declared since it is > magic included. This makes this do nothing since it is always included. Do you > think it is better without this include? Magic included? I don't know what that means. Do you mean the build process is doing it already? Feel free to ignore my comment if this is a special Android build thing. My comment is just from the style guide which encourages reducing header file dependencies via forward declarations.
On 2012/12/13 23:41:09, willchan wrote: > https://codereview.chromium.org/11416347/diff/44009/net/android/gurl_utils.h > File net/android/gurl_utils.h (right): > > https://codereview.chromium.org/11416347/diff/44009/net/android/gurl_utils.h#... > net/android/gurl_utils.h:8: #include <jni.h> > On 2012/12/13 23:32:32, kristianm1 wrote: > > On 2012/12/13 22:44:29, willchan wrote: > > > Can we just forward declare JNIEnv? > > > > It doesn't actually need to be (read can't I think) forward declared since it > is > > magic included. This makes this do nothing since it is always included. Do you > > think it is better without this include? > > Magic included? I don't know what that means. Do you mean the build process is > doing it already? > > Feel free to ignore my comment if this is a special Android build thing. My > comment is just from the style guide which encourages reducing header file > dependencies via forward declarations. Yes, it must be included by the build system. I try to forward declare and that created a warning. Just deleting the include builds fine.
On 13 December 2012 15:43, <kristianm@chromium.org> wrote: > On 2012/12/13 23:41:09, willchan wrote: > >> https://codereview.chromium.**org/11416347/diff/44009/net/** >> android/gurl_utils.h<https://codereview.chromium.org/11416347/diff/44009/net/android/gurl_utils.h> >> File net/android/gurl_utils.h (right): >> > > > https://codereview.chromium.**org/11416347/diff/44009/net/** > android/gurl_utils.h#newcode8<https://codereview.chromium.org/11416347/diff/44009/net/android/gurl_utils.h#newcode8> > >> net/android/gurl_utils.h:8: #include <jni.h> >> On 2012/12/13 23:32:32, kristianm1 wrote: >> > On 2012/12/13 22:44:29, willchan wrote: >> > > Can we just forward declare JNIEnv? >> > >> > It doesn't actually need to be (read can't I think) forward declared >> since >> > it > >> is >> > magic included. This makes this do nothing since it is always included. >> Do >> > you > >> > think it is better without this include? >> > > Magic included? I don't know what that means. Do you mean the build >> process is >> doing it already? >> > > Feel free to ignore my comment if this is a special Android build thing. >> My >> comment is just from the style guide which encourages reducing header file >> dependencies via forward declarations. >> > > Yes, it must be included by the build system. I try to forward declare and > that > created a warning. Just deleting the include builds fine. > > Huh. I didn't think we used for includes on android for this. Pretty sure you need the include there (we have many similar cases for this) Forward declaring JNIEnv is complicated as it's a typedef for an implementation specific internal struct. In those cases, the IWYU rules say to actually include the file, I think. > https://codereview.chromium.**org/11416347/<https://codereview.chromium.org/1... >
https://codereview.chromium.org/11416347/diff/44009/net/android/gurl_utils.cc File net/android/gurl_utils.cc (right): https://codereview.chromium.org/11416347/diff/44009/net/android/gurl_utils.cc... net/android/gurl_utils.cc:17: host.GetOrigin().spec()).Release(); Hmm. You shouldn't need to manager this yourself. AFAIK, the function return type should be definable as ScopedJavaLocalRef<string> and let the bindings do it for you.
On 2012/12/14 02:09:07, Yaron wrote: > https://codereview.chromium.org/11416347/diff/44009/net/android/gurl_utils.cc > File net/android/gurl_utils.cc (right): > > https://codereview.chromium.org/11416347/diff/44009/net/android/gurl_utils.cc... > net/android/gurl_utils.cc:17: host.GetOrigin().spec()).Release(); > Hmm. You shouldn't need to manager this yourself. AFAIK, the function return > type should be definable as ScopedJavaLocalRef<string> and let the bindings do > it for you. lgtm given offline discussions. We can deal with the |Release| call once we have a general solution
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kristianm@chromium.org/11416347/44009
Retried try job too often on win_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kristianm@chromium.org/11416347/44009
Retried try job too often on win_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kristianm@chromium.org/11416347/44009
Message was sent while issue was closed.
Change committed as 173328 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
