Add AsyncWaiter implementation to java API.

Add AsyncWaiter implementation to java API.

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

[qsr, 2014-05-15 16:24:45]

bulach@ or rmcilroy@ for review
viettrungluu@ for owners

[bulach, 2014-05-15 19:32:57]

lgtm, mostly nits. it'd be nice for ross to take a look at it at some point too.
thanks!

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/c...
File mojo/android/javatests/core_test.cc (right):

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/c...
mojo/android/javatests/core_test.cc:39: return reinterpret_cast<jlong>(new
TestEnvironment());
nit: s/jlong/inptr_t

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/c...
mojo/android/javatests/core_test.cc:44: jlong test_environment) {
nit: align

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/s...
File mojo/android/javatests/src/org/chromium/mojo/system/CoreTest.java (right):

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/s...
mojo/android/javatests/src/org/chromium/mojo/system/CoreTest.java:598: //
Closing the peer handle.
nit: you may want to split it into separate test cases?

https://codereview.chromium.org/288993002/diff/60001/mojo/android/system/core...
File mojo/android/system/core_impl.cc (right):

https://codereview.chromium.org/288993002/diff/60001/mojo/android/system/core...
mojo/android/system/core_impl.cc:23: JNIEnv* env;
better to just call AttachCurrentThread rather than caching env.

https://codereview.chromium.org/288993002/diff/60001/mojo/public/java/src/org...
File mojo/public/java/src/org/chromium/mojo/system/AsyncWaiter.java (right):

https://codereview.chromium.org/288993002/diff/60001/mojo/public/java/src/org...
mojo/public/java/src/org/chromium/mojo/system/AsyncWaiter.java:10: * Help
classes allowing asynchronously calling wait on a background thread, and pass
the result to
nit: the callback has also an error condition, so perhaps:

Allows asynchronously waiting on a background thread.
The given callback {@link AsyncWaiterCallback} will be notified
on the current thread.
Returns a {@link Cancellable} object that can be used to stop waiting
(and must only be used on the current thread.). The cancellable becomes
invalid once the callback has been notified.

https://codereview.chromium.org/288993002/diff/60001/mojo/public/java/src/org...
File mojo/public/java/src/org/chromium/mojo/system/AsyncWaiterCallback.java
(right):

https://codereview.chromium.org/288993002/diff/60001/mojo/public/java/src/org...
mojo/public/java/src/org/chromium/mojo/system/AsyncWaiterCallback.java:10:
public interface AsyncWaiterCallback {
nit: if this can only be used within the AsyncWaiter context, then having it as
inner interface there (like Cancellable) would keep it more contained.

[rmcilroy, 2014-05-15 22:36:51]

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/c...
File mojo/android/javatests/core_test.cc (right):

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/c...
mojo/android/javatests/core_test.cc:17: namespace {
Nit - newline between namespace and struct for clarity.

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/c...
mojo/android/javatests/core_test.cc:25: }
Ditto

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/s...
File mojo/android/javatests/src/org/chromium/mojo/system/CoreTest.java (right):

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/s...
mojo/android/javatests/src/org/chromium/mojo/system/CoreTest.java:37: private
static final long RUN_LOOP_DELAY = 50;
RUN_LOOP_TIMEOUT_MS

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/s...
mojo/android/javatests/src/org/chromium/mojo/system/CoreTest.java:583:
assertEquals(null, asyncWaiterResult.getException());
nit - would probably be clearer with newlines between asserts and next stage of
test (here and below)

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/s...
mojo/android/javatests/src/org/chromium/mojo/system/CoreTest.java:590:
nativeRunLoop(50);
Use RUN_LOOP_TIMEOUT_MS throughout.

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/s...
mojo/android/javatests/src/org/chromium/mojo/system/CoreTest.java:598: //
Closing the peer handle.
On 2014/05/15 19:32:58, bulach wrote:
> nit: you may want to split it into separate test cases?

+1 (and for each of the other test case).

https://codereview.chromium.org/288993002/diff/60001/mojo/android/system/core...
File mojo/android/system/core_impl.cc (right):

https://codereview.chromium.org/288993002/diff/60001/mojo/android/system/core...
mojo/android/system/core_impl.cc:22: struct AsyncWaitCallbackData {
Add a constructor and use it below.to setup fields.

https://codereview.chromium.org/288993002/diff/60001/mojo/android/system/core...
mojo/android/system/core_impl.cc:324: MojoAsyncWaiter* async_waiter =
mojo::GetDefaultAsyncWaiter();
nit - I think this could be scoped in the if branch below.

https://codereview.chromium.org/288993002/diff/60001/mojo/android/system/core...
mojo/android/system/core_impl.cc:342: env, jcaller, cancel_id,
reinterpret_cast<jlong>(callback_data));
reinterpret_cast<intptr_t>

https://codereview.chromium.org/288993002/diff/60001/mojo/android/system/src/...
File mojo/android/system/src/org/chromium/mojo/system/CoreImpl.java (right):

https://codereview.chromium.org/288993002/diff/60001/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/CoreImpl.java:501:
AsyncWaiterCancellableImpl cancellable) {
align

https://codereview.chromium.org/288993002/diff/60001/mojo/public/java/src/org...
File mojo/public/java/src/org/chromium/mojo/system/AsyncWaiter.java (right):

https://codereview.chromium.org/288993002/diff/60001/mojo/public/java/src/org...
mojo/public/java/src/org/chromium/mojo/system/AsyncWaiter.java:10: * Help
classes allowing asynchronously calling wait on a background thread, and pass
the result to
Most of this is describes the asyncWait method.  I would simply have here
something like:

A class which implements the AsyncWaiter allows asynchronously waiting on a 
background thread.  

Then augment the asyncWait javadoc as below.

https://codereview.chromium.org/288993002/diff/60001/mojo/public/java/src/org...
mojo/public/java/src/org/chromium/mojo/system/AsyncWaiter.java:17: * Allow to
stop waiting.
Allows cancelation of an asyncWait operation.

https://codereview.chromium.org/288993002/diff/60001/mojo/public/java/src/org...
mojo/public/java/src/org/chromium/mojo/system/AsyncWaiter.java:21: * Stop
waiting. Has no effect if the callback has already been called. Must be called
from
Cancels an asyncWait operation.  Has no effect if the operation has already been
canceled or the callback has already been called. Must be called from the same
thread as {@link AsyncWaiter#asyncWait(Handle, WaitFlags, long,
AsyncWaiterCallback)} was called from.

https://codereview.chromium.org/288993002/diff/60001/mojo/public/java/src/org...
mojo/public/java/src/org/chromium/mojo/system/AsyncWaiter.java:30: * callback on
the current thread.
Asynchronously call wait on a background thread.  The given
{@link AsyncWaiterCallback} will be notified of the result of the wait 
on the same thread as asyncWait was called.

@return a {@link Cancellable} object that can be used to cancel waiting.
The cancellable should only be used on the current thread, and becomes
invalid once the callback has been notified.

https://codereview.chromium.org/288993002/diff/60001/mojo/public/java/src/org...
File mojo/public/java/src/org/chromium/mojo/system/AsyncWaiterCallback.java
(right):

https://codereview.chromium.org/288993002/diff/60001/mojo/public/java/src/org...
mojo/public/java/src/org/chromium/mojo/system/AsyncWaiterCallback.java:10:
public interface AsyncWaiterCallback {
On 2014/05/15 19:32:58, bulach wrote:
> nit: if this can only be used within the AsyncWaiter context, then having it
as
> inner interface there (like Cancellable) would keep it more contained.

+1

[qsr, 2014-05-19 12:12:09]

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/c...
File mojo/android/javatests/core_test.cc (right):

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/c...
mojo/android/javatests/core_test.cc:17: namespace {
On 2014/05/15 22:36:51, rmcilroy wrote:
> Nit - newline between namespace and struct for clarity.

Done.

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/c...
mojo/android/javatests/core_test.cc:25: }
On 2014/05/15 22:36:51, rmcilroy wrote:
> Ditto

Done.

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/c...
mojo/android/javatests/core_test.cc:39: return reinterpret_cast<jlong>(new
TestEnvironment());
On 2014/05/15 19:32:58, bulach wrote:
> nit: s/jlong/inptr_t

Done.

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/c...
mojo/android/javatests/core_test.cc:44: jlong test_environment) {
On 2014/05/15 19:32:58, bulach wrote:
> nit: align

Done.

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/s...
File mojo/android/javatests/src/org/chromium/mojo/system/CoreTest.java (right):

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/s...
mojo/android/javatests/src/org/chromium/mojo/system/CoreTest.java:37: private
static final long RUN_LOOP_DELAY = 50;
On 2014/05/15 22:36:51, rmcilroy wrote:
> RUN_LOOP_TIMEOUT_MS

Done.

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/s...
mojo/android/javatests/src/org/chromium/mojo/system/CoreTest.java:583:
assertEquals(null, asyncWaiterResult.getException());
On 2014/05/15 22:36:51, rmcilroy wrote:
> nit - would probably be clearer with newlines between asserts and next stage
of
> test (here and below) 

Done.

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/s...
mojo/android/javatests/src/org/chromium/mojo/system/CoreTest.java:590:
nativeRunLoop(50);
On 2014/05/15 22:36:51, rmcilroy wrote:
> Use RUN_LOOP_TIMEOUT_MS throughout.

Done.

https://codereview.chromium.org/288993002/diff/60001/mojo/android/javatests/s...
mojo/android/javatests/src/org/chromium/mojo/system/CoreTest.java:598: //
Closing the peer handle.
On 2014/05/15 19:32:58, bulach wrote:
> nit: you may want to split it into separate test cases?

Done.

https://codereview.chromium.org/288993002/diff/60001/mojo/android/system/core...
File mojo/android/system/core_impl.cc (right):

https://codereview.chromium.org/288993002/diff/60001/mojo/android/system/core...
mojo/android/system/core_impl.cc:22: struct AsyncWaitCallbackData {
On 2014/05/15 22:36:51, rmcilroy wrote:
> Add a constructor and use it below.to setup fields.

Done.

https://codereview.chromium.org/288993002/diff/60001/mojo/android/system/core...
mojo/android/system/core_impl.cc:23: JNIEnv* env;
 Done, but any reason for this, knowing that everything must be called on the
same thread (this is from all the API of the async waiter).

https://codereview.chromium.org/288993002/diff/60001/mojo/android/system/core...
mojo/android/system/core_impl.cc:324: MojoAsyncWaiter* async_waiter =
mojo::GetDefaultAsyncWaiter();
On 2014/05/15 22:36:51, rmcilroy wrote:
> nit - I think this could be scoped in the if branch below.

Done.

https://codereview.chromium.org/288993002/diff/60001/mojo/android/system/core...
mojo/android/system/core_impl.cc:342: env, jcaller, cancel_id,
reinterpret_cast<jlong>(callback_data));
On 2014/05/15 22:36:51, rmcilroy wrote:
> reinterpret_cast<intptr_t>

Done.

https://codereview.chromium.org/288993002/diff/60001/mojo/android/system/src/...
File mojo/android/system/src/org/chromium/mojo/system/CoreImpl.java (right):

https://codereview.chromium.org/288993002/diff/60001/mojo/android/system/src/...
mojo/android/system/src/org/chromium/mojo/system/CoreImpl.java:501:
AsyncWaiterCancellableImpl cancellable) {
On 2014/05/15 22:36:51, rmcilroy wrote:
> align

 My autoformatter refuse to do alignment for this, it wants to indent 8 chars.
If you do not mind, I prefer to keep a style that works with it, as it makes it
a lot simpler to edit this file.

https://codereview.chromium.org/288993002/diff/60001/mojo/public/java/src/org...
File mojo/public/java/src/org/chromium/mojo/system/AsyncWaiter.java (right):

https://codereview.chromium.org/288993002/diff/60001/mojo/public/java/src/org...
mojo/public/java/src/org/chromium/mojo/system/AsyncWaiter.java:10: * Help
classes allowing asynchronously calling wait on a background thread, and pass
the result to
On 2014/05/15 22:36:51, rmcilroy wrote:
> Most of this is describes the asyncWait method.  I would simply have here
> something like:
> 
> A class which implements the AsyncWaiter allows asynchronously waiting on a 
> background thread.  
> 
> Then augment the asyncWait javadoc as below.

Done.

https://codereview.chromium.org/288993002/diff/60001/mojo/public/java/src/org...
mojo/public/java/src/org/chromium/mojo/system/AsyncWaiter.java:17: * Allow to
stop waiting.
On 2014/05/15 22:36:51, rmcilroy wrote:
> Allows cancelation of an asyncWait operation.

Done.

https://codereview.chromium.org/288993002/diff/60001/mojo/public/java/src/org...
mojo/public/java/src/org/chromium/mojo/system/AsyncWaiter.java:21: * Stop
waiting. Has no effect if the callback has already been called. Must be called
from
On 2014/05/15 22:36:51, rmcilroy wrote:
> Cancels an asyncWait operation.  Has no effect if the operation has already
been
> canceled or the callback has already been called. Must be called from the same
> thread as {@link AsyncWaiter#asyncWait(Handle, WaitFlags, long,
> AsyncWaiterCallback)} was called from.

Done.

https://codereview.chromium.org/288993002/diff/60001/mojo/public/java/src/org...
mojo/public/java/src/org/chromium/mojo/system/AsyncWaiter.java:30: * callback on
the current thread.
On 2014/05/15 22:36:51, rmcilroy wrote:
> Asynchronously call wait on a background thread.  The given
> {@link AsyncWaiterCallback} will be notified of the result of the wait 
> on the same thread as asyncWait was called.
> 
> @return a {@link Cancellable} object that can be used to cancel waiting.
> The cancellable should only be used on the current thread, and becomes
> invalid once the callback has been notified.

Done.

https://codereview.chromium.org/288993002/diff/60001/mojo/public/java/src/org...
File mojo/public/java/src/org/chromium/mojo/system/AsyncWaiterCallback.java
(right):

https://codereview.chromium.org/288993002/diff/60001/mojo/public/java/src/org...
mojo/public/java/src/org/chromium/mojo/system/AsyncWaiterCallback.java:10:
public interface AsyncWaiterCallback {
 That was causing an issue with our jni generator. Updated the generator in a //
CL, and move this class to  AsyncWaiter

[rmcilroy, 2014-05-19 13:06:27]

lgtm other than one question and a small nit.

https://codereview.chromium.org/288993002/diff/100001/mojo/android/javatests/...
File mojo/android/javatests/src/org/chromium/mojo/system/CoreTest.java (right):

https://codereview.chromium.org/288993002/diff/100001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/system/CoreTest.java:738:
WaitFlags.none().setReadable(true), RUN_LOOP_TIMEOUT_MS / 4, asyncWaiterResult);
Is there the possibility of flake here? How much precision does the asyncWait's
deadline have?

https://codereview.chromium.org/288993002/diff/100001/mojo/android/system/cor...
File mojo/android/system/core_impl.cc (right):

https://codereview.chromium.org/288993002/diff/100001/mojo/android/system/cor...
mojo/android/system/core_impl.cc:336: cancel_id = 0;
nit - maybe replace '0' with a public static final INVALID_HANDLE_CANCEL_ID = 0
here and below in CancelAsyncWait.  This should make CancelAsyncWait clearer.

https://codereview.chromium.org/288993002/diff/100001/mojo/android/system/cor...
mojo/android/system/core_impl.cc:355: // AsyncWaitCallback will be called and
will clear the data_ptr.
With the change above, the comment could be:
  // If |id| is INVALID_HANDLE_CANCEL_ID, then the AsyncWaitCallback
  // will be called via PostTask and will clear the data_ptr.

[qsr, 2014-05-19 14:43:21]

https://codereview.chromium.org/288993002/diff/100001/mojo/android/javatests/...
File mojo/android/javatests/src/org/chromium/mojo/system/CoreTest.java (right):

https://codereview.chromium.org/288993002/diff/100001/mojo/android/javatests/...
mojo/android/javatests/src/org/chromium/mojo/system/CoreTest.java:738:
WaitFlags.none().setReadable(true), RUN_LOOP_TIMEOUT_MS / 4, asyncWaiterResult);
On 2014/05/19 13:06:27, rmcilroy wrote:
> Is there the possibility of flake here? How much precision does the
asyncWait's
> deadline have?

 This is not specified. In practice, I didn't get any flake yet, but it might
