Remove Sync GetPose VRService call, implement VRVSyncProvider

Remove Sync GetPose VRService call, implement VRVSyncProvider

This CL removes the Synchronous GetPose call from the VRService, in favor of a VRVSyncProvider that sends the pose with each VSync.

BUG=655722
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2624633002
Cr-Commit-Position: refs/heads/master@{#446581}
Committed: https://chromium.googlesource.com/chromium/src/+/a316edeb21fbc8c122dbe64232c72c574155ed1b

[mthiesse, 2017-01-09 23:50:32]

mthiesse@chromium.org changed reviewers:
+ bajones@chromium.org, klausw@chromium.org

[mthiesse, 2017-01-09 23:50:32]

PTAL,

This CL isn't quite done, see the description. I haven't spent much time
measuring the performance improvements here; this new approach messes with the
webvr.info FPS meter (the rAF isn't fired at consistent intervals due to the
asynchronous VSync, so the meter jumps around a lot), but the animating cube
moves more smoothly :P

[bajones, 2017-01-10 00:22:18]

Skimmed through and looks pretty good, with a couple of quick comments. I want
to look through it a bit more thoroughly later.

https://codereview.chromium.org/2624633002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2624633002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:625: if
(m_canUpdateFramePose)
I think we can safely drop the logic behind m_canUpdateFramePose in this patch
and just always update it in OnVSync()

https://codereview.chromium.org/2624633002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:629:
doc->loader()->timing().referenceMonotonicTime() + timeSeconds);
How confident are we that this will align with the values provided to
window.requestAnimationFrame? Ideally they should match.

[mthiesse, 2017-01-10 00:25:24]

https://codereview.chromium.org/2624633002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2624633002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:629:
doc->loader()->timing().referenceMonotonicTime() + timeSeconds);
On 2017/01/10 00:22:17, bajones wrote:
> How confident are we that this will align with the values provided to
> window.requestAnimationFrame? Ideally they should match.

I haven't checked, but I can pretty much guarantee they won't match as-is. How
important is it that they match, and what purpose does that serve?

[klausw, 2017-01-10 00:35:23]

Looks generally promising, though of course we'd need non-presenting mode to
work also.

As discussed separately I'm still not convinced that it's a good idea to
completely remove getPose since it would be preferable to do a just-in-time
update at some point for fresher poses. But I guess we can always reintroduce
getPose later if we do want to go that route, so I'm ok with the vsync-linked
method for now.

https://codereview.chromium.org/2624633002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2624633002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:625: if
(m_canUpdateFramePose)
On 2017/01/10 00:22:17, bajones wrote:
> I think we can safely drop the logic behind m_canUpdateFramePose in this patch
> and just always update it in OnVSync()

Strictly speaking that may be a spec violation. As written, if a rAF callback
doesn't call submitFrame, it seems to be required to keep reusing the same pose
for the next rAF too. I don't think that's the intention though, and I think it
would be saner to use a fresh pose.

https://codereview.chromium.org/2624633002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:629:
doc->loader()->timing().referenceMonotonicTime() + timeSeconds);
On 2017/01/10 00:25:24, mthiesse wrote:
> On 2017/01/10 00:22:17, bajones wrote:
> > How confident are we that this will align with the values provided to
> > window.requestAnimationFrame? Ideally they should match.
> 
> I haven't checked, but I can pretty much guarantee they won't match as-is. How
> important is it that they match, and what purpose does that serve?

This becomes important when switching between window.rAF (not presenting) and
VRDisplay.rAF (when presenting), the time must not jump around during
transitions.

[mthiesse, 2017-01-11 22:39:46]

Description was changed from

==========
Remove Sync GetPose VRService call, impelement VRVSyncProvider(Client)

This CL isn't quite done but I'd like some feedback before I fix non-presenting
rendering.
(If you patch this and try it out you'll notice rAF isn't fired until you start
presenting)

BUG=655722
==========

to

==========
Remove Sync GetPose VRService call, impelement VRVSyncProvider(Client)

This CL isn't quite done but I'd like some feedback before I fix non-presenting
rendering.
(If you patch this and try it out you'll notice rAF isn't fired until you start
presenting)

BUG=655722
==========

[mthiesse, 2017-01-11 22:41:23]

Description was changed from

==========
Remove Sync GetPose VRService call, impelement VRVSyncProvider(Client)

This CL isn't quite done but I'd like some feedback before I fix non-presenting
rendering.
(If you patch this and try it out you'll notice rAF isn't fired until you start
presenting)

BUG=655722
==========

to

==========
Remove Sync GetPose VRService call, impelement VRVSyncProvider

This CL removes the Synchronous GetPose call from the VRService, in favor of a
VRVSyncProvider that sends the pose with each VSync.

BUG=655722
==========

[mthiesse, 2017-01-11 22:42:20]

PTAL, implementation is finished. I did some re-jiggering of VrShellGl's render
loop to make things simpler.

[mthiesse, 2017-01-11 22:43:14]

