Add mojo service to get notified of vsync.

Add mojo service to get notified of vsync.

R=ppi@chromium.org
BUG=Fixes https://github.com/domokit/mojo/issues/535

Committed: https://chromium.googlesource.com/external/mojo/+/057118f7d346a5633044bd1ab4459d973b96e952

[ppi, 2015-11-17 16:40:46]

https://codereview.chromium.org/1458453003/diff/40001/mojo/services/vsync/int...
File mojo/services/vsync/interfaces/vsync.mojom (right):

https://codereview.chromium.org/1458453003/diff/40001/mojo/services/vsync/int...
mojo/services/vsync/interfaces/vsync.mojom:9: // Timebase is in
MojoGetTimeTicksNow.
Please document what this does.

E.g. Waits for the next vsync and returns its timestamp once it happens.

https://codereview.chromium.org/1458453003/diff/40001/mojo/services/vsync/int...
mojo/services/vsync/interfaces/vsync.mojom:9: // Timebase is in
MojoGetTimeTicksNow.
The source of this timestamp is actually not MojoGetTimeTicksNow(), we get this
from the OS and it represents time "since boot, not counting time spent in deep
sleep.". Is this in line with what we use for mojo timeticks on Android?

https://codereview.chromium.org/1458453003/diff/40001/services/vsync/src/org/...
File services/vsync/src/org/chromium/mojo/vsync/VSyncProviderImpl.java (right):

