Allow VRDisplay to specify which frame the layer bounds should be updated at.

Allow VRDisplay to specify which frame the layer bounds should be updated at.

BUG=655722, 670891
Review-Url: https://codereview.chromium.org/2612333002
Cr-Commit-Position: refs/heads/master@{#446707}
Committed: https://chromium.googlesource.com/chromium/src/+/961dc47e8f5b088debbab559fe81b85f32e9f9e4

[mthiesse, 2017-01-05 20:15:59]

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

[mthiesse, 2017-01-05 20:15:59]

PTAL, no functional changes yet, just paving the way for them.

[mthiesse, 2017-01-05 20:32:09]

Description was changed from

==========
Allow VRDisplay to specify which frame the layer bounds should be updated at.

BUG=678721
==========

to

==========
Allow VRDisplay to specify which frame the layer bounds should be updated at.

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

[klausw, 2017-01-05 20:40:31]

On 2017/01/05 20:15:59, mthiesse wrote:
> PTAL, no functional changes yet, just paving the way for them.

Please also add:

https://bugs.chromium.org/p/chromium/issues/detail?id=670891 "WebVR
dynamic-resolution sample page is not displayed correctly in presentation mode"

[mthiesse, 2017-01-05 20:42:35]

Description was changed from

==========
Allow VRDisplay to specify which frame the layer bounds should be updated at.

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

to

==========
Allow VRDisplay to specify which frame the layer bounds should be updated at.

BUG=655722,670891
==========

[mthiesse, 2017-01-05 20:42:55]

On 2017/01/05 20:40:31, klausw wrote:
> On 2017/01/05 20:15:59, mthiesse wrote:
> > PTAL, no functional changes yet, just paving the way for them.
> 
> Please also add:
> 
> https://bugs.chromium.org/p/chromium/issues/detail?id=670891 "WebVR
> dynamic-resolution sample page is not displayed correctly in presentation
mode"

done

[klausw, 2017-01-05 20:44:06]

On 2017/01/05 20:42:55, mthiesse wrote:
> On 2017/01/05 20:40:31, klausw wrote:
> > On 2017/01/05 20:15:59, mthiesse wrote:
> > > PTAL, no functional changes yet, just paving the way for them.
> > 
> > Please also add:
> > 
> > https://bugs.chromium.org/p/chromium/issues/detail?id=670891 "WebVR
> > dynamic-resolution sample page is not displayed correctly in presentation
> mode"
> 
> done

LGTM

[mthiesse, 2017-01-05 20:56:58]

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

[bajones, 2017-01-05 20:56:58]

Plumbing approved! LGTM.

[mthiesse, 2017-01-05 20:56:58]

dcheng@, please review .mojom changes.

[dcheng, 2017-01-10 06:53:19]

https://codereview.chromium.org/2612333002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2612333002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:941: // |for_frame_index|.
Sorry for missing this in my review queue. Is there any way to upstream the IPC
changes with the complete implementation changes?

[klausw, 2017-01-10 08:10:56]

FYI, I have a rough implementation for the bounds change in the WIP change
https://codereview.chromium.org/2584343002/diff/60001/chrome/browser/android/...
, see around lines 1128 and 1176.

On Jan 9, 2017 22:53, <dcheng@chromium.org> wrote:

>
> https://codereview.chromium.org/2612333002/diff/1/chrome/
> browser/android/vr_shell/vr_shell_gl.cc
> File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):
>
> https://codereview.chromium.org/2612333002/diff/1/chrome/
> browser/android/vr_shell/vr_shell_gl.cc#newcode941
> chrome/browser/android/vr_shell/vr_shell_gl.cc:941: //
> |for_frame_index|.
> Sorry for missing this in my review queue. Is there any way to upstream
> the IPC changes with the complete implementation changes?
>
> https://codereview.chromium.org/2612333002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[klausw, 2017-01-10 08:10:56]

FYI, I have a rough implementation for the bounds change in the WIP change
https://codereview.chromium.org/2584343002/diff/60001/chrome/browser/android/...
, see around lines 1128 and 1176.

On Jan 9, 2017 22:53, <dcheng@chromium.org> wrote:

>
> https://codereview.chromium.org/2612333002/diff/1/chrome/
> browser/android/vr_shell/vr_shell_gl.cc
> File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):
>
> https://codereview.chromium.org/2612333002/diff/1/chrome/
> browser/android/vr_shell/vr_shell_gl.cc#newcode941
> chrome/browser/android/vr_shell/vr_shell_gl.cc:941: //
> |for_frame_index|.
> Sorry for missing this in my review queue. Is there any way to upstream
> the IPC changes with the complete implementation changes?
>
> https://codereview.chromium.org/2612333002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dcheng, 2017-01-11 22:48:34]

On 2017/01/10 08:10:56, klausw wrote:
> FYI, I have a rough implementation for the bounds change in the WIP change
>
https://codereview.chromium.org/2584343002/diff/60001/chrome/browser/android/...
> , see around lines 1128 and 1176.

The patch looks straightforward, but from a security review perspective, it's
still preferable to review them as one whole unit. This CL itself is pretty
small, and shouldn't add a big burden to the other patch. WDYT?

> 
> On Jan 9, 2017 22:53, <mailto:dcheng@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/2612333002/diff/1/chrome/
> > browser/android/vr_shell/vr_shell_gl.cc
> > File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):
> >
> > https://codereview.chromium.org/2612333002/diff/1/chrome/
> > browser/android/vr_shell/vr_shell_gl.cc#newcode941
> > chrome/browser/android/vr_shell/vr_shell_gl.cc:941: //
> > |for_frame_index|.
> > Sorry for missing this in my review queue. Is there any way to upstream
> > the IPC changes with the complete implementation changes?
> >
> > https://codereview.chromium.org/2612333002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Blink
> Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

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

On 2017/01/11 22:48:34, dcheng wrote:
> On 2017/01/10 08:10:56, klausw wrote:
> > FYI, I have a rough implementation for the bounds change in the WIP change
> >
>
https://codereview.chromium.org/2584343002/diff/60001/chrome/browser/android/...
> > , see around lines 1128 and 1176.
> 
> The patch looks straightforward, but from a security review perspective, it's
> still preferable to review them as one whole unit. This CL itself is pretty
> small, and shouldn't add a big burden to the other patch. WDYT?
> 
> > 
> > On Jan 9, 2017 22:53, <mailto:dcheng@chromium.org> wrote:
> > 
> > >
> > > https://codereview.chromium.org/2612333002/diff/1/chrome/
> > > browser/android/vr_shell/vr_shell_gl.cc
> > > File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):
> > >
> > > https://codereview.chromium.org/2612333002/diff/1/chrome/
> > > browser/android/vr_shell/vr_shell_gl.cc#newcode941
> > > chrome/browser/android/vr_shell/vr_shell_gl.cc:941: //
> > > |for_frame_index|.
> > > Sorry for missing this in my review queue. Is there any way to upstream
> > > the IPC changes with the complete implementation changes?
> > >
> > > https://codereview.chromium.org/2612333002/
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> "Blink
> > Reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:blink-reviews+unsubscribe@chromium.org.

Fine by me, I'll add the relevant bits of the implementation to this CL and ping
when it's ready.

[mthiesse, 2017-01-12 20:13:03]

On 2017/01/11 22:50:41, mthiesse wrote:
> On 2017/01/11 22:48:34, dcheng wrote:
> > On 2017/01/10 08:10:56, klausw wrote:
> > > FYI, I have a rough implementation for the bounds change in the WIP change
> > >
> >
>
https://codereview.chromium.org/2584343002/diff/60001/chrome/browser/android/...
> > > , see around lines 1128 and 1176.
> > 
> > The patch looks straightforward, but from a security review perspective,
it's
> > still preferable to review them as one whole unit. This CL itself is pretty
> > small, and shouldn't add a big burden to the other patch. WDYT?
> > 
> > > 
> > > On Jan 9, 2017 22:53, <mailto:dcheng@chromium.org> wrote:
> > > 
> > > >
> > > > https://codereview.chromium.org/2612333002/diff/1/chrome/
> > > > browser/android/vr_shell/vr_shell_gl.cc
> > > > File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):
> > > >
> > > > https://codereview.chromium.org/2612333002/diff/1/chrome/
> > > > browser/android/vr_shell/vr_shell_gl.cc#newcode941
> > > > chrome/browser/android/vr_shell/vr_shell_gl.cc:941: //
> > > > |for_frame_index|.
> > > > Sorry for missing this in my review queue. Is there any way to upstream
> > > > the IPC changes with the complete implementation changes?
> > > >
> > > > https://codereview.chromium.org/2612333002/
> > > >
> > > 
> > > -- 
> > > You received this message because you are subscribed to the Google Groups
> > "Blink
> > > Reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:blink-reviews+unsubscribe@chromium.org.
> 
> Fine by me, I'll add the relevant bits of the implementation to this CL and
ping
> when it's ready.

