Deny the geolocation permission request by default.

Deny the geolocation permission request by default.

This patch denies the request if there is no response when GeolocationPermission.Callback object is gc-ed.

BUG=513752
Committed: https://crrev.com/97d22bb63d36d4d3049ef8f4009503acefb72bc9
Cr-Commit-Position: refs/heads/master@{#341760}

[michaelbai, 2015-08-03 23:51:21]

michaelbai@chromium.org changed reviewers:
+ boliu@chromium.org

[boliu, 2015-08-04 00:37:16]

https://codereview.chromium.org/1267103003/diff/20001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/permission/AwGeolocationCallback.java
(right):

https://codereview.chromium.org/1267103003/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/permission/AwGeolocationCallback.java:27:
private WeakReference<AwContents> mAwContents;
Doesn't need to be weak I think. Previous implementation had an implicit strong
ref back anyway.

https://codereview.chromium.org/1267103003/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/permission/AwGeolocationCallback.java:67:
Log.w(TAG, "Response for this geolocation request has been received."
This means the logs in native code can't happen anymore, right? get rid of
those?

https://codereview.chromium.org/1267103003/diff/20001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java
(right):

https://codereview.chromium.org/1267103003/diff/20001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java:52:
+ "            gotPos, function() { }, { });\n"
should this be errorCallback too?

https://codereview.chromium.org/1267103003/diff/20001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java:279:
initAwContents(new TestAwContentsClient());
This one should be more explicit. You want the client
onGeolocationPermissionsShowPrompt to be no-op, right? Then create one that's
no-op.

Maybe someone will change the implementation in TestAwContentsClient and not
notice this.

[michaelbai, 2015-08-04 18:06:18]

PTAL

https://codereview.chromium.org/1267103003/diff/20001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/permission/AwGeolocationCallback.java
(right):

https://codereview.chromium.org/1267103003/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/permission/AwGeolocationCallback.java:27:
private WeakReference<AwContents> mAwContents;
On 2015/08/04 00:37:16, boliu wrote:
> Doesn't need to be weak I think. Previous implementation had an implicit
strong
> ref back anyway.

This object is passed to apps, misbehavior app could hold it even after WebView
is not needed (this is rare case), then WebView can't be gc-ed, we could prevent
it from happening, and there is no side effect.

https://codereview.chromium.org/1267103003/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/permission/AwGeolocationCallback.java:67:
Log.w(TAG, "Response for this geolocation request has been received."
On 2015/08/04 00:37:16, boliu wrote:
> This means the logs in native code can't happen anymore, right? get rid of
> those?

Removed the log from native side.

https://codereview.chromium.org/1267103003/diff/20001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java
(right):

https://codereview.chromium.org/1267103003/diff/20001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java:52:
+ "            gotPos, function() { }, { });\n"
On 2015/08/04 00:37:16, boliu wrote:
> should this be errorCallback too?

Done.

https://codereview.chromium.org/1267103003/diff/20001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java:279:
initAwContents(new TestAwContentsClient());
On 2015/08/04 00:37:16, boliu wrote:
> This one should be more explicit. You want the client
> onGeolocationPermissionsShowPrompt to be no-op, right? Then create one that's
> no-op.
> 
> Maybe someone will change the implementation in TestAwContentsClient and not
> notice this.

Done.

[boliu, 2015-08-04 18:35:05]

lgtm

https://codereview.chromium.org/1267103003/diff/20001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/permission/AwGeolocationCallback.java
(right):

https://codereview.chromium.org/1267103003/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/permission/AwGeolocationCallback.java:27:
private WeakReference<AwContents> mAwContents;
On 2015/08/04 18:06:18, michaelbai wrote:
> On 2015/08/04 00:37:16, boliu wrote:
> > Doesn't need to be weak I think. Previous implementation had an implicit
> strong
> > ref back anyway.
> 
> This object is passed to apps, misbehavior app could hold it even after
WebView
> is not needed (this is rare case), then WebView can't be gc-ed, we could
prevent
> it from happening, and there is no side effect. 

So you mean this is another separate fix.

https://codereview.chromium.org/1267103003/diff/40001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java
(right):

https://codereview.chromium.org/1267103003/diff/40001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java:71:
GeolocationPermissions.Callback callback) {
probably add a comment here this is an explicit no-op, and maybe move it closer
to where it's used

[michaelbai, 2015-08-04 18:59:41]

https://codereview.chromium.org/1267103003/diff/20001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/permission/AwGeolocationCallback.java
(right):

https://codereview.chromium.org/1267103003/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/permission/AwGeolocationCallback.java:27:
private WeakReference<AwContents> mAwContents;
On 2015/08/04 18:35:04, boliu wrote:
> On 2015/08/04 18:06:18, michaelbai wrote:
> > On 2015/08/04 00:37:16, boliu wrote:
> > > Doesn't need to be weak I think. Previous implementation had an implicit
> > strong
> > > ref back anyway.
> > 
> > This object is passed to apps, misbehavior app could hold it even after
> WebView
> > is not needed (this is rare case), then WebView can't be gc-ed, we could
> prevent
> > it from happening, and there is no side effect. 
> 
> So you mean this is another separate fix.

Yes

https://codereview.chromium.org/1267103003/diff/40001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java
(right):

https://codereview.chromium.org/1267103003/diff/40001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java:71:
GeolocationPermissions.Callback callback) {
On 2015/08/04 18:35:04, boliu wrote:
> probably add a comment here this is an explicit no-op, and maybe move it
closer
> to where it's used

added comment

[michaelbai, 2015-08-04 18:59:44]

The CQ bit was checked by michaelbai@chromium.org

[michaelbai, 2015-08-04 18:59:45]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/1267103003/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-08-04 19:00:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1267103003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1267103003/60001

[commit-bot: I haz the power, 2015-08-04 19:34:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-04 19:36:45]

Patchset 4 (id:??) landed as
https://crrev.com/97d22bb63d36d4d3049ef8f4009503acefb72bc9
Cr-Commit-Position: refs/heads/master@{#341760}