Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12211047:
Implementing geolocation for the Android Webview (Closed)
|
Created:
7 years, 10 months ago by Kristian Monsen Modified:
7 years, 10 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplementing geolocation for the Android Webview
Enabling geolocation callbacks
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182465
Patch Set 1 #Patch Set 2 : Minor style updates #
Total comments: 53
Patch Set 3 : Addressed issues from reviews #
Total comments: 16
Patch Set 4 : More fixes from comments #Patch Set 5 : Updated indenting #
Total comments: 18
Patch Set 6 : Invoke propmt in a task, misc review feedback #
Total comments: 15
Patch Set 7 : Removed On prefix and check for weak ref null #Patch Set 8 : store a local ref during method call #
Total comments: 2
Patch Set 9 : Store a local ref when calling hide #
Total comments: 4
Patch Set 10 : removed catching ClassCastException #Patch Set 11 : Added const #Messages
Total messages: 22 (0 generated)
Martin, I plan to create a native AwContentsClient and move the code from aw_contents.cc there after this if you think that would be a good idea.
https://codereview.chromium.org/12211047/diff/2001/android_webview/browser/aw... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/12211047/diff/2001/android_webview/browser/aw... android_webview/browser/aw_content_browser_client.cc:26: class DummyAccessTokenStore : public content::AccessTokenStore { Rename this class now? https://codereview.chromium.org/12211047/diff/2001/android_webview/browser/aw... android_webview/browser/aw_content_browser_client.cc:38: virtual void SaveAccessToken( Really weird two overriden methods from same class belong in different visibility blocks. Can you move this to public and Add a comment saying these implementing content::AccessTokenStore https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:865: private class ChromiumGeolocationCallback implements GeolocationPermissions.Callback { Might be better to call this AwGeolocationCallback https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:904: origin, new ChromiumGeolocationCallback(permissions)); Not sure if this is a big concern, but can we just have one instance variable of this created lazily here to avoid creating and gc-ing every single time? https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java (right): https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:29: // TODO(kristianm): Make this private once framework is updated to use getInstance 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? https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:38: public static synchronized AwGeolocationPermissions getInstance() { Can we use synchronized blocks instead of having the whole function be synchornized? Just not wanting to commit the api to it. Same below https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:48: public static synchronized AwGeolocationPermissions createInstance( getInstance vs createInstance? I'm confused if this is a singleton or not https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:53: sInstance = new AwGeolocationPermissions(sharedPreferences); sInstance is set here and in the constructor, duplicated? https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:141: final boolean finalAllowed = getAllowed(origin); given this is async, should this be more async that getAllowed be called in the Runnable below? Similar below. https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... android_webview/native/aw_contents.cc:218: geolocation_callbacks_.clear(); is this necessary? https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... android_webview/native/aw_contents.cc:726: } // namespace https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... android_webview/native/aw_contents.cc:732: geolocation_callbacks_.push_back(OriginCallback(origin, callback)); 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? https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... android_webview/native/aw_contents.cc:761: if (removed_current_outstanding_callback) { Can removed_current_outstanding_callback ever be false here? If not, then might be better to use a DCHECK. https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... android_webview/native/aw_contents.cc:766: java_ref_, geolocation_callbacks_.front().first); check front() is not null? https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... File android_webview/native/aw_contents.h (right): https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... android_webview/native/aw_contents.h:189: std::list<OriginCallback> geolocation_callbacks_; man this just begs for a native->java callback translation as Martin mentioned in another patch. I guess using list is a conscious decision since you don't expect more than a few of these? sg https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... File android_webview/native/aw_geolocation_permission_context.cc (right): https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... android_webview/native/aw_geolocation_permission_context.cc:28: const content::RenderViewHost* rvh = Is this id->AwContents code duplicated with Selim's patch? I think I suggested to him to move it into AwContents. (I'm not sure if he's landed yet) https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... android_webview/native/aw_geolocation_permission_context.cc:85: const content::RenderViewHost* rvh = Certainly don't duplicate this logic twice in the same file.
https://codereview.chromium.org/12211047/diff/2001/android_webview/browser/aw... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/12211047/diff/2001/android_webview/browser/aw... android_webview/browser/aw_content_browser_client.cc:32: request.Run(access_token_set_, NULL); comment that access tokens not used on android, but we need to complete the request to startup the geolocation subsystem? https://codereview.chromium.org/12211047/diff/2001/android_webview/browser/aw... android_webview/browser/aw_content_browser_client.cc:41: AccessTokenStore::AccessTokenSet access_token_set_; I think you can make this a local in the method above rather than a member, as request.Run() will take a deep copy anyway. https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java (right): https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:27: private static AwGeolocationPermissions sInstance; 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. https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:108: public boolean getAllowed(String origin) { I think isOriginAllowed / isOriginDenied would be better for both these. https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:118: return allowed; 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; } https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:129: allowed = mSharedPreferences.getBoolean(key, true); 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(..); https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... android_webview/native/aw_contents.cc:732: geolocation_callbacks_.push_back(OriginCallback(origin, callback)); On 2013/02/06 20:29:54, boliu wrote: > 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? onGeolocationPermissionsShowPrompt on java side already does this. (called from GeolocationShowPromp for the item at front of the queue) https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... android_webview/native/aw_contents.cc:756: if (it == geolocation_callbacks_.begin()) { '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. https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... File android_webview/native/aw_geolocation_permission_context.cc (right): https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... android_webview/native/aw_geolocation_permission_context.cc:46: } yeah, this will all be cleaner with a method factored out (locally or otherwise). AwContents* aw_contetnts == FoBar..GetAwContentsFromID(process_id, view_id); if (!aw_contetnts) { callback.Run(false); return; }
https://codereview.chromium.org/12211047/diff/5002/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java (right): https://codereview.chromium.org/12211047/diff/5002/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:83: * Clear one origin from being allowed and denied. nit: Does "Clear the stored permission for a particular origin." read better? https://codereview.chromium.org/12211047/diff/5002/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:85: public void clear(String origin) { Should we document the format for origin anywhere? https://codereview.chromium.org/12211047/diff/5002/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:93: * Clear all origins set to allowed or denied. nit: Does "Clear stored permissions for all origins." read better? https://codereview.chromium.org/12211047/diff/5002/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:116: } catch (ClassCastException e) { Do you need to catch this? https://codereview.chromium.org/12211047/diff/5002/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwSettings.java (right): https://codereview.chromium.org/12211047/diff/5002/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwSettings.java:147: mGeolocationEnabled = flag; nit: the pattern for other setters is to only update the member if there is a change in the value. https://codereview.chromium.org/12211047/diff/5002/android_webview/native/aw_... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12211047/diff/5002/android_webview/native/aw_... android_webview/native/aw_contents.cc:718: JavaObjectWeakGlobalRef java_ref, const GURL& origin) { nit: one param per line for function definitions/declarations here and throughout https://codereview.chromium.org/12211047/diff/5002/android_webview/native/aw_... File android_webview/native/aw_contents.h (right): https://codereview.chromium.org/12211047/diff/5002/android_webview/native/aw_... android_webview/native/aw_contents.h:136: JNIEnv* env, jobject obj, jboolean value, jstring origin); nit: one param per line. https://codereview.chromium.org/12211047/diff/5002/android_webview/native/aw_... File android_webview/native/aw_geolocation_permission_context.cc (right): https://codereview.chromium.org/12211047/diff/5002/android_webview/native/aw_... android_webview/native/aw_geolocation_permission_context.cc:21: // TODO(kristianm): Removed this when https://codereview.chromium.org/12207061/ guess you can do this now :)
Should have addressed all comments from reviews. PTAL. https://codereview.chromium.org/12211047/diff/2001/android_webview/browser/aw... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/12211047/diff/2001/android_webview/browser/aw... android_webview/browser/aw_content_browser_client.cc:26: class DummyAccessTokenStore : public content::AccessTokenStore { On 2013/02/06 20:29:54, boliu wrote: > Rename this class now? Renamed it to AwAccessTokenStore, but really not much more functionality now than before. https://codereview.chromium.org/12211047/diff/2001/android_webview/browser/aw... android_webview/browser/aw_content_browser_client.cc:32: request.Run(access_token_set_, NULL); On 2013/02/06 22:09:51, joth wrote: > comment that access tokens not used on android, but we need to complete the > request to startup the geolocation subsystem? Done. https://codereview.chromium.org/12211047/diff/2001/android_webview/browser/aw... android_webview/browser/aw_content_browser_client.cc:38: virtual void SaveAccessToken( On 2013/02/06 20:29:54, boliu wrote: > Really weird two overriden methods from same class belong in different > visibility blocks. Can you move this to public and Add a comment saying these > implementing content::AccessTokenStore Done. https://codereview.chromium.org/12211047/diff/2001/android_webview/browser/aw... android_webview/browser/aw_content_browser_client.cc:41: AccessTokenStore::AccessTokenSet access_token_set_; On 2013/02/06 22:09:51, joth wrote: > I think you can make this a local in the method above rather than a member, as > request.Run() will take a deep copy anyway. Done. https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:865: private class ChromiumGeolocationCallback implements GeolocationPermissions.Callback { On 2013/02/06 20:29:54, boliu wrote: > Might be better to call this AwGeolocationCallback Done. https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:904: origin, new ChromiumGeolocationCallback(permissions)); On 2013/02/06 20:29:54, boliu wrote: > Not sure if this is a big concern, but can we just have one instance variable of > this created lazily here to avoid creating and gc-ing every single time? There can be many outstanding at a time if they get cancelled and never replied to. https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java (right): https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:27: private static AwGeolocationPermissions sInstance; On 2013/02/06 22:09:51, joth wrote: > 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. OK. https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:29: // TODO(kristianm): Make this private once framework is updated to use getInstance On 2013/02/06 20:29:54, boliu wrote: > 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? Android -> framework/webview. That is where this is created. I need to be able to get to the instance from AwContents.java. https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:38: public static synchronized AwGeolocationPermissions getInstance() { On 2013/02/06 20:29:54, boliu wrote: > Can we use synchronized blocks instead of having the whole function be > synchornized? Just not wanting to commit the api to it. Same below Done. https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:48: public static synchronized AwGeolocationPermissions createInstance( On 2013/02/06 20:29:54, boliu wrote: > getInstance vs createInstance? I'm confused if this is a singleton or not 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) https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:53: sInstance = new AwGeolocationPermissions(sharedPreferences); On 2013/02/06 20:29:54, boliu wrote: > sInstance is set here and in the constructor, duplicated? 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. https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:108: public boolean getAllowed(String origin) { On 2013/02/06 22:09:51, joth wrote: > I think isOriginAllowed / isOriginDenied would be better for both these. Done. https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:118: return allowed; On 2013/02/06 22:09:51, joth wrote: > 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; > } > Done. https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:129: allowed = mSharedPreferences.getBoolean(key, true); On 2013/02/06 22:09:51, joth wrote: > 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(..); > 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. https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:141: final boolean finalAllowed = getAllowed(origin); On 2013/02/06 20:29:54, boliu wrote: > given this is async, should this be more async that getAllowed be called in the > Runnable below? Similar below. 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. https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... android_webview/native/aw_contents.cc:218: geolocation_callbacks_.clear(); On 2013/02/06 20:29:54, boliu wrote: > is this necessary? When thinking about it, no. It happens automatically when the list goes out of scope. https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... android_webview/native/aw_contents.cc:726: } On 2013/02/06 20:29:54, boliu wrote: > // namespace Done. https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... android_webview/native/aw_contents.cc:756: if (it == geolocation_callbacks_.begin()) { On 2013/02/06 22:09:51, joth wrote: > '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. 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. https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... android_webview/native/aw_contents.cc:761: if (removed_current_outstanding_callback) { On 2013/02/06 20:29:54, boliu wrote: > Can removed_current_outstanding_callback ever be false here? If not, then might > be better to use a DCHECK. Yes, if it was cancelled before the callback came back here. https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... android_webview/native/aw_contents.cc:766: java_ref_, geolocation_callbacks_.front().first); On 2013/02/06 20:29:54, boliu wrote: > check front() is not null? Good catch! Adding an empty list check. https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... File android_webview/native/aw_contents.h (right): https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... android_webview/native/aw_contents.h:189: std::list<OriginCallback> geolocation_callbacks_; On 2013/02/06 20:29:54, boliu wrote: > man this just begs for a native->java callback translation as Martin mentioned > in another patch. > > I guess using list is a conscious decision since you don't expect more than a > few of these? sg The problem here is that these have to be stored native (I think), as the callback can contain refcounted instances. Used list since I expect there to be few, and the front to be removed. This is the most expensive operation on a vector. https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... File android_webview/native/aw_geolocation_permission_context.cc (right): https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... android_webview/native/aw_geolocation_permission_context.cc:28: const content::RenderViewHost* rvh = On 2013/02/06 20:29:54, boliu wrote: > Is this id->AwContents code duplicated with Selim's patch? I think I suggested > to him to move it into AwContents. (I'm not sure if he's landed yet) Coordinating with Selim, will use that before landing this patch. https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... android_webview/native/aw_geolocation_permission_context.cc:46: } On 2013/02/06 22:09:51, joth wrote: > yeah, this will all be cleaner with a method factored out (locally or > otherwise). > > AwContents* aw_contetnts == FoBar..GetAwContentsFromID(process_id, view_id); > if (!aw_contetnts) { > callback.Run(false); > return; > } Done. https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... android_webview/native/aw_geolocation_permission_context.cc:85: const content::RenderViewHost* rvh = On 2013/02/06 20:29:54, boliu wrote: > Certainly don't duplicate this logic twice in the same file. Done. https://codereview.chromium.org/12211047/diff/5002/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java (right): https://codereview.chromium.org/12211047/diff/5002/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:83: * Clear one origin from being allowed and denied. On 2013/02/07 17:22:39, benm wrote: > nit: Does "Clear the stored permission for a particular origin." read better? Done. https://codereview.chromium.org/12211047/diff/5002/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:85: public void clear(String origin) { On 2013/02/07 17:22:39, benm wrote: > Should we document the format for origin anywhere? I don't think that is needed. It is a regular concept for url's, and if anything else than a origin is passed in we just get the origin part of it. https://codereview.chromium.org/12211047/diff/5002/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:93: * Clear all origins set to allowed or denied. On 2013/02/07 17:22:39, benm wrote: > nit: Does "Clear stored permissions for all origins." read better? Done. https://codereview.chromium.org/12211047/diff/5002/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:116: } catch (ClassCastException e) { On 2013/02/07 17:22:39, benm wrote: > Do you need to catch this? I think it is best. If somehow there got to be another pref with the same name it would raise an exception all the way out. Don't think we want that on release builds. This is not optimal either so I could be convinced to remove. Might make sense to remove the bad pref as well. https://codereview.chromium.org/12211047/diff/5002/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwSettings.java (right): https://codereview.chromium.org/12211047/diff/5002/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwSettings.java:147: mGeolocationEnabled = flag; On 2013/02/07 17:22:39, benm wrote: > nit: the pattern for other setters is to only update the member if there is a > change in the value. Is there any reasoning behind this? Updated, but would rather remove all these checks. https://codereview.chromium.org/12211047/diff/5002/android_webview/native/aw_... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12211047/diff/5002/android_webview/native/aw_... android_webview/native/aw_contents.cc:718: JavaObjectWeakGlobalRef java_ref, const GURL& origin) { On 2013/02/07 17:22:39, benm wrote: > nit: one param per line for function definitions/declarations here and > throughout Done. https://codereview.chromium.org/12211047/diff/5002/android_webview/native/aw_... File android_webview/native/aw_contents.h (right): https://codereview.chromium.org/12211047/diff/5002/android_webview/native/aw_... android_webview/native/aw_contents.h:136: JNIEnv* env, jobject obj, jboolean value, jstring origin); On 2013/02/07 17:22:39, benm wrote: > nit: one param per line. I thought I was using the chromium style: For function declarations and definitions, put each argument on a separate line unless the whole declaration fits on one line. I see that it say whole definition and not all parameters so updated. https://codereview.chromium.org/12211047/diff/5002/android_webview/native/aw_... File android_webview/native/aw_geolocation_permission_context.cc (right): https://codereview.chromium.org/12211047/diff/5002/android_webview/native/aw_... android_webview/native/aw_geolocation_permission_context.cc:21: // TODO(kristianm): Removed this when https://codereview.chromium.org/12207061/ On 2013/02/07 17:22:39, benm wrote: > guess you can do this now :) Done. I didn't send out a review mail for this patch since I knew this one was landing.
https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java (right): https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:129: allowed = mSharedPreferences.getBoolean(key, true); On 2013/02/08 00:07:44, Kristian Monsen wrote: > On 2013/02/06 22:09:51, joth wrote: > > 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(..); > > > > 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. 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(). https://codereview.chromium.org/12211047/diff/2001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:141: final boolean finalAllowed = getAllowed(origin); On 2013/02/08 00:07:44, Kristian Monsen wrote: > On 2013/02/06 20:29:54, boliu wrote: > > given this is async, should this be more async that getAllowed be called in > the > > Runnable below? Similar below. > 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. 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. https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12211047/diff/2001/android_webview/native/aw_... android_webview/native/aw_contents.cc:756: if (it == geolocation_callbacks_.begin()) { On 2013/02/08 00:07:44, Kristian Monsen wrote: > On 2013/02/06 22:09:51, joth wrote: > > '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. > > 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. 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. https://codereview.chromium.org/12211047/diff/3011/android_webview/native/aw_... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12211047/diff/3011/android_webview/native/aw_... android_webview/native/aw_contents.cc:736: ConvertUTF8ToJavaString(env,origin.spec())); nit: space after comma https://codereview.chromium.org/12211047/diff/3011/android_webview/native/aw_... android_webview/native/aw_contents.cc:763: GeolocationShowPrompt(java_ref_, geolocation_callbacks_.front().first); 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. https://codereview.chromium.org/12211047/diff/3011/android_webview/native/aw_... android_webview/native/aw_contents.cc:771: for ( ; it != geolocation_callbacks_.end(); ) { This construct has it's very own special syntax: while (it != geolocation_callbacks_.end()) { :-) https://codereview.chromium.org/12211047/diff/3011/android_webview/native/aw_... android_webview/native/aw_contents.cc:774: if (it == geolocation_callbacks_.begin()) { 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; } } https://codereview.chromium.org/12211047/diff/3011/android_webview/native/aw_... File android_webview/native/aw_contents.h (right): https://codereview.chromium.org/12211047/diff/3011/android_webview/native/aw_... android_webview/native/aw_contents.h:194: typedef std::pair<const GURL&, base::Callback<void(bool)> > OriginCallback; add a comment that the Callback<void(bool)> is the content-layer supplied function to run invoked passing the result of the application geolocation permission prompt. (Callback in the typename and member is pretty vague. Trying to think of a suggestion to improve it. pending_geolocation_prompts_ ? also comment that the first item in the list is always the currently pending request (i.e. request we sent to the application for which we're still awaiting a result).
https://codereview.chromium.org/12211047/diff/3011/android_webview/browser/aw... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/12211047/diff/3011/android_webview/browser/aw... android_webview/browser/aw_content_browser_client.cc:291: // TODO(boliu): Implement as part of geolocation code. nit: remove this todo now? https://codereview.chromium.org/12211047/diff/3011/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java (right): https://codereview.chromium.org/12211047/diff/3011/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:111: * Sync method to get if an origin is set to be allowed. nit: Synchronous https://codereview.chromium.org/12211047/diff/3011/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:117: return false; How could we ever get a pref with the same name but the wrong type? That would indicate a serious error on our part, I don't think we'd want to silently ignore it. https://codereview.chromium.org/12211047/diff/3011/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:129: * Async method to get if an origin set to be allowed. nit: Asynchronous
https://chromiumcodereview.appspot.com/12211047/diff/1010/android_webview/nat... File android_webview/native/aw_contents.cc (right): https://chromiumcodereview.appspot.com/12211047/diff/1010/android_webview/nat... android_webview/native/aw_contents.cc:734: LOG(INFO) << "Running task geo"; nit: remove logging? https://chromiumcodereview.appspot.com/12211047/diff/1010/android_webview/nat... android_webview/native/aw_contents.cc:739: java_ref.get(env).obj(), This will NPE if java_ref has been collected. https://chromiumcodereview.appspot.com/12211047/diff/1010/android_webview/nat... android_webview/native/aw_contents.cc:760: GeolocationShowPrompt(java_ref_, origin); I think we should take a strong ref to the java_ref_ or make GeolocationShowPromptTask robust to receiving a null weak ref. I'd be inclined to take a Global ref here, early out if it's null to avoid posting unnecessary tasks, and then clear up the exta ref when we return from java with the answer to the prompt. You can then use const JavaRef<jobject>& as your parameter type for the helpers. Same in Invoke and Hide below. https://chromiumcodereview.appspot.com/12211047/diff/1010/android_webview/nat... android_webview/native/aw_contents.cc:790: it++; nit: style guide says to pre-increment. https://chromiumcodereview.appspot.com/12211047/diff/1010/android_webview/nat... File android_webview/native/aw_contents.h (right): https://chromiumcodereview.appspot.com/12211047/diff/1010/android_webview/nat... android_webview/native/aw_contents.h:138: void OnGeolocationHidePrompt(const GURL& origin); "On" prefix is normally used for IPC handlers, perhaps just remove it?
Addressed comments, PTAL https://chromiumcodereview.appspot.com/12211047/diff/3011/android_webview/bro... File android_webview/browser/aw_content_browser_client.cc (right): https://chromiumcodereview.appspot.com/12211047/diff/3011/android_webview/bro... android_webview/browser/aw_content_browser_client.cc:291: // TODO(boliu): Implement as part of geolocation code. On 2013/02/08 13:25:19, benm wrote: > nit: remove this todo now? Done. https://chromiumcodereview.appspot.com/12211047/diff/3011/android_webview/jav... File android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java (right): https://chromiumcodereview.appspot.com/12211047/diff/3011/android_webview/jav... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:111: * Sync method to get if an origin is set to be allowed. On 2013/02/08 13:25:19, benm wrote: > nit: Synchronous Done. https://chromiumcodereview.appspot.com/12211047/diff/3011/android_webview/jav... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:117: return false; On 2013/02/08 13:25:19, benm wrote: > How could we ever get a pref with the same name but the wrong type? That would > indicate a serious error on our part, I don't think we'd want to silently ignore > it. Agree, but I also don't think it is right to throw an unexpected Exception that might kill the program. I will clear the pref as that should fix it for future calls. https://chromiumcodereview.appspot.com/12211047/diff/3011/android_webview/jav... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:129: * Async method to get if an origin set to be allowed. On 2013/02/08 13:25:19, benm wrote: > nit: Asynchronous Done. https://chromiumcodereview.appspot.com/12211047/diff/3011/android_webview/nat... File android_webview/native/aw_contents.cc (right): https://chromiumcodereview.appspot.com/12211047/diff/3011/android_webview/nat... android_webview/native/aw_contents.cc:736: ConvertUTF8ToJavaString(env,origin.spec())); On 2013/02/08 02:15:01, joth wrote: > nit: space after comma Done. https://chromiumcodereview.appspot.com/12211047/diff/3011/android_webview/nat... android_webview/native/aw_contents.cc:763: GeolocationShowPrompt(java_ref_, geolocation_callbacks_.front().first); On 2013/02/08 02:15:01, joth wrote: > 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. Converted showprompt into a task, that actually found a bug that would have showed up in real apps, so well worth doing :-) https://chromiumcodereview.appspot.com/12211047/diff/3011/android_webview/nat... android_webview/native/aw_contents.cc:771: for ( ; it != geolocation_callbacks_.end(); ) { On 2013/02/08 02:15:01, joth wrote: > This construct has it's very own special syntax: > > while (it != geolocation_callbacks_.end()) { > > > :-) Done. https://chromiumcodereview.appspot.com/12211047/diff/3011/android_webview/nat... android_webview/native/aw_contents.cc:774: if (it == geolocation_callbacks_.begin()) { On 2013/02/08 02:15:01, joth wrote: > 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; > } > } > > 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. https://chromiumcodereview.appspot.com/12211047/diff/3011/android_webview/nat... File android_webview/native/aw_contents.h (right): https://chromiumcodereview.appspot.com/12211047/diff/3011/android_webview/nat... android_webview/native/aw_contents.h:194: typedef std::pair<const GURL&, base::Callback<void(bool)> > OriginCallback; On 2013/02/08 02:15:01, joth wrote: > add a comment that the Callback<void(bool)> is the content-layer supplied > function to run invoked passing the result of the application geolocation > permission prompt. > (Callback in the typename and member is pretty vague. Trying to think of a > suggestion to improve it. pending_geolocation_prompts_ ? > > also comment that the first item in the list is always the currently pending > request (i.e. request we sent to the application for which we're still awaiting > a result). Done. https://chromiumcodereview.appspot.com/12211047/diff/1010/android_webview/nat... File android_webview/native/aw_contents.cc (right): https://chromiumcodereview.appspot.com/12211047/diff/1010/android_webview/nat... android_webview/native/aw_contents.cc:734: LOG(INFO) << "Running task geo"; On 2013/02/12 10:46:30, benm wrote: > nit: remove logging? Done. https://chromiumcodereview.appspot.com/12211047/diff/1010/android_webview/nat... android_webview/native/aw_contents.cc:739: java_ref.get(env).obj(), On 2013/02/12 10:46:30, benm wrote: > This will NPE if java_ref has been collected. Done. https://chromiumcodereview.appspot.com/12211047/diff/1010/android_webview/nat... android_webview/native/aw_contents.cc:760: GeolocationShowPrompt(java_ref_, origin); On 2013/02/12 10:46:30, benm wrote: > I think we should take a strong ref to the java_ref_ or make > GeolocationShowPromptTask robust to receiving a null weak ref. > > I'd be inclined to take a Global ref here, early out if it's null to avoid > posting unnecessary tasks, and then clear up the exta ref when we return from > java with the answer to the prompt. You can then use const JavaRef<jobject>& as > your parameter type for the helpers. > > Same in Invoke and Hide below. I have checked if the weak pointer is not null before using. I don't think it makes sense to take the global ref as it doesn't make sense to call the prompt if the ref is going away. https://chromiumcodereview.appspot.com/12211047/diff/1010/android_webview/nat... android_webview/native/aw_contents.cc:790: it++; On 2013/02/12 10:46:30, benm wrote: > nit: style guide says to pre-increment. Done. https://chromiumcodereview.appspot.com/12211047/diff/1010/android_webview/nat... File android_webview/native/aw_contents.h (right): https://chromiumcodereview.appspot.com/12211047/diff/1010/android_webview/nat... android_webview/native/aw_contents.h:138: void OnGeolocationHidePrompt(const GURL& origin); On 2013/02/12 10:46:30, benm wrote: > "On" prefix is normally used for IPC handlers, perhaps just remove it? Removed On, and put the verb first.
https://codereview.chromium.org/12211047/diff/1010/android_webview/native/aw_... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12211047/diff/1010/android_webview/native/aw_... android_webview/native/aw_contents.cc:760: GeolocationShowPrompt(java_ref_, origin); I don't think checking for null is enough, it may get collected after you've checked for null. The only way to ensure the GC doesn't collect is if there is a strong ref to the object.
https://codereview.chromium.org/12211047/diff/1010/android_webview/native/aw_... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12211047/diff/1010/android_webview/native/aw_... android_webview/native/aw_contents.cc:760: GeolocationShowPrompt(java_ref_, origin); On 2013/02/12 19:03:21, benm wrote: > I don't think checking for null is enough, it may get collected after you've > checked for null. The only way to ensure the GC doesn't collect is if there is a > strong ref to the object. Right, but this is happening when you call get() (I think?), and it creates a local ref. Updated to keep the ref inside the method.
https://codereview.chromium.org/12211047/diff/1010/android_webview/native/aw_... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12211047/diff/1010/android_webview/native/aw_... android_webview/native/aw_contents.cc:760: GeolocationShowPrompt(java_ref_, origin); Sorry, I got confused this morning, I confused WeakPtr with WeakRef. Do what Ben says.
https://codereview.chromium.org/12211047/diff/1010/android_webview/native/aw_... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12211047/diff/1010/android_webview/native/aw_... android_webview/native/aw_contents.cc:760: GeolocationShowPrompt(java_ref_, origin); Yes, get() will return a ScopedJavaLocalRef. I just looked at the patch again and see that you are taking one of those (only spotted the null check last time), so I think it should be fine now.
https://codereview.chromium.org/12211047/diff/1010/android_webview/native/aw_... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12211047/diff/1010/android_webview/native/aw_... android_webview/native/aw_contents.cc:760: GeolocationShowPrompt(java_ref_, origin); On 2013/02/12 19:21:05, Ben Murdoch wrote: > Yes, get() will return a ScopedJavaLocalRef. I just looked at the patch again > and see that you are taking one of those (only spotted the null check last > time), so I think it should be fine now. I updated after your comment as I realized it could go bad between the if and the use :-) Sorry if that was not clear.
https://codereview.chromium.org/12211047/diff/9002/android_webview/native/aw_... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12211047/diff/9002/android_webview/native/aw_... android_webview/native/aw_contents.cc:802: if (java_ref_.get(env).obj()) { Think you need to call get() up a scope?
Addressed comments, PTAL https://codereview.chromium.org/12211047/diff/9002/android_webview/native/aw_... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12211047/diff/9002/android_webview/native/aw_... android_webview/native/aw_contents.cc:802: if (java_ref_.get(env).obj()) { On 2013/02/12 19:24:54, Ben Murdoch wrote: > Think you need to call get() up a scope? Done.
Sorry to keep banging on about the ClassCastException. I'm just not convinced that we need it/it's the right thing to do. https://codereview.chromium.org/12211047/diff/10011/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java (right): https://codereview.chromium.org/12211047/diff/10011/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:124: mSharedPreferences.edit().remove(getOriginKey(origin)).apply(); Sorry, I'm still not sure I like this. If the embedder messes up and manually stores a key with the same name but a different type (the only way we can get into this state, isn't it?), then WebView will silently kill whatever setting they have set rather than alerting them to the problem by crashing. https://codereview.chromium.org/12211047/diff/10011/android_webview/native/aw... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12211047/diff/10011/android_webview/native/aw... android_webview/native/aw_contents.cc:732: void ShowGeolocationPromptHelperTask(JavaObjectWeakGlobalRef java_ref, nit: const& ? (throughout)
Addressed comments, PTAL. https://codereview.chromium.org/12211047/diff/10011/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java (right): https://codereview.chromium.org/12211047/diff/10011/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwGeolocationPermissions.java:124: mSharedPreferences.edit().remove(getOriginKey(origin)).apply(); On 2013/02/12 19:40:33, benm wrote: > Sorry, I'm still not sure I like this. If the embedder messes up and manually > stores a key with the same name but a different type (the only way we can get > into this state, isn't it?), then WebView will silently kill whatever setting > they have set rather than alerting them to the problem by crashing. Done https://codereview.chromium.org/12211047/diff/10011/android_webview/native/aw... File android_webview/native/aw_contents.cc (right): https://codereview.chromium.org/12211047/diff/10011/android_webview/native/aw... android_webview/native/aw_contents.cc:732: void ShowGeolocationPromptHelperTask(JavaObjectWeakGlobalRef java_ref, On 2013/02/12 19:40:33, benm wrote: > nit: const& ? (throughout) Added const here. Didn't do it for the callback as it is not const or a reference in GeolocationPermissionContext.
lgtm Thanks Kristian!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kristianm@chromium.org/12211047/9004
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kristianm@chromium.org/12211047/9004
Message was sent while issue was closed.
Change committed as 182465 |