happen. Would you prefer I change this by RUN_LOOP_TIMEOUT_MS and run the loop
10 times this delay? That would only be 50ms with the current values, so should
be fine. WDYT?

https://codereview.chromium.org/288993002/diff/100001/mojo/android/system/cor...
File mojo/android/system/core_impl.cc (right):

https://codereview.chromium.org/288993002/diff/100001/mojo/android/system/cor...
mojo/android/system/core_impl.cc:336: cancel_id = 0;
On 2014/05/19 13:06:27, rmcilroy wrote:
> nit - maybe replace '0' with a public static final INVALID_HANDLE_CANCEL_ID =
0
> here and below in CancelAsyncWait.  This should make CancelAsyncWait clearer.

Done.

https://codereview.chromium.org/288993002/diff/100001/mojo/android/system/cor...
mojo/android/system/core_impl.cc:355: // AsyncWaitCallback will be called and
will clear the data_ptr.
On 2014/05/19 13:06:27, rmcilroy wrote:
> With the change above, the comment could be:
>   // If |id| is INVALID_HANDLE_CANCEL_ID, then the AsyncWaitCallback
>   // will be called via PostTask and will clear the data_ptr.

Done.

[qsr, 2014-05-19 14:43:41]

viettrungluu@ -> ping for OWNERS

