VR: Wire VrShell UI-related state to the scene manager.

VR: Wire VrShell UI-related state to the scene manager.

This extends the work started by ymalik@ to clean up cross-thread calls,
and uses the new mechanism to pass the state we need to the UI.

BUG=715713
Review-Url: https://codereview.chromium.org/2866853002
Cr-Commit-Position: refs/heads/master@{#470318}
Committed: https://chromium.googlesource.com/chromium/src/+/4cf2a298101b0b2ce018e31be498c13fe61a9ce8

[cjgrant, 2017-05-05 21:58:09]

cjgrant@chromium.org changed reviewers:
+ amp@chromium.org, mthiesse@chromium.org, ymalik@chromium.org

[cjgrant, 2017-05-05 21:58:10]

Folks, here's the addendum to Yash's CL that completes the cross-thread
interfaces.  Now VrShell can cleanly post UI state to scene manager, without
caring where the calls are being directed.

[ymalik, 2017-05-08 14:05:01]

lgtm % questions

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.h (right):

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.h:33: void SetFullscreen(bool
enabled);
nit: add a comment here or somewhere mentioning that most of these are unused
currently or will be used in future CLs?

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

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:205: void
VrShell::PostToGlThreadWhenReady(const base::Closure& task) {
If we're encapsulating the calls between vr shell and the gl thread behind
interfaces implemented by VrGlThread, it may make sense to wrap these as well?

thoughts?

[cjgrant, 2017-05-08 14:14:37]

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.h (right):

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.h:33: void SetFullscreen(bool
enabled);
On 2017/05/08 14:05:00, ymalik wrote:
> nit: add a comment here or somewhere mentioning that most of these are unused
> currently or will be used in future CLs?

Sure, but that future will be in the next 2 days. :)

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

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:205: void
VrShell::PostToGlThreadWhenReady(const base::Closure& task) {
On 2017/05/08 14:05:00, ymalik wrote:
> If we're encapsulating the calls between vr shell and the gl thread behind
> interfaces implemented by VrGlThread, it may make sense to wrap these as well?
> 
> thoughts?

I thought about that, and didn't for the following reason:  The UI calls don't
really care about threading - VrShell wants to instantiate some object to manage
the UI, and in this case, the calls happen to be queued to the GL thread.  But,
for the WebVR system, there are explicit dependencies between the main and GL
threads, and I think it's good if that relationship stays crystal clear (ie.
ui_->DoWebVrImplementationThing() doesn't make sense to me).  That's my
reasoning anyway.

[mthiesse, 2017-05-08 16:04:56]

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_interface.h (right):

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_interface.h:26: virtual void SetWebVr(bool
enabled) = 0;
nit: enabled is a strange name for this...

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

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_gl_thread.cc:71:
weak_vr_shell_gl_->GetTaskRunner()->PostTask(
Should this be task_runner()->PostTask()?

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_gl_thread.h (right):

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_gl_thread.h:26: class VrGLThread : public
VrBrowserInterface,
Sidenote, we should think about renaming this class. I'm not sure VrGLThread is
the best name for this anymore.

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_gl_thread.h:27: public base::Thread,
Is there some style-guide approved way to order these, or is it just random?

Maybe make this alphabetical?

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

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:205: void
VrShell::PostToGlThreadWhenReady(const base::Closure& task) {
On 2017/05/08 14:14:37, cjgrant wrote:
> On 2017/05/08 14:05:00, ymalik wrote:
> > If we're encapsulating the calls between vr shell and the gl thread behind
> > interfaces implemented by VrGlThread, it may make sense to wrap these as
well?
> > 
> > thoughts?
> 
> I thought about that, and didn't for the following reason:  The UI calls don't
> really care about threading - VrShell wants to instantiate some object to
manage
> the UI, and in this case, the calls happen to be queued to the GL thread. 
But,
> for the WebVR system, there are explicit dependencies between the main and GL
> threads, and I think it's good if that relationship stays crystal clear (ie.
> ui_->DoWebVrImplementationThing() doesn't make sense to me).  That's my
> reasoning anyway.

A future CL should probably add another interface for the VrShell/VrShellGl
communication.

[amp, 2017-05-08 18:30:11]

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

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:205: void
VrShell::PostToGlThreadWhenReady(const base::Closure& task) {
On 2017/05/08 16:04:55, mthiesse wrote:
> On 2017/05/08 14:14:37, cjgrant wrote:
> > On 2017/05/08 14:05:00, ymalik wrote:
> > > If we're encapsulating the calls between vr shell and the gl thread behind
> > > interfaces implemented by VrGlThread, it may make sense to wrap these as
> well?
> > > 
> > > thoughts?
> > 
> > I thought about that, and didn't for the following reason:  The UI calls
don't
> > really care about threading - VrShell wants to instantiate some object to
> manage
> > the UI, and in this case, the calls happen to be queued to the GL thread. 
> But,
> > for the WebVR system, there are explicit dependencies between the main and
GL
> > threads, and I think it's good if that relationship stays crystal clear (ie.
> > ui_->DoWebVrImplementationThing() doesn't make sense to me).  That's my
> > reasoning anyway.
> 
> A future CL should probably add another interface for the VrShell/VrShellGl
> communication.

+1, it was confusing to see VrShellGl as the implementation of the browser, yet
it's also the way that VrShell communicates with the UI.  Splitting things out
would reduce confusion.

Did anyone scribble some boxes on a whiteboard somewhere about how it should
look like (even if we can't get there immediately)?

[cjgrant, 2017-05-08 19:17:23]

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_interface.h (right):

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_interface.h:26: virtual void SetWebVr(bool
enabled) = 0;
On 2017/05/08 16:04:55, mthiesse wrote:
> nit: enabled is a strange name for this...

Done.  I renamed this to SetWebVrMode, to match elsewhere, and also renamed a
bunch of SetWebVR to lowercase 'Vr'.  We aren't consistent everywhere, but I
believe this is what it should be.

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.h (right):

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.h:33: void SetFullscreen(bool
enabled);
On 2017/05/08 14:14:37, cjgrant wrote:
> On 2017/05/08 14:05:00, ymalik wrote:
> > nit: add a comment here or somewhere mentioning that most of these are
unused
> > currently or will be used in future CLs?
> 
> Sure, but that future will be in the next 2 days. :)

Done.

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

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_gl_thread.cc:71:
weak_vr_shell_gl_->GetTaskRunner()->PostTask(
On 2017/05/08 16:04:55, mthiesse wrote:
> Should this be task_runner()->PostTask()?

Done. Good call.

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_gl_thread.h (right):

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_gl_thread.h:26: class VrGLThread : public
VrBrowserInterface,
On 2017/05/08 16:04:55, mthiesse wrote:
> Sidenote, we should think about renaming this class. I'm not sure VrGLThread
is
> the best name for this anymore.

I think the name is still right, but all the cross-thread interfaces could be
managed in a proxy class.  I think this fits in the Technical Debt category.

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_gl_thread.h:27: public base::Thread,
On 2017/05/08 16:04:55, mthiesse wrote:
> Is there some style-guide approved way to order these, or is it just random?
> 
> Maybe make this alphabetical?

I don't think there's a style guide.  But, having Thread first is probably best,
and I alphabetized the other two.

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

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:205: void
VrShell::PostToGlThreadWhenReady(const base::Closure& task) {
On 2017/05/08 18:30:11, amp wrote:
> On 2017/05/08 16:04:55, mthiesse wrote:
> > On 2017/05/08 14:14:37, cjgrant wrote:
> > > On 2017/05/08 14:05:00, ymalik wrote:
> > > > If we're encapsulating the calls between vr shell and the gl thread
behind
> > > > interfaces implemented by VrGlThread, it may make sense to wrap these as
> > well?
> > > > 
> > > > thoughts?
> > > 
> > > I thought about that, and didn't for the following reason:  The UI calls
> don't
> > > really care about threading - VrShell wants to instantiate some object to
> > manage
> > > the UI, and in this case, the calls happen to be queued to the GL thread. 
> > But,
> > > for the WebVR system, there are explicit dependencies between the main and
> GL
> > > threads, and I think it's good if that relationship stays crystal clear
(ie.
> > > ui_->DoWebVrImplementationThing() doesn't make sense to me).  That's my
> > > reasoning anyway.
> > 
> > A future CL should probably add another interface for the VrShell/VrShellGl
> > communication.
> 
> +1, it was confusing to see VrShellGl as the implementation of the browser,
yet
> it's also the way that VrShell communicates with the UI.  Splitting things out
> would reduce confusion.
> 
> Did anyone scribble some boxes on a whiteboard somewhere about how it should
> look like (even if we can't get there immediately)?

We basically have 3 components - UI, Shell and GL.  We could have an interface
for each of the 6 possible interaction paths (although at least one of them
isn't used).  As for keeping them in one class, the goal was to keep all the
bind calls and "who's on what thread" in one spot, rather than strewn around. 
Ie, the GL thread is supplying all the cross-thread help.  So I'm not sure
splitting them out would "reduce" confusion, but we should certainly discuss.

Whatever we do, we need to keep the interfaces structured so that blocks can
easily be unit tested.

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

https://codereview.chromium.org/2866853002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_gl_thread.cc:116: void
VrGLThread::SetLoadProgress(double progress) {
I alphabetized these as well, to match the interface.

[mthiesse, 2017-05-08 20:21:40]

lgtm

[amp, 2017-05-08 20:30:06]

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

https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:205: void
VrShell::PostToGlThreadWhenReady(const base::Closure& task) {
On 2017/05/08 19:17:22, cjgrant wrote:
> On 2017/05/08 18:30:11, amp wrote:
> > On 2017/05/08 16:04:55, mthiesse wrote:
> > > On 2017/05/08 14:14:37, cjgrant wrote:
> > > > On 2017/05/08 14:05:00, ymalik wrote:
> > > > > If we're encapsulating the calls between vr shell and the gl thread
> behind
> > > > > interfaces implemented by VrGlThread, it may make sense to wrap these
as
> > > well?
> > > > > 
> > > > > thoughts?
> > > > 
> > > > I thought about that, and didn't for the following reason:  The UI calls
> > don't
> > > > really care about threading - VrShell wants to instantiate some object
to
> > > manage
> > > > the UI, and in this case, the calls happen to be queued to the GL
thread. 
> > > But,
> > > > for the WebVR system, there are explicit dependencies between the main
and
> > GL
> > > > threads, and I think it's good if that relationship stays crystal clear
> (ie.
> > > > ui_->DoWebVrImplementationThing() doesn't make sense to me).  That's my
> > > > reasoning anyway.
> > > 
> > > A future CL should probably add another interface for the
VrShell/VrShellGl
> > > communication.
> > 
> > +1, it was confusing to see VrShellGl as the implementation of the browser,
> yet
> > it's also the way that VrShell communicates with the UI.  Splitting things
out
> > would reduce confusion.
> > 
> > Did anyone scribble some boxes on a whiteboard somewhere about how it should
> > look like (even if we can't get there immediately)?
> 
> We basically have 3 components - UI, Shell and GL.  We could have an interface
> for each of the 6 possible interaction paths (although at least one of them
> isn't used).  As for keeping them in one class, the goal was to keep all the
> bind calls and "who's on what thread" in one spot, rather than strewn around. 
> Ie, the GL thread is supplying all the cross-thread help.  So I'm not sure
> splitting them out would "reduce" confusion, but we should certainly discuss.
> 
> Whatever we do, we need to keep the interfaces structured so that blocks can
> easily be unit tested.
I suppose we don't need to pull anything out, but perhaps just name them
differently (per Michael's other comment about the name above).  This is fine to
leave to future technical debt.

Re: GL thread supplying cross-thread help, that feels like the GL thread is
taking on a lot of responsibility.  It's not just the gl thread, it's also the
thing that knows about all the other threads and how to run things on them. (or
am I reading too much into your response?)

The separate interfaces to access those features of GL thread is a nice
abstraction, but it's not what I would imagine is happening if I just read the
name VrGLThread.

And yes keeping things structured for better testing is definitely good.

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

https://codereview.chromium.org/2866853002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:248: void
UiSceneManager::SetFullscreen(bool enabled) {}
I went with OnFullscreenChanged in my change since we didn't actually set
anything on the call, but if it's more consistent with the rest of the UI API to
have it 'SetXXX' feel free to change it back when you rebase.

[amp, 2017-05-08 20:30:33]

forgot my lgtm :)

[cjgrant, 2017-05-08 20:39:06]

On 2017/05/08 20:30:06, amp wrote:
>
https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
> File chrome/browser/android/vr_shell/vr_shell.cc (right):
> 
>
https://codereview.chromium.org/2866853002/diff/1/chrome/browser/android/vr_s...
> chrome/browser/android/vr_shell/vr_shell.cc:205: void
> VrShell::PostToGlThreadWhenReady(const base::Closure& task) {
> On 2017/05/08 19:17:22, cjgrant wrote:
> > On 2017/05/08 18:30:11, amp wrote:
> > > On 2017/05/08 16:04:55, mthiesse wrote:
> > > > On 2017/05/08 14:14:37, cjgrant wrote:
> > > > > On 2017/05/08 14:05:00, ymalik wrote:
> > > > > > If we're encapsulating the calls between vr shell and the gl thread
> > behind
> > > > > > interfaces implemented by VrGlThread, it may make sense to wrap
these
> as
> > > > well?
> > > > > > 
> > > > > > thoughts?
> > > > > 
> > > > > I thought about that, and didn't for the following reason:  The UI
calls
> > > don't
> > > > > really care about threading - VrShell wants to instantiate some object
> to
> > > > manage
> > > > > the UI, and in this case, the calls happen to be queued to the GL
> thread. 
> > > > But,
> > > > > for the WebVR system, there are explicit dependencies between the main
> and
> > > GL
> > > > > threads, and I think it's good if that relationship stays crystal
clear
> > (ie.
> > > > > ui_->DoWebVrImplementationThing() doesn't make sense to me).  That's
my
> > > > > reasoning anyway.
> > > > 
> > > > A future CL should probably add another interface for the
> VrShell/VrShellGl
> > > > communication.
> > > 
> > > +1, it was confusing to see VrShellGl as the implementation of the
browser,
> > yet
> > > it's also the way that VrShell communicates with the UI.  Splitting things
> out
> > > would reduce confusion.
> > > 
> > > Did anyone scribble some boxes on a whiteboard somewhere about how it
should
> > > look like (even if we can't get there immediately)?
> > 
> > We basically have 3 components - UI, Shell and GL.  We could have an
interface
> > for each of the 6 possible interaction paths (although at least one of them
> > isn't used).  As for keeping them in one class, the goal was to keep all the
> > bind calls and "who's on what thread" in one spot, rather than strewn
around. 
> > Ie, the GL thread is supplying all the cross-thread help.  So I'm not sure
> > splitting them out would "reduce" confusion, but we should certainly
discuss.
> > 
> > Whatever we do, we need to keep the interfaces structured so that blocks can
> > easily be unit tested.
> I suppose we don't need to pull anything out, but perhaps just name them
> differently (per Michael's other comment about the name above).  This is fine
to
> leave to future technical debt.
> 
> Re: GL thread supplying cross-thread help, that feels like the GL thread is
> taking on a lot of responsibility.  It's not just the gl thread, it's also the
> thing that knows about all the other threads and how to run things on them.
(or
> am I reading too much into your response?)
> 
> The separate interfaces to access those features of GL thread is a nice
> abstraction, but it's not what I would imagine is happening if I just read the
> name VrGLThread.
> 
> And yes keeping things structured for better testing is definitely good.
> 
>
https://codereview.chromium.org/2866853002/diff/20001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):
> 
>
https://codereview.chromium.org/2866853002/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/ui_scene_manager.cc:248: void
> UiSceneManager::SetFullscreen(bool enabled) {}
> I went with OnFullscreenChanged in my change since we didn't actually set
> anything on the call, but if it's more consistent with the rest of the UI API
to
> have it 'SetXXX' feel free to change it back when you rebase.

Cool.  I'll need to rebase onto your change and rework that bit.

As for the interface thing, we could VC about this.  It's not even clear that
the interfaces are sliced the right way.  For example, should the GL module get
passed (1) two interfaces, one to a UI and another to the Shell, or (2) a single
interface called "things the GL module must report"?  Ie, should GL know who
it's feeding information to or not.  So many choices. :)

[cjgrant, 2017-05-09 00:23:09]

The CQ bit was checked by cjgrant@chromium.org

[cjgrant, 2017-05-09 00:23:10]

The patchset sent to the CQ was uploaded after l-g-t-m from ymalik@chromium.org,
mthiesse@chromium.org, amp@chromium.org
Link to the patchset: https://codereview.chromium.org/2866853002/#ps60001
(title: "Rebase; remove now-unused source file.")

[commit-bot: I haz the power, 2017-05-09 00:29:35]

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-05-09 01:24:13]

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

[commit-bot: I haz the power, 2017-05-09 01:24:14]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[cjgrant, 2017-05-09 14:07:21]

The CQ bit was checked by cjgrant@chromium.org

[commit-bot: I haz the power, 2017-05-09 14:07:47]

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-05-09 14:13:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494338841670610,
"parent_rev": "7d4ca9132c8d4cdbf1091c992cc1c82437d7c8c0", "commit_rev":
"4cf2a298101b0b2ce018e31be498c13fe61a9ce8"}

[commit-bot: I haz the power, 2017-05-09 14:13:29]

Description was changed from

==========
VR: Wire VrShell UI-related state to the scene manager.

This extends the work started by ymalik@ to clean up cross-thread calls,
and uses the new mechanism to pass the state we need to the UI.

BUG=715713
==========

to

==========
VR: Wire VrShell UI-related state to the scene manager.

This extends the work started by ymalik@ to clean up cross-thread calls,
and uses the new mechanism to pass the state we need to the UI.

BUG=715713

Review-Url: https://codereview.chromium.org/2866853002
Cr-Commit-Position: refs/heads/master@{#470318}
Committed:
https://chromium.googlesource.com/chromium/src/+/4cf2a298101b0b2ce018e31be498...
==========

[commit-bot: I haz the power, 2017-05-09 14:13:30]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/4cf2a298101b0b2ce018e31be498...