mthiesse@chromium.org changed reviewers:
+ dcheng@chromium.org

[mthiesse, 2017-01-11 22:43:14]

dcheng@ Please review .mojom changes.

[dcheng, 2017-01-12 09:56:56]

https://codereview.chromium.org/2624633002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2624633002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:962: callback_ =
std::move(callback);
One thing I'm not certain about is if it's OK to just drop pending callbacks and
never invoke them inside an impl. I would expect that the calling side would
need to track some mapping of outstanding calls to the result callbacks, and not
invoking them at all would cause those to never be freed.

https://codereview.chromium.org/2624633002/diff/20001/device/vr/vr_service.mojom
File device/vr/vr_service.mojom (right):

https://codereview.chromium.org/2624633002/diff/20001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:93: GetVRVSyncProvider(VRVSyncProvider& request);
Out of curiosity, why a separate interface?

https://codereview.chromium.org/2624633002/diff/20001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:97: GetVSync() => (VRPose? pose, double
time_seconds);
Is it possible to use mojo.common.mojom.TimeTicks here?

(I think we might need to add a Blink typemap though...)

https://codereview.chromium.org/2624633002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2624633002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:646: 100);  // Short delay to
throttle requests.
My understanding is the impl side is hosted in the browser process. If this pipe
was ever unexpectedly disconnected, that would indicate the browser process is
shutting down (in which case this would never succeed). Is it necessary to try
to reconnect?

[mthiesse, 2017-01-12 15:49:03]

https://codereview.chromium.org/2624633002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2624633002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:962: callback_ =
std::move(callback);
On 2017/01/12 09:56:56, dcheng wrote:
> One thing I'm not certain about is if it's OK to just drop pending callbacks
and
> never invoke them inside an impl. I would expect that the calling side would
> need to track some mapping of outstanding calls to the result callbacks, and
not
> invoking them at all would cause those to never be freed.

It's not okay to just drop pending callbacks. I've addressed this by calling any
outstanding callback in the destructor.

Given that this is a send->ack->send->ack loop we'll never have more than one
outstanding callback. (just added a CHECK to verify this)

https://codereview.chromium.org/2624633002/diff/20001/device/vr/vr_service.mojom
File device/vr/vr_service.mojom (right):

https://codereview.chromium.org/2624633002/diff/20001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:93: GetVRVSyncProvider(VRVSyncProvider& request);
On 2017/01/12 09:56:56, dcheng wrote:
> Out of curiosity, why a separate interface?

We bind to the VRVSyncProvider on a separate thread with a separate task runner
from the VRDisplay (sometimes).

There are two VRVSyncProvider implementations, the non-presenting implementation
that lives in the UI thread, and the presenting implementation that lives in the
GL thread (both live in the browser process though).

Let me know if I'm missing an obviously better approach.

https://codereview.chromium.org/2624633002/diff/20001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:97: GetVSync() => (VRPose? pose, double
time_seconds);
On 2017/01/12 09:56:56, dcheng wrote:
> Is it possible to use mojo.common.mojom.TimeTicks here?
> 
> (I think we might need to add a Blink typemap though...)