[viettrungluu, 2014-05-19 19:49:01]

LGTM w/nits.

https://codereview.chromium.org/288993002/diff/140001/mojo/android/javatests/...
File mojo/android/javatests/core_test.cc (right):

https://codereview.chromium.org/288993002/diff/140001/mojo/android/javatests/...
mojo/android/javatests/core_test.cc:23: }
Nit:  "// namespace" and blank line before this.

https://codereview.chromium.org/288993002/diff/140001/mojo/android/javatests/...
mojo/android/javatests/core_test.cc:43: delete
reinterpret_cast<TestEnvironment*>(
Can't you just reinterpret_cast without the static_cast first?

https://codereview.chromium.org/288993002/diff/140001/mojo/public/java/src/or...
File mojo/public/java/src/org/chromium/mojo/system/AsyncWaiter.java (right):

https://codereview.chromium.org/288993002/diff/140001/mojo/public/java/src/or...
mojo/public/java/src/org/chromium/mojo/system/AsyncWaiter.java:16: * Allows
cancelation of an asyncWait operation.
"cancellation" (two l's)

[qsr, 2014-05-20 07:47:18]

https://chromiumcodereview.appspot.com/288993002/diff/140001/mojo/android/jav...
File mojo/android/javatests/core_test.cc (right):

https://chromiumcodereview.appspot.com/288993002/diff/140001/mojo/android/jav...
mojo/android/javatests/core_test.cc:23: }
On 2014/05/19 19:49:01, viettrungluu wrote:
> Nit:  "// namespace" and blank line before this.

Done.

https://chromiumcodereview.appspot.com/288993002/diff/140001/mojo/android/jav...
mojo/android/javatests/core_test.cc:43: delete
reinterpret_cast<TestEnvironment*>(
On 2014/05/19 19:49:01, viettrungluu wrote:
> Can't you just reinterpret_cast without the static_cast first?

done. Wasn't sure about reinterpret_cast on data of different sizes.

https://chromiumcodereview.appspot.com/288993002/diff/140001/mojo/public/java...
File mojo/public/java/src/org/chromium/mojo/system/AsyncWaiter.java (right):

https://chromiumcodereview.appspot.com/288993002/diff/140001/mojo/public/java...
mojo/public/java/src/org/chromium/mojo/system/AsyncWaiter.java:16: * Allows
cancelation of an asyncWait operation.
On 2014/05/19 19:49:01, viettrungluu wrote:
> "cancellation" (two l's)

Done.

[qsr, 2014-05-20 07:48:54]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-05-20 07:49:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qsr@chromium.org/288993002/160001

[qsr, 2014-05-20 10:46:14]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-05-20 10:46:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qsr@chromium.org/288993002/180001

[commit-bot: I haz the power, 2014-05-20 12:17:19]

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:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-05-20 15:36:36]

Change committed as 271660