Alright, everything's in place, PTAL.

[dcheng, 2017-01-14 11:17:01]

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

https://codereview.chromium.org/2612333002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:983: CHECK(callback_.is_null());
Since this is getting called by the renderer, we can't CHECK(). An alternatively
is to call mojo::ReportBadMessage if this condition is true.

[mthiesse, 2017-01-16 22:21:32]

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

https://codereview.chromium.org/2612333002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:983: CHECK(callback_.is_null());
On 2017/01/14 11:17:01, dcheng wrote:
> Since this is getting called by the renderer, we can't CHECK(). An
alternatively
> is to call mojo::ReportBadMessage if this condition is true.

Fixed in the CL this depends on.

[mthiesse, 2017-01-17 23:10:35]

Patchset #4 (id:60001) has been deleted

[dcheng, 2017-01-18 23:58:08]

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

https://codereview.chromium.org/2612333002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc:117:
SendVSync(pending_time_, std::move(callback));
Nit: std::move() here is a no-op

https://codereview.chromium.org/2612333002/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/non_presenting_gvr_delegate.h (right):

https://codereview.chromium.org/2612333002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/non_presenting_gvr_delegate.h:50: void
SendVSync(const base::TimeDelta& time, GetVSyncCallback callback);
Nit: pass by value. It's also convention to pass copyable callbacks by const ref
generally -- is there a reason to do otherwise here?

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

https://codereview.chromium.org/2612333002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:89: static constexpr int
kPoseRingBufferSize = 8;
Nit: unsigned?

https://codereview.chromium.org/2612333002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:636: (int)
std::numeric_limits<typeof(frame_index_)>::max();
Nit: let's be consistent and not mix signed/unsigned. I think this can all be
unsigned.

Also, let's use decltype (which is standard C++11) rather than typeof (which is
a GCC extension)

https://codereview.chromium.org/2612333002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1007: SendVSync(pending_time_,
std::move(callback));
Same comment about std::move() on a const ref being a no-op

https://codereview.chromium.org/2612333002/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell_gl.h (right):

https://codereview.chromium.org/2612333002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.h:127: void SendVSync(const
base::TimeDelta& time, GetVSyncCallback callback);
Ditto: pass TimeDelta by value

https://codereview.chromium.org/2612333002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.h:200: int16_t last_frame_index_ =
0;
Some comments here would be helpful. I'm not an expert in this code, so I don't
quite understand why one is uint8_t, while another field which is similarly
named is int16_t. It could just be a naming issue as well?

[mthiesse, 2017-01-19 01:17:45]

Patchset #6 (id:120001) has been deleted

[mthiesse, 2017-01-19 01:19:09]

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

https://codereview.chromium.org/2612333002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc:117:
SendVSync(pending_time_, std::move(callback));
On 2017/01/18 23:58:07, dcheng wrote:
> Nit: std::move() here is a no-op

Done.

https://codereview.chromium.org/2612333002/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/non_presenting_gvr_delegate.h (right):

https://codereview.chromium.org/2612333002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/non_presenting_gvr_delegate.h:50: void
SendVSync(const base::TimeDelta& time, GetVSyncCallback callback);
On 2017/01/18 23:58:07, dcheng wrote:
> Nit: pass by value. It's also convention to pass copyable callbacks by const
ref
> generally -- is there a reason to do otherwise here?

Done.

The reason I wasn't passing by const ref is so that I could move callback_ and
not have to explicitly Reset() it. Doesn't matter to me either way.

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

https://codereview.chromium.org/2612333002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:89: static constexpr int
kPoseRingBufferSize = 8;
On 2017/01/18 23:58:07, dcheng wrote:
> Nit: unsigned?

Done.