Would using TimeDelta be okay? I should probably add a comment explaining this,
but the code currently assumes that the VSync provider and the blink have
different timebases (whether or not they do in practice I don't actually know),
so time_seconds is just treated as a delta from the first VSync received.


Alternatively, I could make sure the timebases are the same and use TimeTicks...
Are WTF::monotonicallyIncreasingTime() and base::Timeticks::Now guaranteed to be
the same?

https://codereview.chromium.org/2624633002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2624633002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:646: 100);  // Short delay to
throttle requests.
On 2017/01/12 09:56:56, dcheng wrote:
> My understanding is the impl side is hosted in the browser process. If this
pipe
> was ever unexpectedly disconnected, that would indicate the browser process is
> shutting down (in which case this would never succeed). Is it necessary to try
> to reconnect?

No, this pipe shutting down doesn't indicate the browser process is shutting
down. It indicates that we're switching from one VRVSyncProvider to another.
Rather than send a new message saying "Hey, your VSync provider is gone but
another one is available", we use the disconnected error to signal this. It's
assumed that there's always a VRVSyncProvider available.

Is there a cleaner approach here? Can we move a binding to another thread
without breaking it? I would assume not...

[mthiesse, 2017-01-12 16:40:59]

Patchset #4 (id:60001) has been deleted

[mthiesse, 2017-01-12 20:58:52]

Description was changed from

==========
Remove Sync GetPose VRService call, impelement VRVSyncProvider

This CL removes the Synchronous GetPose call from the VRService, in favor of a
VRVSyncProvider that sends the pose with each VSync.

BUG=655722
==========

to

==========
Remove Sync GetPose VRService call, implement VRVSyncProvider

This CL removes the Synchronous GetPose call from the VRService, in favor of a
VRVSyncProvider that sends the pose with each VSync.

BUG=655722
==========

[mthiesse, 2017-01-13 15:49:07]

Ping bajones, klausw, PTAL

[bajones, 2017-01-13 18:55:53]

https://codereview.chromium.org/2624633002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2624633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:177: ConnectVSyncProvider();
Calling this unconditionally would begin the vsync loop even if the page has
never called VRDisplay.rAF. Let's only do it if there are pending rAF calls.

https://codereview.chromium.org/2624633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:612: if
(m_vrVSyncProvider.is_bound()) {
Are there any ill effects if we stop polling? If not, doesn't it make sense to
have this request for the next vsync callback go in
VRDisplay.requestAnimationFrame? That way if the page stops polling the
VRDisplay's vsync loop won't pump forever in the background.

[mthiesse, 2017-01-13 20:08:54]

https://codereview.chromium.org/2624633002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2624633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:177: ConnectVSyncProvider();
On 2017/01/13 18:55:53, bajones wrote:
> Calling this unconditionally would begin the vsync loop even if the page has
> never called VRDisplay.rAF. Let's only do it if there are pending rAF calls.

Done.

https://codereview.chromium.org/2624633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:612: if
(m_vrVSyncProvider.is_bound()) {
On 2017/01/13 18:55:53, bajones wrote:
> Are there any ill effects if we stop polling? If not, doesn't it make sense to
> have this request for the next vsync callback go in
> VRDisplay.requestAnimationFrame? That way if the page stops polling the
> VRDisplay's vsync loop won't pump forever in the background.

Done. No harm stopping polling.

[klausw, 2017-01-13 21:04:36]

LGTM apart from the increment issue.

https://codereview.chromium.org/2624633002/diff/120001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2624633002/diff/120001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:487: // Increment pose frame counter
always, even if it's a faked pose.
Comment doesn't match code, I don't see any incrementing happening here.

[bajones, 2017-01-13 23:29:12]

LGTM with klausw@'s nit fixed.

[dcheng, 2017-01-14 11:22:53]

https://codereview.chromium.org/2624633002/diff/20001/device/vr/vr_service.mojom
File device/vr/vr_service.mojom (right):

https://codereview.chromium.org/2624633002/diff/20001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:93: GetVRVSyncProvider(VRVSyncProvider& request);
On 2017/01/12 15:49:03, mthiesse wrote:
> On 2017/01/12 09:56:56, dcheng wrote:
> > Out of curiosity, why a separate interface?
> 
> We bind to the VRVSyncProvider on a separate thread with a separate task
runner
> from the VRDisplay (sometimes).
> 
> There are two VRVSyncProvider implementations, the non-presenting
implementation
> that lives in the UI thread, and the presenting implementation that lives in
the
> GL thread (both live in the browser process though).
> 
> Let me know if I'm missing an obviously better approach.

The one thing to watch out for is there won't be any ordering guarantees between
VRDisplay and VRSyncProvider (unless you make them associated). I'm not familiar
enough with WebVR to say if that's a requirement here.

https://codereview.chromium.org/2624633002/diff/20001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:97: GetVSync() => (VRPose? pose, double
time_seconds);
On 2017/01/12 15:49:03, mthiesse wrote:
> On 2017/01/12 09:56:56, dcheng wrote:
> > Is it possible to use mojo.common.mojom.TimeTicks here?
> > 
> > (I think we might need to add a Blink typemap though...)
> 
> Would using TimeDelta be okay? I should probably add a comment explaining
this,
> but the code currently assumes that the VSync provider and the blink have
> different timebases (whether or not they do in practice I don't actually
know),
> so time_seconds is just treated as a delta from the first VSync received.
> 
> 
> Alternatively, I could make sure the timebases are the same and use
TimeTicks...
> Are WTF::monotonicallyIncreasingTime() and base::Timeticks::Now guaranteed to
be
> the same?

I'm honestly not sure. I know CC sends around TimeTicks, but I don't know  how
these TimeTicks are used...

Given that we're using this as a delta, TimeDelta seems fine. We can
independently figure out the TimeTicks issue.

https://codereview.chromium.org/2624633002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2624633002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:646: 100);  // Short delay to
throttle requests.
On 2017/01/12 15:49:03, mthiesse wrote:
> On 2017/01/12 09:56:56, dcheng wrote:
> > My understanding is the impl side is hosted in the browser process. If this
> pipe
> > was ever unexpectedly disconnected, that would indicate the browser process
is
> > shutting down (in which case this would never succeed). Is it necessary to
try
> > to reconnect?
> 
> No, this pipe shutting down doesn't indicate the browser process is shutting
> down. It indicates that we're switching from one VRVSyncProvider to another.
> Rather than send a new message saying "Hey, your VSync provider is gone but
> another one is available", we use the disconnected error to signal this. It's
> assumed that there's always a VRVSyncProvider available.
> 
> Is there a cleaner approach here? Can we move a binding to another thread
> without breaking it? I would assume not...

You can use mojo::Binding<T>::Unbind() to get an InterfaceRequest which can be
passed around and bound somewhere else.

https://codereview.chromium.org/2624633002/diff/120001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2624633002/diff/120001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:457: GLThread* thread =
static_cast<GLThread*>(gl_thread_.get());
Can gl_thread_ just be std::unique_ptr<GLThread>? It'd be nice not to have these
downcasts.

https://codereview.chromium.org/2624633002/diff/120001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2624633002/diff/120001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:959: CHECK(callback_.is_null());
As this is invoked by the renderer, it's unsafe to CHECK. This should call
mojo::ReportBadMessage when this condition fails instead.

[haraken, 2017-01-15 23:55:04]

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

[haraken, 2017-01-15 23:55:05]

https://codereview.chromium.org/2624633002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2624633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:646: 100);  // Short delay to
throttle requests.

Won't this have a risk of causing a re-connection flood if something is wrong
with the system and the disconnection happens repeatedly?

[mthiesse, 2017-01-16 20:52:57]

Thanks all, excellent feedback.

https://codereview.chromium.org/2624633002/diff/20001/device/vr/vr_service.mojom
File device/vr/vr_service.mojom (right):

https://codereview.chromium.org/2624633002/diff/20001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:93: GetVRVSyncProvider(VRVSyncProvider& request);
On 2017/01/14 11:22:52, dcheng wrote:
> On 2017/01/12 15:49:03, mthiesse wrote:
> > On 2017/01/12 09:56:56, dcheng wrote:
> > > Out of curiosity, why a separate interface?
> > 
> > We bind to the VRVSyncProvider on a separate thread with a separate task
> runner
> > from the VRDisplay (sometimes).
> > 
> > There are two VRVSyncProvider implementations, the non-presenting
> implementation
> > that lives in the UI thread, and the presenting implementation that lives in
> the
> > GL thread (both live in the browser process though).
> > 
> > Let me know if I'm missing an obviously better approach.
> 
> The one thing to watch out for is there won't be any ordering guarantees
between
> VRDisplay and VRSyncProvider (unless you make them associated). I'm not
familiar
> enough with WebVR to say if that's a requirement here.

Oh, yes I was careful to make sure ordering isn't an issue.

https://codereview.chromium.org/2624633002/diff/20001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:97: GetVSync() => (VRPose? pose, double
time_seconds);
On 2017/01/14 11:22:52, dcheng wrote:
> On 2017/01/12 15:49:03, mthiesse wrote:
> > On 2017/01/12 09:56:56, dcheng wrote:
> > > Is it possible to use mojo.common.mojom.TimeTicks here?
> > > 
> > > (I think we might need to add a Blink typemap though...)
> > 
> > Would using TimeDelta be okay? I should probably add a comment explaining
> this,
> > but the code currently assumes that the VSync provider and the blink have
> > different timebases (whether or not they do in practice I don't actually
> know),
> > so time_seconds is just treated as a delta from the first VSync received.
> > 
> > 
> > Alternatively, I could make sure the timebases are the same and use
> TimeTicks...
> > Are WTF::monotonicallyIncreasingTime() and base::Timeticks::Now guaranteed
to
> be
> > the same?
> 
> I'm honestly not sure. I know CC sends around TimeTicks, but I don't know  how
> these TimeTicks are used...
> 
> Given that we're using this as a delta, TimeDelta seems fine. We can
> independently figure out the TimeTicks issue.

Done?

Sidenote, figuring out I had to include "//mojo/common:common_custom_types", to
the mojo deps was really confusing, because it generated mojom.h files without
complaint, but the callback asked for a  mojo::common::mojom::TimeDeltaPtr
instead of a base::TimeDelta, and I wasn't able to construct a TimeDeltaPtr.
Very surprising behavior that changing the deps changed the generated code.

https://codereview.chromium.org/2624633002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2624633002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:646: 100);  // Short delay to
throttle requests.
On 2017/01/14 11:22:53, dcheng wrote:
> On 2017/01/12 15:49:03, mthiesse wrote:
> > On 2017/01/12 09:56:56, dcheng wrote:
> > > My understanding is the impl side is hosted in the browser process. If
this
> > pipe
> > > was ever unexpectedly disconnected, that would indicate the browser
process
> is
> > > shutting down (in which case this would never succeed). Is it necessary to
> try
> > > to reconnect?
> > 
> > No, this pipe shutting down doesn't indicate the browser process is shutting
> > down. It indicates that we're switching from one VRVSyncProvider to another.
> > Rather than send a new message saying "Hey, your VSync provider is gone but
> > another one is available", we use the disconnected error to signal this.
It's
> > assumed that there's always a VRVSyncProvider available.
> > 
> > Is there a cleaner approach here? Can we move a binding to another thread
> > without breaking it? I would assume not...
> 
> You can use mojo::Binding<T>::Unbind() to get an InterfaceRequest which can be
> passed around and bound somewhere else.

Ah thanks, that's exactly what I was looking for and makes things so much
cleaner.

https://codereview.chromium.org/2624633002/diff/120001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2624633002/diff/120001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:457: GLThread* thread =
static_cast<GLThread*>(gl_thread_.get());
On 2017/01/14 11:22:53, dcheng wrote:
> Can gl_thread_ just be std::unique_ptr<GLThread>? It'd be nice not to have
these
> downcasts.

Done, but this required moving GLThread to its own header/cc file. Apparently
forward declaring from the header into an anonymous namespace in the cc file is
impossible.

https://codereview.chromium.org/2624633002/diff/120001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:487: // Increment pose frame counter
always, even if it's a faked pose.
On 2017/01/13 21:04:36, klausw wrote:
> Comment doesn't match code, I don't see any incrementing happening here.

Done.

https://codereview.chromium.org/2624633002/diff/120001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2624633002/diff/120001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:959: CHECK(callback_.is_null());
On 2017/01/14 11:22:53, dcheng wrote:
> As this is invoked by the renderer, it's unsafe to CHECK. This should call
> mojo::ReportBadMessage when this condition fails instead.

Done.

https://codereview.chromium.org/2624633002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2624633002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:646: 100);  // Short delay to
throttle requests.
On 2017/01/15 23:55:04, haraken wrote:
> 
> Won't this have a risk of causing a re-connection flood if something is wrong
> with the system and the disconnection happens repeatedly?
> 
Yes, removed thanks to dcheng's suggestion.

[dcheng, 2017-01-17 23:01:20]

It seems like the latest PS is missing; do you mind re-uploading?

[mthiesse, 2017-01-17 23:25:27]

On 2017/01/17 23:01:20, dcheng wrote:
> It seems like the latest PS is missing; do you mind re-uploading?

Whoops! Sorry about that :P Uploaded now.

[dcheng, 2017-01-18 23:46:06]

https://codereview.chromium.org/2624633002/diff/160001/chrome/browser/android...
File chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc (right):

https://codereview.chromium.org/2624633002/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc:25:
binding_.Close();
Nit: Close() already checks this, so just unconditionally Close(). Similar
comment for most other is_bound() checks.

(It's also unclear to me why this needs to explicitly call Close; shouldn't the
dtor automatically do that?)

https://codereview.chromium.org/2624633002/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc:66: paused_ =
false;
Nit: minor comment about why paused_ = false is correct here would be nice
(Pause() sets it to true, so it's unclear why this should be the opposite)

https://codereview.chromium.org/2624633002/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc:114: callback_ =
std::move(callback);
Nit: |callback| is a const ref, so std::move() doesn't do anything.

https://codereview.chromium.org/2624633002/diff/160001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_gl_thread.cc (right):

https://codereview.chromium.org/2624633002/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_gl_thread.cc:27:
main_thread_task_runner_(std::move(main_thread_task_runner)),
Nit: #include <utility>

https://codereview.chromium.org/2624633002/diff/160001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_gl_thread.h (right):

https://codereview.chromium.org/2624633002/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_gl_thread.h:48: base::WeakPtr<VrShell>
weak_vr_shell_;
Nit: maybe note that this state is used for initializing VrShellGl, or something
along those lines.

https://codereview.chromium.org/2624633002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.h (right):

https://codereview.chromium.org/2624633002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.h:135:
mojo::common::mojom::blink::TimeDeltaPtr);
Doh. Let me add a typemap for the Blink side... (I'll clean this up if it lands
after your CL).

[mthiesse, 2017-01-19 00:19:05]

https://codereview.chromium.org/2624633002/diff/160001/chrome/browser/android...
File chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc (right):

https://codereview.chromium.org/2624633002/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc:25:
binding_.Close();
On 2017/01/18 23:46:06, dcheng wrote:
> Nit: Close() already checks this, so just unconditionally Close(). Similar
> comment for most other is_bound() checks.
> 
> (It's also unclear to me why this needs to explicitly call Close; shouldn't
the
> dtor automatically do that?)

haha it probably does, I was erring a little too far on the side of caution.
Done.

https://codereview.chromium.org/2624633002/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc:66: paused_ =
false;
On 2017/01/18 23:46:06, dcheng wrote:
> Nit: minor comment about why paused_ = false is correct here would be nice
> (Pause() sets it to true, so it's unclear why this should be the opposite)

Renamed and added clarifying comment.

https://codereview.chromium.org/2624633002/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc:114: callback_ =
std::move(callback);
On 2017/01/18 23:46:06, dcheng wrote:
> Nit: |callback| is a const ref, so std::move() doesn't do anything.

Done.

https://codereview.chromium.org/2624633002/diff/160001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_gl_thread.cc (right):

https://codereview.chromium.org/2624633002/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_gl_thread.cc:27:
main_thread_task_runner_(std::move(main_thread_task_runner)),
On 2017/01/18 23:46:06, dcheng wrote:
> Nit: #include <utility>

Done.

https://codereview.chromium.org/2624633002/diff/160001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_gl_thread.h (right):

https://codereview.chromium.org/2624633002/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_gl_thread.h:48: base::WeakPtr<VrShell>
weak_vr_shell_;
On 2017/01/18 23:46:06, dcheng wrote:
> Nit: maybe note that this state is used for initializing VrShellGl, or
something
> along those lines.

Done.

https://codereview.chromium.org/2624633002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.h (right):

https://codereview.chromium.org/2624633002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.h:135:
mojo::common::mojom::blink::TimeDeltaPtr);
On 2017/01/18 23:46:06, dcheng wrote:
> Doh. Let me add a typemap for the Blink side... (I'll clean this up if it
lands
> after your CL).

Acknowledged.

[dcheng, 2017-01-19 00:25:23]

mojo lgtm

sorry for the long review latency

[mthiesse, 2017-01-19 00:38:46]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-01-19 00:38:47]

