Pause drawing webvr when the App button is pressed

Pause drawing webvr when the App button is pressed

Clicking the App button toggles between pausing and resuming webvr drawing. This
is the first step to crbug.com/713377. We still need to figure out how to properly
handle app button clicks from WebVr presentation mode. This CL mainly has all the
plumbing required for the UiSceneManager to handle the clicks.

This CL declares a VrBrowserInterface that VrGLThread implements. VrShellLGl gets
this interface and calls functions on it which has the PostTask/binding logic.
Future-work: do the same for calls from VrShell -> Gl thread (VrUiInterface). 

BUG=713377
Review-Url: https://codereview.chromium.org/2833773005
Cr-Commit-Position: refs/heads/master@{#469138}
Committed: https://chromium.googlesource.com/chromium/src/+/847ddc166e042a5e929ac16c9327dc3e02cba21c

[ymalik, 2017-04-21 19:39:57]

ymalik@chromium.org changed reviewers:
+ bshe@chromium.org, cjgrant@chromium.org

[ymalik, 2017-04-21 19:39:58]

PTAL :)

[bshe, 2017-04-21 21:08:42]

https://codereview.chromium.org/2833773005/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene.h (right):

https://codereview.chromium.org/2833773005/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene.h:68: void
SetWebVrRenderingEnabled(bool);
nit: bool enabled

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

https://codereview.chromium.org/2833773005/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:423: case SET_CONTENT_PAUSED: {
wondering if this UiAction is still needed? If not, perhap remove it?

https://codereview.chromium.org/2833773005/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.h (right):

https://codereview.chromium.org/2833773005/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.h:187: bool content_paused_ = false;
nit: remove this too?

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

https://codereview.chromium.org/2833773005/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:663: content_paused_ =
!content_paused_;
perhaps rename to content_rendering_enabled_ to make it more readable?

[cjgrant, 2017-04-21 21:36:16]

On 2017/04/21 21:08:42, bshe wrote:
>
https://codereview.chromium.org/2833773005/diff/20001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/ui_scene.h (right):
> 
>
https://codereview.chromium.org/2833773005/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/ui_scene.h:68: void
> SetWebVrRenderingEnabled(bool);
> nit: bool enabled
> 
>
https://codereview.chromium.org/2833773005/diff/20001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/vr_shell.cc (right):
> 
>
https://codereview.chromium.org/2833773005/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/vr_shell.cc:423: case SET_CONTENT_PAUSED: {
> wondering if this UiAction is still needed? If not, perhap remove it?
> 
>
https://codereview.chromium.org/2833773005/diff/20001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/vr_shell.h (right):
> 
>
https://codereview.chromium.org/2833773005/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/vr_shell.h:187: bool content_paused_ = false;
> nit: remove this too?
> 
>
https://codereview.chromium.org/2833773005/diff/20001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):
> 
>
https://codereview.chromium.org/2833773005/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/vr_shell_gl.cc:663: content_paused_ =
> !content_paused_;
> perhaps rename to content_rendering_enabled_ to make it more readable?

There's a change coming to route the app button press through the scene manager,
so it can control whether WebVR and/or buttons are needed.  That will cause some
of this to be reworked.

[bsheedy, 2017-04-21 21:44:30]

This should be unittestable, right? If so, could you add some?

Might also want to consider adding an end-to-end test since we can use the
controller in tests once https://codereview.chromium.org/2826273002/ lands.

[cjgrant, 2017-04-24 14:12:38]

On 2017/04/21 21:44:30, bsheedy wrote:
> This should be unittestable, right? If so, could you add some?
> 
> Might also want to consider adding an end-to-end test since we can use the
> controller in tests once https://codereview.chromium.org/2826273002/ lands.

On end-to-end tests - I haven't thought about those since we abandoned the
Java/Javascript/Native CL for the HTML UI.  Brian, the way you see this, how do
we check the result?  Check at the WebVR page level?

[bsheedy, 2017-04-24 16:42:08]

On 2017/04/24 14:12:38, cjgrant wrote:
> On 2017/04/21 21:44:30, bsheedy wrote:
> > This should be unittestable, right? If so, could you add some?
> > 
> > Might also want to consider adding an end-to-end test since we can use the
> > controller in tests once https://codereview.chromium.org/2826273002/ lands.
> 
> On end-to-end tests - I haven't thought about those since we abandoned the
> Java/Javascript/Native CL for the HTML UI.  Brian, the way you see this, how
do
> we check the result?  Check at the WebVR page level?

That might be an option? Will this have any impact on WebVR's render loop as far
as JavaScript can tell? E.g. will vrDisplay.requestAnimationFrame() stop being
called? If so, we could use that.