https://codereview.chromium.org/2612333002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:636: (int)
std::numeric_limits<typeof(frame_index_)>::max();
On 2017/01/18 23:58:07, dcheng wrote:
> Nit: let's be consistent and not mix signed/unsigned. I think this can all be
> unsigned.
> 
> Also, let's use decltype (which is standard C++11) rather than typeof (which
is
> a GCC extension)

Done.

https://codereview.chromium.org/2612333002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1007: SendVSync(pending_time_,
std::move(callback));
On 2017/01/18 23:58:07, dcheng wrote:
> Same comment about std::move() on a const ref being a no-op

Done.

https://codereview.chromium.org/2612333002/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell_gl.h (right):

https://codereview.chromium.org/2612333002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.h:127: void SendVSync(const
base::TimeDelta& time, GetVSyncCallback callback);
On 2017/01/18 23:58:07, dcheng wrote:
> Ditto: pass TimeDelta by value

Done.

https://codereview.chromium.org/2612333002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.h:200: int16_t last_frame_index_ =
0;
On 2017/01/18 23:58:07, dcheng wrote:
> Some comments here would be helpful. I'm not an expert in this code, so I
don't
> quite understand why one is uint8_t, while another field which is similarly
> named is int16_t. It could just be a naming issue as well?

I did this so it could be initialized to -1... and then didn't initialize it to
-1.

In practice it doesn't really matter, but to be correct it shouldn't be
initialized to an in-band value of frame_index_.

Added a comment.

[mthiesse, 2017-01-19 01:24:57]

Patchset #7 (id:160001) has been deleted

[klausw (use chromium instead), 2017-01-19 01:34:26]

klausw@google.com changed reviewers:
+ klausw@google.com

[klausw (use chromium instead), 2017-01-19 01:34:27]

LGTM apart from type nit.

https://codereview.chromium.org/2612333002/diff/140001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell_gl.h (right):

https://codereview.chromium.org/2612333002/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.h:201: uint16_t last_frame_index_ =
-1;
Initializing unsigned integers to negative numbers seems odd. Assuming it just
needs to be big enough to store a frame index and -1, just make it a signed
int16_t?

[mthiesse, 2017-01-26 23:23:03]

https://codereview.chromium.org/2612333002/diff/140001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell_gl.h (right):

https://codereview.chromium.org/2612333002/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.h:201: uint16_t last_frame_index_ =
-1;
On 2017/01/19 01:34:27, klausw (use chromium instead) wrote:
> Initializing unsigned integers to negative numbers seems odd. Assuming it just
> needs to be big enough to store a frame index and -1, just make it a signed
> int16_t?

dcheng asked for all types to be unsigned, so we don't get a mismatch of signed
and unsigned. I tend to agree.

Initializing unsigned to -1 isn't very common, but it's not unheard of. See
https://cs.chromium.org/search/?q=uint.*%5C+%3D%5C+-1;&sq=package:chromium&ty...

[klausw, 2017-01-27 00:24:55]

https://codereview.chromium.org/2612333002/diff/140001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell_gl.h (right):

https://codereview.chromium.org/2612333002/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.h:201: uint16_t last_frame_index_ =
-1;
On 2017/01/26 23:23:03, mthiesse wrote:
> On 2017/01/19 01:34:27, klausw (use chromium instead) wrote:
> > Initializing unsigned integers to negative numbers seems odd. Assuming it
just
> > needs to be big enough to store a frame index and -1, just make it a signed
> > int16_t?
> 
> dcheng asked for all types to be unsigned, so we don't get a mismatch of
signed
> and unsigned. I tend to agree.
> 
> Initializing unsigned to -1 isn't very common, but it's not unheard of. See
>
https://cs.chromium.org/search/?q=uint.*%5C+%3D%5C+-1;&sq=package:chromium&ty...

How about UINT16_MAX from cstdint then? The -1 initialization seems like a
potential trap for someone doing a (last_frame_index >= 0) check somewhere,
though presumably the compiler would catch that.

[mthiesse, 2017-01-27 00:57:28]

https://codereview.chromium.org/2612333002/diff/140001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell_gl.h (right):