The patchset sent to the CQ was uploaded after l-g-t-m from klausw@chromium.org,
bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/2624633002/#ps200001
(title: "Address comments")

[commit-bot: I haz the power, 2017-01-19 00:39:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mthiesse, 2017-01-19 00:42:56]

The CQ bit was unchecked by mthiesse@chromium.org

[mthiesse, 2017-01-19 00:48:07]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2017-01-19 00:48:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2017-01-19 00:49:56]

LGTM

https://codereview.chromium.org/2624633002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2624633002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:622: if (pose.is_null() &&
timeDelta == WTF::TimeDelta()) {

Just help me understand: What does 'timeDelta == WTF::TimeDelta()' mean?

https://codereview.chromium.org/2624633002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.h (right):

https://codereview.chromium.org/2624633002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.h:10: #include
"core/dom/TaskRunnerHelper.h"

Not needed?

[mthiesse, 2017-01-19 00:51:15]

The CQ bit was unchecked by mthiesse@chromium.org

[mthiesse, 2017-01-19 00:54:04]

https://codereview.chromium.org/2624633002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2624633002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:622: if (pose.is_null() &&
timeDelta == WTF::TimeDelta()) {
On 2017/01/19 00:49:56, haraken wrote:
> 
> Just help me understand: What does 'timeDelta == WTF::TimeDelta()' mean?

This is checking that the timeDelta default initialized, or set to 0, which is
used as a signal that this isn't actually a vsync, but we were forced to respond
to the callback regardless and had to send *something*.

https://codereview.chromium.org/2624633002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.h (right):

https://codereview.chromium.org/2624633002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.h:10: #include
"core/dom/TaskRunnerHelper.h"
On 2017/01/19 00:49:56, haraken wrote:
> 
> Not needed?

Done.

[mthiesse, 2017-01-19 00:55:20]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-01-19 00:55:21]

