Keep the reference of passing request

Keep the reference of passing request

Otherwise the request will be gc-ed, and There is no onRequestCancelled invoked.

BUG=375972
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272268

[boliu, 2014-05-21 22:08:25]

https://codereview.chromium.org/292183007/diff/1/android_webview/javatests/sr...
File
android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java
(right):

https://codereview.chromium.org/292183007/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java:144:
if (mFirstRequest == awPermissionRequest)
Does this mean any app using this api also have to do this? That seems bad.

Why can't native AwPermissionRequest hold on to a strong ref to the java side?

https://codereview.chromium.org/292183007/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java:159:
assert (helper.canceled());
Assert is a language level thing, should be assertTrue, a junit condition test.

[michaelbai, 2014-05-21 22:17:03]

PTAL

https://codereview.chromium.org/292183007/diff/1/android_webview/javatests/sr...
File
android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java
(right):

https://codereview.chromium.org/292183007/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java:144:
if (mFirstRequest == awPermissionRequest)
Right, it because the user could drop the request without responding to us, in
this case the request should be GC-ed, and we can't hold the strong ref as we
don't know whether user will respond us or not.

On 2014/05/21 22:08:25, boliu wrote:
> Does this mean any app using this api also have to do this? That seems bad.
> 
> Why can't native AwPermissionRequest hold on to a strong ref to the java side?

https://codereview.chromium.org/292183007/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java:159:
assert (helper.canceled());
On 2014/05/21 22:08:25, boliu wrote:
> Assert is a language level thing, should be assertTrue, a junit condition
test.

Done.

[boliu, 2014-05-21 22:25:04]

https://codereview.chromium.org/292183007/diff/1/android_webview/javatests/sr...
File
android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java
(right):

https://codereview.chromium.org/292183007/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java:144:
if (mFirstRequest == awPermissionRequest)
On 2014/05/21 22:17:04, michaelbai wrote:
> Right, it because the user could drop the request without responding to us, in
> this case the request should be GC-ed, and we can't hold the strong ref as we
> don't know whether user will respond us or not.
> 
> On 2014/05/21 22:08:25, boliu wrote:
> > Does this mean any app using this api also have to do this? That seems bad.
> > 
> > Why can't native AwPermissionRequest hold on to a strong ref to the java
side?
> 

Hmm, ok. Then apps can't rely on Cancel always being called. Not sure that's a
great api design.

You are holding on to the first request object. Does that on navigate, a second
onPermissionRequest could come before the first one is cancelled?

[michaelbai, 2014-05-21 22:34:02]

On 2014/05/21 22:25:04, boliu wrote:
>
https://codereview.chromium.org/292183007/diff/1/android_webview/javatests/sr...
> File
>
android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java
> (right):
> 
>
https://codereview.chromium.org/292183007/diff/1/android_webview/javatests/sr...
>
android_webview/javatests/src/org/chromium/android_webview/test/MediaAccessPermissionRequestTest.java:144:
> if (mFirstRequest == awPermissionRequest)
> On 2014/05/21 22:17:04, michaelbai wrote:
> > Right, it because the user could drop the request without responding to us,
in
> > this case the request should be GC-ed, and we can't hold the strong ref as
we
> > don't know whether user will respond us or not.
> > 
> > On 2014/05/21 22:08:25, boliu wrote:
> > > Does this mean any app using this api also have to do this? That seems
bad.
> > > 
> > > Why can't native AwPermissionRequest hold on to a strong ref to the java
> side?
> > 
> 
> Hmm, ok. Then apps can't rely on Cancel always being called. Not sure that's a
> great api design.
> 
> You are holding on to the first request object. Does that on navigate, a
second
> onPermissionRequest could come before the first one is cancelled?


If users want to respond us, he should keep the strong ref of request,
otherwise, it
means he doesn't care about the request, in this case, we have warning in the
log when
the request is gc-ed, and because user doesn't care about the request, we
shouldn't bother
him about the cancellation of request.


Q2: No it will not as I can see, if this test is flaky again, it might means the
second request
could come before cancel is called.

[boliu, 2014-05-21 22:38:08]

On 2014/05/21 22:34:02, michaelbai wrote:
> Q2: No it will not as I can see, if this test is flaky again, it might means
the
> second request
> could come before cancel is called.

If the second request can not come before the first cancel, then can you change
assert (junit kind) that mFirstRequest is null in onPermissionRequest, and
assert it is not null in onPermissionRequestCanceled but set it to null at the
end. Also just name it mRequest

[michaelbai, 2014-05-21 23:06:17]

PTAL

[boliu, 2014-05-21 23:06:55]

lgtm, thanks!

[michaelbai, 2014-05-21 23:08:34]

The CQ bit was checked by michaelbai@chromium.org

[commit-bot: I haz the power, 2014-05-21 23:09:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaelbai@chromium.org/292183007/100001

[commit-bot: I haz the power, 2014-05-22 07:20:34]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)

[commit-bot: I haz the power, 2014-05-22 07:41:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-22 07:41:01]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)

[michaelbai, 2014-05-22 15:31:58]

The CQ bit was checked by michaelbai@chromium.org

[commit-bot: I haz the power, 2014-05-22 15:33:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaelbai@chromium.org/292183007/120001

[commit-bot: I haz the power, 2014-05-22 19:20:21]

Change committed as 272268