https://codereview.chromium.org/1458453003/diff/40001/services/vsync/src/org/...
services/vsync/src/org/chromium/mojo/vsync/VSyncProviderImpl.java:19: public
class VSyncProviderImpl implements VSyncProvider, Choreographer.FrameCallback {
nit: listening to FrameCallback should be an implementation detail. Can we
implement this in an inner private class?

https://codereview.chromium.org/1458453003/diff/40001/services/vsync/src/org/...
services/vsync/src/org/chromium/mojo/vsync/VSyncProviderImpl.java:62:
setBinding(null);
should we clear mCallback here? Is it a problem if we get doFrame called after
we unbind the binding upon error?

https://codereview.chromium.org/1458453003/diff/40001/shell/android/apk/src/o...
File shell/android/apk/src/org/chromium/mojo/shell/JavaApplicationRegistry.java
(right):

https://codereview.chromium.org/1458453003/diff/40001/shell/android/apk/src/o...
shell/android/apk/src/org/chromium/mojo/shell/JavaApplicationRegistry.java:35:
private final HandlerThread mHandlerThread = new
HandlerThread("FrameworkThread");
// Thread with a Looper required for VsyncFactory.

https://codereview.chromium.org/1458453003/diff/40001/shell/android/apk/src/o...
File shell/android/apk/src/org/chromium/mojo/shell/VsyncFactory.java (right):

https://codereview.chromium.org/1458453003/diff/40001/shell/android/apk/src/o...
shell/android/apk/src/org/chromium/mojo/shell/VsyncFactory.java:16: * A
ServiceFactoryBinder for the input service.
for the vsync service?

https://codereview.chromium.org/1458453003/diff/40001/shell/android/apk/src/o...
shell/android/apk/src/org/chromium/mojo/shell/VsyncFactory.java:19: private
final HandlerThread mHandlerThread;
Can we be passing around just the looper?

[qsr, 2015-11-17 17:19:41]

https://codereview.chromium.org/1458453003/diff/40001/mojo/services/vsync/int...
File mojo/services/vsync/interfaces/vsync.mojom (right):

https://codereview.chromium.org/1458453003/diff/40001/mojo/services/vsync/int...
mojo/services/vsync/interfaces/vsync.mojom:9: // Timebase is in
MojoGetTimeTicksNow.
On 2015/11/17 16:40:46, ppi wrote:
> Please document what this does.
> 
> E.g. Waits for the next vsync and returns its timestamp once it happens.

Done.

https://codereview.chromium.org/1458453003/diff/40001/mojo/services/vsync/int...
mojo/services/vsync/interfaces/vsync.mojom:9: // Timebase is in
MojoGetTimeTicksNow.
On 2015/11/17 16:40:46, ppi wrote:
> The source of this timestamp is actually not MojoGetTimeTicksNow(), we get
this
> from the OS and it represents time "since boot, not counting time spent in
deep
> sleep.". Is this in line with what we use for mojo timeticks on Android?

Removed any reference of the timestamp, saying that it can only be compared with
other calls of this method.

https://codereview.chromium.org/1458453003/diff/40001/services/vsync/src/org/...
File services/vsync/src/org/chromium/mojo/vsync/VSyncProviderImpl.java (right):

https://codereview.chromium.org/1458453003/diff/40001/services/vsync/src/org/...
services/vsync/src/org/chromium/mojo/vsync/VSyncProviderImpl.java:19: public
class VSyncProviderImpl implements VSyncProvider, Choreographer.FrameCallback {
On 2015/11/17 16:40:46, ppi wrote:
> nit: listening to FrameCallback should be an implementation detail. Can we
> implement this in an inner private class?

As discussed, this class is already an implementation detail by itself. Given
the cost of new classes and method calls on Android, I won't add a new class
here.

https://codereview.chromium.org/1458453003/diff/40001/services/vsync/src/org/...
services/vsync/src/org/chromium/mojo/vsync/VSyncProviderImpl.java:62:
setBinding(null);
On 2015/11/17 16:40:46, ppi wrote:
> should we clear mCallback here? Is it a problem if we get doFrame called after
> we unbind the binding upon error?

It is not a problem in the current state. If a clear mCallback, then I need to
check for null in doFrame, which I'd rather not do.

https://codereview.chromium.org/1458453003/diff/40001/shell/android/apk/src/o...
File shell/android/apk/src/org/chromium/mojo/shell/JavaApplicationRegistry.java
(right):

https://codereview.chromium.org/1458453003/diff/40001/shell/android/apk/src/o...
shell/android/apk/src/org/chromium/mojo/shell/JavaApplicationRegistry.java:35:
private final HandlerThread mHandlerThread = new
HandlerThread("FrameworkThread");
On 2015/11/17 16:40:46, ppi wrote:
> // Thread with a Looper required for VsyncFactory.

Done. But didn't speak about VSyncFactory, as the idea is to be able to reuse
this thread if other cases appear.

https://codereview.chromium.org/1458453003/diff/40001/shell/android/apk/src/o...
File shell/android/apk/src/org/chromium/mojo/shell/VsyncFactory.java (right):

https://codereview.chromium.org/1458453003/diff/40001/shell/android/apk/src/o...
shell/android/apk/src/org/chromium/mojo/shell/VsyncFactory.java:16: * A
ServiceFactoryBinder for the input service.
On 2015/11/17 16:40:46, ppi wrote:
> for the vsync service?

Done.

https://codereview.chromium.org/1458453003/diff/40001/shell/android/apk/src/o...
shell/android/apk/src/org/chromium/mojo/shell/VsyncFactory.java:19: private
final HandlerThread mHandlerThread;
On 2015/11/17 16:40:46, ppi wrote:
> Can we be passing around just the looper?

Done.

[ppi, 2015-11-17 17:54:22]

lgtm

[qsr, 2015-11-17 17:59:54]

Committed patchset #4 (id:60001) manually as
057118f7d346a5633044bd1ab4459d973b96e952 (presubmit successful).

[abarth-chromium, 2015-11-17 18:03:35]

abarth@chromium.org changed reviewers:
+ abarth@chromium.org

[abarth-chromium, 2015-11-17 18:03:36]

https://codereview.chromium.org/1458453003/diff/60001/services/vsync/src/org/...
File services/vsync/src/org/chromium/mojo/vsync/VSyncProviderImpl.java (right):

https://codereview.chromium.org/1458453003/diff/60001/services/vsync/src/org/...
services/vsync/src/org/chromium/mojo/vsync/VSyncProviderImpl.java:73:
mRunLoop.postDelayedTask(new Runnable() {
Why do we need an extra thread hop here?  The Sky version of this class doesn't
need the thread hop.  This adds yet-another-context-switch to the main animation
pipeline, which reduces the amount of time we have to do real work per frame.

[qsr, 2015-11-18 13:16:56]

https://codereview.chromium.org/1458453003/diff/60001/services/vsync/src/org/...
File services/vsync/src/org/chromium/mojo/vsync/VSyncProviderImpl.java (right):

https://codereview.chromium.org/1458453003/diff/60001/services/vsync/src/org/...
services/vsync/src/org/chromium/mojo/vsync/VSyncProviderImpl.java:73:
mRunLoop.postDelayedTask(new Runnable() {
On 2015/11/17 18:03:36, abarth-chromium wrote:
> Why do we need an extra thread hop here?  The Sky version of this class
doesn't
> need the thread hop.  This adds yet-another-context-switch to the main
animation
> pipeline, which reduces the amount of time we have to do real work per frame.

My guess is that sky is using the main thread to run this code? By default, out
of the main thread, a thread has only one of a MessageLoop and a Looper. We need
a MessageLoop to listen on mojo handler, and Choreographer needs a Looper to
send its callbacks. That's why we have this thread hop.

I changed this in https://codereview.chromium.org/1456913002/ by creating a new
thread with both looper and message loop for this service.