The patchset sent to the CQ was uploaded after l-g-t-m from klausw@chromium.org,
haraken@chromium.org, bajones@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2624633002/#ps220001
(title: "Address comment")

[commit-bot: I haz the power, 2017-01-19 00:55:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 01:17:33]

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

[commit-bot: I haz the power, 2017-01-19 01:17:35]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mthiesse, 2017-01-19 01:26:22]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-01-19 01:26:22]

The patchset sent to the CQ was uploaded after l-g-t-m from klausw@chromium.org,
haraken@chromium.org, bajones@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2624633002/#ps240001
(title: "Fix compuile")

[commit-bot: I haz the power, 2017-01-19 01:26:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 02:01:33]

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

[commit-bot: I haz the power, 2017-01-19 02:01:34]

Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[mthiesse, 2017-01-19 21:37:46]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-01-19 21:37:46]

The patchset sent to the CQ was uploaded after l-g-t-m from klausw@chromium.org,
haraken@chromium.org, bajones@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2624633002/#ps260001
(title: "Fix tests")

[commit-bot: I haz the power, 2017-01-19 21:38:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-01-19 22:16:12]

https://codereview.chromium.org/2624633002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2624633002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:622: if (pose.is_null() &&
timeDelta == WTF::TimeDelta()) {
On 2017/01/19 00:54:04, mthiesse wrote:
> On 2017/01/19 00:49:56, haraken wrote:
> > 
> > Just help me understand: What does 'timeDelta == WTF::TimeDelta()' mean?
> 
> This is checking that the timeDelta default initialized, or set to 0, which is
> used as a signal that this isn't actually a vsync, but we were forced to
respond
> to the callback regardless and had to send *something*.

Can we write this as is_zero()?

[commit-bot: I haz the power, 2017-01-19 22:28:35]

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

[commit-bot: I haz the power, 2017-01-19 22:28:37]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[mthiesse, 2017-01-19 22:56:49]

https://codereview.chromium.org/2624633002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2624633002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:622: if (pose.is_null() &&
timeDelta == WTF::TimeDelta()) {
On 2017/01/19 22:16:11, dcheng wrote:
> On 2017/01/19 00:54:04, mthiesse wrote:
> > On 2017/01/19 00:49:56, haraken wrote:
> > > 
> > > Just help me understand: What does 'timeDelta == WTF::TimeDelta()' mean?
> > 
> > This is checking that the timeDelta default initialized, or set to 0, which
is
> > used as a signal that this isn't actually a vsync, but we were forced to
> respond
> > to the callback regardless and had to send *something*.
> 
> Can we write this as is_zero()?

Done.

[mthiesse, 2017-01-19 22:56:58]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-01-19 22:56:58]