https://codereview.chromium.org/2612333002/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.h:201: uint16_t last_frame_index_ =
-1;
On 2017/01/27 00:24:55, klausw wrote:
> On 2017/01/26 23:23:03, mthiesse wrote:
> > On 2017/01/19 01:34:27, klausw (use chromium instead) wrote:
> > > Initializing unsigned integers to negative numbers seems odd. Assuming it
> just
> > > needs to be big enough to store a frame index and -1, just make it a
signed
> > > int16_t?
> > 
> > dcheng asked for all types to be unsigned, so we don't get a mismatch of
> signed
> > and unsigned. I tend to agree.
> > 
> > Initializing unsigned to -1 isn't very common, but it's not unheard of. See
> >
>
https://cs.chromium.org/search/?q=uint.*%5C+%3D%5C+-1;&sq=package:chromium&ty...
> 
> How about UINT16_MAX from cstdint then? The -1 initialization seems like a
> potential trap for someone doing a (last_frame_index >= 0) check somewhere,
> though presumably the compiler would catch that.

So std::numeric_limits<decltype(last_frame_index_)>::max() is an option, but I
decided against it because it doesn't really convey the intent like -1 does. It
isn't really obvious that UINT16_MAX is impossible to reach after
initialization, especially when comparing against it in the cc file. -1 clearly
conveys that this is an uninitialized value that shouldn't be reached after
initialization.

Writing last_frame_index >= 0 for a uint type is just clearly a mistake in any
case, so we shouldn't worry about it.

[dcheng, 2017-01-27 09:41:33]

mojo lgtm

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

https://codereview.chromium.org/2612333002/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc:60:
callback_.Run(nullptr, base::TimeDelta(), -1);
One possibility (if you want) is base::ResetAndReturn(...).Run(...);

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

https://codereview.chromium.org/2612333002/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:646: if (index < frame_index)
index += max;
Nit: a few more comments here could be helpful for future readers (explaining
the purpose of these two branches)

https://codereview.chromium.org/2612333002/diff/140001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell_gl.h (right):

https://codereview.chromium.org/2612333002/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.h:152: typedef std::pair<gvr::Rectf,
gvr::Rectf> BoundsPair;
Nit: prefer using A = B over typedef B A;

[mthiesse, 2017-01-27 16:15:31]

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

https://codereview.chromium.org/2612333002/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc:60:
callback_.Run(nullptr, base::TimeDelta(), -1);
On 2017/01/27 09:41:33, dcheng wrote:
> One possibility (if you want) is base::ResetAndReturn(...).Run(...);

That works well, thanks!

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

https://codereview.chromium.org/2612333002/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.cc:646: if (index < frame_index)
index += max;
On 2017/01/27 09:41:33, dcheng wrote:
> Nit: a few more comments here could be helpful for future readers (explaining
> the purpose of these two branches)

Done.

https://codereview.chromium.org/2612333002/diff/140001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell_gl.h (right):

https://codereview.chromium.org/2612333002/diff/140001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_gl.h:152: typedef std::pair<gvr::Rectf,
gvr::Rectf> BoundsPair;
On 2017/01/27 09:41:33, dcheng wrote:
> Nit: prefer using A = B over typedef B A;

Done.

[mthiesse, 2017-01-27 16:15:46]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-01-27 16:15:47]

The patchset sent to the CQ was uploaded after l-g-t-m from klausw@chromium.org,
bajones@chromium.org, dcheng@chromium.org, klausw@google.com
Link to the patchset: https://codereview.chromium.org/2612333002/#ps180001
(title: "rebase + address comments")

[commit-bot: I haz the power, 2017-01-27 16:16:17]

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 18:06:00]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1485533746643810,
"parent_rev": "eb339344b5cd49e4c382c15adbe62adc754825b8", "commit_rev":
"961dc47e8f5b088debbab559fe81b85f32e9f9e4"}

[commit-bot: I haz the power, 2017-01-27 18:06:49]

Description was changed from

==========
Allow VRDisplay to specify which frame the layer bounds should be updated at.

BUG=655722,670891
==========

to

==========
Allow VRDisplay to specify which frame the layer bounds should be updated at.

BUG=655722,670891

Review-Url: https://codereview.chromium.org/2612333002
Cr-Commit-Position: refs/heads/master@{#446707}
Committed:
https://chromium.googlesource.com/chromium/src/+/961dc47e8f5b088debbab559fe81...
==========

[commit-bot: I haz the power, 2017-01-27 18:06:51]

Committed patchset #7 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/961dc47e8f5b088debbab559fe81...