Otherwise, might just have to add some Java/Native plumbing.

[tiborg, 2017-04-27 14:46:49]

Description was changed from

==========
Pause drawing webvr when the App button is pressed

Clicking the App button toggles betwen pausing and resuming webvr drawing. This
is a first step to crbug.co/713377.

This CL removed the "content_paused" state from vr_shell to have the login in
one place (vr_shell_gl).

BUG=713377
==========

to

==========
Pause drawing webvr when the App button is pressed

Clicking the App button toggles betwen pausing and resuming webvr drawing. This
is a first step to crbug.co/713377.

This CL removed the "content_paused" state from vr_shell to have the login in
one place (vr_shell_gl).

BUG=713377
==========

[ymalik, 2017-04-28 00:10:11]

On 2017/04/24 16:42:08, bsheedy wrote:
> On 2017/04/24 14:12:38, cjgrant wrote:
> > On 2017/04/21 21:44:30, bsheedy wrote:
> > > This should be unittestable, right? If so, could you add some?
> > > 
> > > Might also want to consider adding an end-to-end test since we can use the
> > > controller in tests once https://codereview.chromium.org/2826273002/
lands.
> > 
> > On end-to-end tests - I haven't thought about those since we abandoned the
> > Java/Javascript/Native CL for the HTML UI.  Brian, the way you see this, how
> do
> > we check the result?  Check at the WebVR page level?
> 
> That might be an option? Will this have any impact on WebVR's render loop as
far
> as JavaScript can tell? E.g. will vrDisplay.requestAnimationFrame() stop being
> called? If so, we could use that.
> 
> Otherwise, might just have to add some Java/Native plumbing.

I have added a unittest for now. I think end to end tests make more sense once
we have figured out what we want to do with the app button press.

[ymalik, 2017-04-28 00:12:04]

@cjgrant, @bshe

PTAL. I have refactored post tasks to the main thread from the gl thread to go
through a new interface. This makes the communication between the two threads
cleaner. 

I am currently just pausing webvr rendering when the app button is pressed, but
we can get rid of it if you prefer that for now.

https://codereview.chromium.org/2833773005/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene.h (right):

https://codereview.chromium.org/2833773005/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene.h:68: void
SetWebVrRenderingEnabled(bool);
On 2017/04/21 21:08:42, bshe wrote:
> nit: bool enabled

Done.

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

https://codereview.chromium.org/2833773005/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:423: case SET_CONTENT_PAUSED: {
On 2017/04/21 21:08:42, bshe wrote:
> wondering if this UiAction is still needed? If not, perhap remove it?

I think we have it here as reference for things that need to be implemented.
Added comment to clarify.

https://codereview.chromium.org/2833773005/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.h (right):

https://codereview.chromium.org/2833773005/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.h:187: bool content_paused_ = false;
On 2017/04/21 21:08:42, bshe wrote:
> nit: remove this too?

Done.

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

https://codereview.chromium.org/2833773005/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:663: content_paused_ =
!content_paused_;
On 2017/04/21 21:08:42, bshe wrote:
> perhaps rename to content_rendering_enabled_ to make it more readable?

Done.

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

https://codereview.chromium.org/2833773005/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:112: // unclear what we want
to do here and this will most likely change.
I'm just pausing the webvr rending for now. If y'all prefer just doing nothing
here until we figure out how we want to handle the app button, I'd be okay with
that too.

[ymalik, 2017-04-28 00:16:59]

Description was changed from

==========
Pause drawing webvr when the App button is pressed

Clicking the App button toggles betwen pausing and resuming webvr drawing. This
is a first step to crbug.co/713377.

This CL removed the "content_paused" state from vr_shell to have the login in
one place (vr_shell_gl).

BUG=713377
==========

to

==========
Pause drawing webvr when the App button is pressed

Clicking the App button toggles between pausing and resuming webvr drawing. This
is the first step to crbug.com/713377. We still need to figure out how to
properly
handle app button clicks from WebVr presentation mode. This CL mainly has all
the
plumbing required for the UiSceneManager to handle the clicks.

This CL makes all the PostTasks from the gl thread to the main thread
go through VrThreadGl, which implements the VrThreadEnvoy interface. 

BUG=713377
==========

[cjgrant, 2017-04-28 18:12:07]

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

[cjgrant, 2017-04-28 18:12:08]

+mthiesse

[cjgrant, 2017-04-28 18:21:06]

An initial high-level comment: This CL does clean up the bind calls, for sure. 
But I'm still wrestling with how much each of VrShell and VrShellGl still need
to know of each other.  The thing I'm visualizing is this:

- Scene manager is given an object that implements a browser interface
- VrShell is given an object that implements UiInterface.
- Both of those implementations are implemented by the thread object, and the
thread object does the bindings, because it knows which thread each caller is
on.

General thoughts?

[cjgrant, 2017-04-28 18:25:23]

https://codereview.chromium.org/2833773005/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right):

https://codereview.chromium.org/2833773005/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:72:
std::unique_ptr<MockVrThreadEnvoy> envoy =
A scene manager test base class should probably make the envoy, scene and
manager, so that a specific test just has to feed the manager an input and
verify a result.

https://codereview.chromium.org/2833773005/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:79:
EXPECT_CALL(*envoy, GetVrShell()).Times(1).WillOnce(Return(nullptr));
Based on my top level comment, I figure this would be better as:

shell_proxy = new shell proxy
manager = new manager(shell_proxy)
EXPECT(shell_proxy, ShowWebVr(false)
manager->AppButtonPressed()

It's weird that we're testing that scene manager fished for task runners, as
opposed to calling a method in response to input.

[ymalik, 2017-05-02 17:43:45]

On 2017/04/28 18:21:06, cjgrant wrote:
> An initial high-level comment: This CL does clean up the bind calls, for sure.

> But I'm still wrestling with how much each of VrShell and VrShellGl still need
> to know of each other.  The thing I'm visualizing is this:
> 
> - Scene manager is given an object that implements a browser interface
> - VrShell is given an object that implements UiInterface.
> - Both of those implementations are implemented by the thread object, and the
> thread object does the bindings, because it knows which thread each caller is
> on.
> 
> General thoughts?

Talked to you offline. The latest patch set does the BrowserInterface part.

[ymalik, 2017-05-02 17:45:53]

Description was changed from

==========
Pause drawing webvr when the App button is pressed

Clicking the App button toggles between pausing and resuming webvr drawing. This
is the first step to crbug.com/713377. We still need to figure out how to
properly
handle app button clicks from WebVr presentation mode. This CL mainly has all
the
plumbing required for the UiSceneManager to handle the clicks.

This CL makes all the PostTasks from the gl thread to the main thread
go through VrThreadGl, which implements the VrThreadEnvoy interface. 

BUG=713377
==========

to

==========
Pause drawing webvr when the App button is pressed

Clicking the App button toggles between pausing and resuming webvr drawing. This
is the first step to crbug.com/713377. We still need to figure out how to
properly
handle app button clicks from WebVr presentation mode. This CL mainly has all
the
plumbing required for the UiSceneManager to handle the clicks.

This CL declares a VrBrowserInterface that VrGLThread implements. VrShellLGl
gets
this interface and calls functions on it which has the PostTask/binding logic.
Future-work: do the same for calls from VrShell -> Gl thread (VrUiInterface). 

BUG=713377
==========

[ymalik, 2017-05-02 17:46:00]

https://codereview.chromium.org/2833773005/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right):

https://codereview.chromium.org/2833773005/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:72:
std::unique_ptr<MockVrThreadEnvoy> envoy =
On 2017/04/28 18:25:23, cjgrant wrote:
> A scene manager test base class should probably make the envoy, scene and
> manager, so that a specific test just has to feed the manager an input and
> verify a result.

Done.

https://codereview.chromium.org/2833773005/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:79:
EXPECT_CALL(*envoy, GetVrShell()).Times(1).WillOnce(Return(nullptr));
On 2017/04/28 18:25:23, cjgrant wrote:
> Based on my top level comment, I figure this would be better as:
> 
> shell_proxy = new shell proxy
> manager = new manager(shell_proxy)
> EXPECT(shell_proxy, ShowWebVr(false)
> manager->AppButtonPressed()
> 
> It's weird that we're testing that scene manager fished for task runners, as
> opposed to calling a method in response to input.

Done.

[cjgrant, 2017-05-02 18:07:06]

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

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:15: #include
"chrome/browser/android/vr_shell/vr_gl_thread.h"
Is the thread header needed?

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:41: :
weak_browser_interface_(weak_browser_interface),
s/weak_browser_interface_/browser_/ or /shell_/? Ie, shorter name, independent
of type.

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.h (right):

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.h:31: void AppButtonPressed();
How about OnAppButtonPressed or HandleAppButtonPressed? Otherwise it sounds like
a getter.

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right):

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:36: class
UiSceneManagerTest : public testing::Test {
This looks awesome now IMO.  Thanks!

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:51:
TEST_F(UiSceneManagerTest, AppButtonPressed) {
I think style prefers verbose test names that describe what's covered, but I
can't find a reference to this.  Here, maybe "ContentPausesOnAppButtonPress"?

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:51:
TEST_F(UiSceneManagerTest, AppButtonPressed) {
"Pressed" sounds like "pressed and held".  "Clicked" seems better to me.

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:53:
manager_->AppButtonPressed();
Why not also check that another click unpaused content?

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:53:
manager_->AppButtonPressed();
Should you check that the scene is also now reporting "don't draw WebVR?"

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_browser_interface.h (right):

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_browser_interface.h:24: // An interface for
communication with Vr Browser.
Here you could actually say this is an interface to VrShell.

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_browser_interface.h:29: // TODO(ymalik): Add
comments for these functions.
It'd be better to have a single high-level comment (up with the class comment)
saying that these calls are proxies to methods on VrShell.  No point in
duplicating function descriptions IMO.

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_browser_interface.h:34: virtual void
AppButtonPressed() {}
See previous note about "Pressed"

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

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_gl_thread.cc:18: void RunDisplayInfoCallback(
This will need a rebase with Michael's change I think?

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_gl_thread.cc:55: void
VrGLThread::ContentSurfaceChanged(jobject surface) {
This is cool.  Thanks for rejigging this to keep the binding stuff centralized.

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

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:498:
weak_browser_interface_->GvrDelegateReady();
I can't handle this - the code looks too clean now.

[ymalik, 2017-05-02 20:28:41]

@cjgrant ptal :)

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

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:15: #include
"chrome/browser/android/vr_shell/vr_gl_thread.h"
On 2017/05/02 18:07:05, cjgrant wrote:
> Is the thread header needed?

Totally not. thanks!

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:41: :
weak_browser_interface_(weak_browser_interface),
On 2017/05/02 18:07:06, cjgrant wrote:
> s/weak_browser_interface_/browser_/ or /shell_/? Ie, shorter name, independent
> of type.

Done here and VrShellGl

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.h (right):

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.h:31: void AppButtonPressed();
On 2017/05/02 18:07:06, cjgrant wrote:
> How about OnAppButtonPressed or HandleAppButtonPressed? Otherwise it sounds
like
> a getter.

Done.

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right):

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:36: class
UiSceneManagerTest : public testing::Test {
On 2017/05/02 18:07:06, cjgrant wrote:
> This looks awesome now IMO.  Thanks!

Acknowledged.

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:51:
TEST_F(UiSceneManagerTest, AppButtonPressed) {
On 2017/05/02 18:07:06, cjgrant wrote:
> "Pressed" sounds like "pressed and held".  "Clicked" seems better to me.

Done. Renamed OnAppButtonPressed to OnAppButtonClicked.

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:53:
manager_->AppButtonPressed();
On 2017/05/02 18:07:06, cjgrant wrote:
> Should you check that the scene is also now reporting "don't draw WebVR?"

Totally!

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_browser_interface.h (right):

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_browser_interface.h:24: // An interface for
communication with Vr Browser.
On 2017/05/02 18:07:06, cjgrant wrote:
> Here you could actually say this is an interface to VrShell.

That's not entirely true because for e.g. OnAppButtonPressed goes to
UiSceneManager which runs on the gl thread (which then post tasks to vr shell to
pause the web contents). Does that make sense?

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_browser_interface.h:29: // TODO(ymalik): Add
comments for these functions.
On 2017/05/02 18:07:06, cjgrant wrote:
> It'd be better to have a single high-level comment (up with the class comment)
> saying that these calls are proxies to methods on VrShell.  No point in
> duplicating function descriptions IMO.

That's fair.

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_browser_interface.h:34: virtual void
AppButtonPressed() {}
On 2017/05/02 18:07:06, cjgrant wrote:
> See previous note about "Pressed"

Done.

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

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_gl_thread.cc:18: void RunDisplayInfoCallback(
On 2017/05/02 18:07:06, cjgrant wrote:
> This will need a rebase with Michael's change I think?

Yes. Rebased.

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_gl_thread.cc:55: void
VrGLThread::ContentSurfaceChanged(jobject surface) {
On 2017/05/02 18:07:06, cjgrant wrote:
> This is cool.  Thanks for rejigging this to keep the binding stuff
centralized.

Acknowledged.

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

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:498:
weak_browser_interface_->GvrDelegateReady();
On 2017/05/02 18:07:06, cjgrant wrote:
> I can't handle this - the code looks too clean now.

:D thanks. Just to be clear, this comment doesn't have anything actionable
right? :P

[cjgrant, 2017-05-03 14:40:38]

lgtm

lgtm pending the latest test comment.

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

https://codereview.chromium.org/2833773005/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:498:
weak_browser_interface_->GvrDelegateReady();
On 2017/05/02 20:28:40, ymalik (slow) wrote:
> On 2017/05/02 18:07:06, cjgrant wrote:
> > I can't handle this - the code looks too clean now.
> 
> :D thanks. Just to be clear, this comment doesn't have anything actionable
> right? :P

Correct.  Sorry. :)

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

https://codereview.chromium.org/2833773005/diff/120001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:60:
EXPECT_CALL(*browser_, OnContentPaused(false)).Times(1);
This looks good, but I think it touches on the ordering behaviour of mocks. 
See:
https://github.com/google/googletest/blob/master/googlemock/docs/ForDummies.m...

For kicks, can you see if this test still passes if you reverse the expectations
to 'false' then 'true'?  If it falsely passes, the doc shows how to make this
work right.

[mthiesse, 2017-05-03 16:12:04]

lgtm

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

https://codereview.chromium.org/2833773005/diff/120001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene_manager.cc:195:
browser_->OnContentPaused(content_rendering_enabled_);
Need to check that this weak ptr isn't null before using.

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

https://codereview.chromium.org/2833773005/diff/120001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:418: // actions that need to
implemented natively.
nit: s/to/to be/

[ymalik, 2017-05-03 20:26:38]

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

https://codereview.chromium.org/2833773005/diff/120001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene_manager.cc:195:
browser_->OnContentPaused(content_rendering_enabled_);
On 2017/05/03 16:12:03, mthiesse wrote:
> Need to check that this weak ptr isn't null before using.

Made it a raw ptr.

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

https://codereview.chromium.org/2833773005/diff/120001/chrome/browser/android...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:60:
EXPECT_CALL(*browser_, OnContentPaused(false)).Times(1);
On 2017/05/03 14:40:38, cjgrant wrote:
> This looks good, but I think it touches on the ordering behaviour of mocks. 
> See:
>
https://github.com/google/googletest/blob/master/googlemock/docs/ForDummies.m...
> 
> For kicks, can you see if this test still passes if you reverse the
expectations
> to 'false' then 'true'?  If it falsely passes, the doc shows how to make this
> work right.

Totally. In fact it helped me find a bug in the code :P

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

https://codereview.chromium.org/2833773005/diff/120001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:418: // actions that need to
implemented natively.
On 2017/05/03 16:12:03, mthiesse wrote:
> nit: s/to/to be/

Done. Thanks.

[ymalik, 2017-05-03 20:26:45]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2017-05-03 20:26:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, cjgrant@chromium.org
Link to the patchset: https://codereview.chromium.org/2833773005/#ps140001
(title: "make UiBrowserInterface a raw ptr")

[commit-bot: I haz the power, 2017-05-03 20:27:28]

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-03 21:43:48]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493843205211770,
"parent_rev": "23935bc344576994ce5eb91be72a3907f7a68f3e", "commit_rev":
"847ddc166e042a5e929ac16c9327dc3e02cba21c"}

[commit-bot: I haz the power, 2017-05-03 21:44:06]

Description was changed from

==========
Pause drawing webvr when the App button is pressed

Clicking the App button toggles between pausing and resuming webvr drawing. This
is the first step to crbug.com/713377. We still need to figure out how to
properly
handle app button clicks from WebVr presentation mode. This CL mainly has all
the
plumbing required for the UiSceneManager to handle the clicks.

This CL declares a VrBrowserInterface that VrGLThread implements. VrShellLGl
gets
this interface and calls functions on it which has the PostTask/binding logic.
Future-work: do the same for calls from VrShell -> Gl thread (VrUiInterface). 

BUG=713377
==========

to

==========
Pause drawing webvr when the App button is pressed

Clicking the App button toggles between pausing and resuming webvr drawing. This
is the first step to crbug.com/713377. We still need to figure out how to
properly
handle app button clicks from WebVr presentation mode. This CL mainly has all
the
plumbing required for the UiSceneManager to handle the clicks.

This CL declares a VrBrowserInterface that VrGLThread implements. VrShellLGl
gets
this interface and calls functions on it which has the PostTask/binding logic.
Future-work: do the same for calls from VrShell -> Gl thread (VrUiInterface). 

BUG=713377

Review-Url: https://codereview.chromium.org/2833773005
Cr-Commit-Position: refs/heads/master@{#469138}
Committed:
https://chromium.googlesource.com/chromium/src/+/847ddc166e042a5e929ac16c9327...
==========

[commit-bot: I haz the power, 2017-05-03 21:44:07]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/847ddc166e042a5e929ac16c9327...