The patchset sent to the CQ was uploaded after l-g-t-m from klausw@chromium.org,
haraken@chromium.org, bajones@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2624633002/#ps280001
(title: "Fix more compile errors")

[commit-bot: I haz the power, 2017-01-19 22:57:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 23:44:16]

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

[commit-bot: I haz the power, 2017-01-19 23:44:17]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[mthiesse, 2017-01-20 01:52:23]

dcheng@, don't spend too much time on this, but is the build failure here
obvious to you?
(https://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds...)

I can't figure out what about my CL could have caused this.

[mthiesse, 2017-01-20 16:02:05]

Description was changed from

==========
Remove Sync GetPose VRService call, implement VRVSyncProvider

This CL removes the Synchronous GetPose call from the VRService, in favor of a
VRVSyncProvider that sends the pose with each VSync.

BUG=655722
==========

to

==========
Remove Sync GetPose VRService call, implement VRVSyncProvider

This CL removes the Synchronous GetPose call from the VRService, in favor of a
VRVSyncProvider that sends the pose with each VSync.

BUG=655722
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[mthiesse, 2017-01-20 16:03:46]

Patchset #18 (id:350001) has been deleted

[mthiesse, 2017-01-20 17:49:13]

mthiesse@chromium.org changed reviewers:
+ brettw@chromium.org

[mthiesse, 2017-01-20 17:49:14]

brettw@chromium.org: Please review content/browser/BUILD.gn

[mthiesse, 2017-01-20 19:14:17]

The CQ bit was checked by mthiesse@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 19:14:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 19:19:46]

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

[commit-bot: I haz the power, 2017-01-20 19:19:48]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mthiesse, 2017-01-20 19:32:27]

The CQ bit was checked by mthiesse@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 19:33:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 21:33:03]

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

[commit-bot: I haz the power, 2017-01-20 21:33:05]

Dry run: This issue passed the CQ dry run.

[mthiesse, 2017-01-24 02:56:14]

On 2017/01/20 17:49:14, mthiesse wrote:
> mailto:brettw@chromium.org: Please review content/browser/BUILD.gn

ping brettw@, PTAL

[mthiesse, 2017-01-24 02:56:19]

On 2017/01/20 17:49:14, mthiesse wrote:
> mailto:brettw@chromium.org: Please review content/browser/BUILD.gn

ping brettw, PTAL

[mthiesse, 2017-01-25 00:25:48]

Description was changed from

==========
Remove Sync GetPose VRService call, implement VRVSyncProvider

This CL removes the Synchronous GetPose call from the VRService, in favor of a
VRVSyncProvider that sends the pose with each VSync.

BUG=655722
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove Sync GetPose VRService call, implement VRVSyncProvider

This CL removes the Synchronous GetPose call from the VRService, in favor of a
VRVSyncProvider that sends the pose with each VSync.

BUG=655722
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[mthiesse, 2017-01-25 00:25:48]

mthiesse@chromium.org changed reviewers:
- brettw@chromium.org

[mthiesse, 2017-01-25 00:28:04]

mthiesse@chromium.org changed reviewers:
+ clamy@chromium.org

[mthiesse, 2017-01-25 00:28:05]

clamy@chromium.org: Please review content/browser/BUILD.gn

(-brettw in favor of a more specific owner)

[clamy, 2017-01-25 15:19:48]

https://codereview.chromium.org/2624633002/diff/440001/content/browser/BUILD.gn
File content/browser/BUILD.gn (left):

https://codereview.chromium.org/2624633002/diff/440001/content/browser/BUILD....
content/browser/BUILD.gn:1997: if (enable_webvr) {
If I understand correctly this means we'll always build vr by default, right?
Why the change?

[mthiesse, 2017-01-25 15:36:13]

https://codereview.chromium.org/2624633002/diff/440001/content/browser/BUILD.gn
File content/browser/BUILD.gn (left):

https://codereview.chromium.org/2624633002/diff/440001/content/browser/BUILD....
content/browser/BUILD.gn:1997: if (enable_webvr) {
On 2017/01/25 15:19:47, clamy wrote:
> If I understand correctly this means we'll always build vr by default, right?
> Why the change?

To be honest, I'm really not sure how this managed to link at all before my
change here, as render_frame_host_impl includes device/vr/vr_service_impl.h
unconditionally.

The change to device/vr/BUILD.gn that adds DEVICE_VR_EXPORT to the VRService
mojo files somehow exposed this and prevented render_frame_host_impl from
linking.

Most of VR still won't be built by default, this just builds the
platform-independent mojo bindings by default.

[mthiesse, 2017-01-25 15:57:20]

The CQ bit was checked by mthiesse@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 15:57:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 16:11:22]

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

[commit-bot: I haz the power, 2017-01-25 16:11:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mthiesse, 2017-01-25 17:42:02]

Patchset #20 (id:410001) has been deleted

[mthiesse, 2017-01-25 17:42:21]

Patchset #16 (id:300002) has been deleted

[mthiesse, 2017-01-25 17:42:35]

Patchset #17 (id:370001) has been deleted

[mthiesse, 2017-01-25 19:01:35]

clamy, how does the latest patchset look? I've added a new target rather than
building more of the vr code than is strictly necessary.

[clamy, 2017-01-26 17:48:30]

Thanks! Lgtm.

[mthiesse, 2017-01-26 18:43:03]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-01-26 18:43:03]

The patchset sent to the CQ was uploaded after l-g-t-m from klausw@chromium.org,
haraken@chromium.org, bajones@chromium.org, dcheng@chromium.org,
clamy@chromium.org
Link to the patchset: https://codereview.chromium.org/2624633002/#ps520001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-26 18:43:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 18:49:11]

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

[commit-bot: I haz the power, 2017-01-26 18:49:14]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[mthiesse, 2017-01-26 18:56:44]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-01-26 18:56:45]

The patchset sent to the CQ was uploaded after l-g-t-m from klausw@chromium.org,
haraken@chromium.org, clamy@chromium.org, bajones@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2624633002/#ps540001
(title: "Fix gn check")

[commit-bot: I haz the power, 2017-01-26 18:57:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 19:33:44]

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

[commit-bot: I haz the power, 2017-01-26 19:33:46]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mthiesse, 2017-01-26 19:46:19]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-01-26 19:46:20]

The patchset sent to the CQ was uploaded after l-g-t-m from klausw@chromium.org,
haraken@chromium.org, clamy@chromium.org, bajones@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2624633002/#ps560001
(title: "ugh why")

[commit-bot: I haz the power, 2017-01-26 19:46:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 20:08:06]

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

[commit-bot: I haz the power, 2017-01-26 20:08:08]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mthiesse, 2017-01-26 20:25:37]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-01-26 20:25:38]

The patchset sent to the CQ was uploaded after l-g-t-m from klausw@chromium.org,
haraken@chromium.org, clamy@chromium.org, bajones@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2624633002/#ps580001
(title: "Use source set?")

[commit-bot: I haz the power, 2017-01-26 20:26:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mthiesse, 2017-01-26 21:10:55]

The CQ bit was checked by mthiesse@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 21:11:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mthiesse, 2017-01-26 21:15:44]

Patchset #27 (id:600001) has been deleted

[commit-bot: I haz the power, 2017-01-26 21:16:25]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[mthiesse, 2017-01-26 21:16:54]

The CQ bit was checked by mthiesse@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 21:17:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mthiesse, 2017-01-26 22:02:05]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-01-26 22:02:05]

The patchset sent to the CQ was uploaded after l-g-t-m from klausw@chromium.org,
haraken@chromium.org, clamy@chromium.org, bajones@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2624633002/#ps640001
(title: "Cleanup")

[commit-bot: I haz the power, 2017-01-26 22:03:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 22:08:42]

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

[commit-bot: I haz the power, 2017-01-26 22:08:45]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mthiesse, 2017-01-26 22:16:38]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-01-26 22:16:39]

The patchset sent to the CQ was uploaded after l-g-t-m from klausw@chromium.org,
haraken@chromium.org, clamy@chromium.org, bajones@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2624633002/#ps660001
(title: "oops")

[commit-bot: I haz the power, 2017-01-26 22:17:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 02:08:05]

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

[commit-bot: I haz the power, 2017-01-27 02:08:07]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mthiesse, 2017-01-27 02:52:13]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2017-01-27 02:53:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 03:58:21]

CQ is committing da patch.

Bot data: {"patchset_id": 660001, "attempt_start_ts": 1485485533939290,
"parent_rev": "f542e50ddb59cb5bbb3d818e2a68f633f171149e", "commit_rev":
"a316edeb21fbc8c122dbe64232c72c574155ed1b"}

[commit-bot: I haz the power, 2017-01-27 03:58:55]

Description was changed from

==========
Remove Sync GetPose VRService call, implement VRVSyncProvider

This CL removes the Synchronous GetPose call from the VRService, in favor of a
VRVSyncProvider that sends the pose with each VSync.

BUG=655722
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove Sync GetPose VRService call, implement VRVSyncProvider

This CL removes the Synchronous GetPose call from the VRService, in favor of a
VRVSyncProvider that sends the pose with each VSync.

BUG=655722
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2624633002
Cr-Commit-Position: refs/heads/master@{#446581}
Committed:
https://chromium.googlesource.com/chromium/src/+/a316edeb21fbc8c122dbe64232c7...
==========

[commit-bot: I haz the power, 2017-01-27 03:58:57]

Committed patchset #29 (id:660001) as
https://chromium.googlesource.com/chromium/src/+/a316edeb21fbc8c122dbe64232c7...