Implement WebVR presentation pausing for VR Shell Menu Mode

Implement WebVR presentation pausing for VR Shell Menu Mode

This CL also passes VRDeviceProvider around as a WeakPtr, removes unsafe thread hopping, and fixes some races in VR Shell init/teardown.

BUG=644565
Committed: https://crrev.com/6b438bcc55da335795a7aa4b53ae590cd8c71eee
Cr-Commit-Position: refs/heads/master@{#431939}

[mthiesse, 2016-11-01 15:52:47]

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

[mthiesse, 2016-11-01 15:52:47]

PTAL

[bajones, 2016-11-01 18:11:32]

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

https://codereview.chromium.org/2471433002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:288: // TODO(mthiesse): Also pause
tracking for all webvr pages.
Is this comment accurate? Seems like we're already doing that?

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

https://codereview.chromium.org/2471433002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_delegate.cc:9: #include
"device/vr/android/gvr/gvr_device_provider.h"
Is this necessary?

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

https://codereview.chromium.org/2471433002/diff/1/device/vr/vr_service.mojom#...
device/vr/vr_service.mojom:87: OnResumePresent(uint32 index);
Let's prefer the verbiage defined by the spec here, which is blur/focus.
(https://w3c.github.io/webvr/#window-onvrdisplayblur-event) So these should be
OnDisplayBlur and OnDisplayFocus.

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

https://codereview.chromium.org/2471433002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:183:
m_scriptedAnimationController->suspend();
This would be the perfect place to fire the "vrdisplayblur" event. Please do so!
(https://w3c.github.io/webvr/#window-onvrdisplayblur-event)

https://codereview.chromium.org/2471433002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:191:
doc->resumeScheduledTasks();
Likewise, let's fire "vrdisplayfocus" here.

[bshe, 2016-11-01 19:29:31]

On 2016/11/01 18:11:32, bajones wrote:
>
https://codereview.chromium.org/2471433002/diff/1/chrome/browser/android/vr_s...
> File chrome/browser/android/vr_shell/vr_shell.cc (right):
> 
>
https://codereview.chromium.org/2471433002/diff/1/chrome/browser/android/vr_s...
> chrome/browser/android/vr_shell/vr_shell.cc:288: // TODO(mthiesse): Also pause
> tracking for all webvr pages.
> Is this comment accurate? Seems like we're already doing that?
> 
>
https://codereview.chromium.org/2471433002/diff/1/chrome/browser/android/vr_s...
> File chrome/browser/android/vr_shell/vr_shell_delegate.cc (right):
> 
>
https://codereview.chromium.org/2471433002/diff/1/chrome/browser/android/vr_s...
> chrome/browser/android/vr_shell/vr_shell_delegate.cc:9: #include
> "device/vr/android/gvr/gvr_device_provider.h"
> Is this necessary?
> 
> https://codereview.chromium.org/2471433002/diff/1/device/vr/vr_service.mojom
> File device/vr/vr_service.mojom (right):
> 
>
https://codereview.chromium.org/2471433002/diff/1/device/vr/vr_service.mojom#...
> device/vr/vr_service.mojom:87: OnResumePresent(uint32 index);
> Let's prefer the verbiage defined by the spec here, which is blur/focus.
> (https://w3c.github.io/webvr/#window-onvrdisplayblur-event) So these should be
> OnDisplayBlur and OnDisplayFocus.
> 
>
https://codereview.chromium.org/2471433002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):
> 
>
https://codereview.chromium.org/2471433002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/vr/VRDisplay.cpp:183:
> m_scriptedAnimationController->suspend();
> This would be the perfect place to fire the "vrdisplayblur" event. Please do
so!
> (https://w3c.github.io/webvr/#window-onvrdisplayblur-event)
> 
>
https://codereview.chromium.org/2471433002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/vr/VRDisplay.cpp:191:
> doc->resumeScheduledTasks();
> Likewise, let's fire "vrdisplayfocus" here.

vr_shell lgtm with Brandon's review addressed.

[mthiesse, 2016-11-02 14:39:02]

I also went ahead and fixed a bunch of rendering bugs in vr_shell.cc around mode
switching that this exposes.

PTAL

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

https://codereview.chromium.org/2471433002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:288: // TODO(mthiesse): Also pause
tracking for all webvr pages.
On 2016/11/01 18:11:31, bajones wrote:
> Is this comment accurate? Seems like we're already doing that?

As discussed offline, we're going to ignore the non-presenting WebVR case for
now as it's causing a lot of problems and we may not ship with it.

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

https://codereview.chromium.org/2471433002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_delegate.cc:9: #include
"device/vr/android/gvr/gvr_device_provider.h"
On 2016/11/01 18:11:31, bajones wrote:
> Is this necessary?

Yes, it was previously implicitly included by another dependency.

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

https://codereview.chromium.org/2471433002/diff/1/device/vr/vr_service.mojom#...
device/vr/vr_service.mojom:87: OnResumePresent(uint32 index);
On 2016/11/01 18:11:31, bajones wrote:
> Let's prefer the verbiage defined by the spec here, which is blur/focus.
> (https://w3c.github.io/webvr/#window-onvrdisplayblur-event) So these should be
> OnDisplayBlur and OnDisplayFocus.

Done.

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

https://codereview.chromium.org/2471433002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:183:
m_scriptedAnimationController->suspend();
On 2016/11/01 18:11:31, bajones wrote:
> This would be the perfect place to fire the "vrdisplayblur" event. Please do
so!
> (https://w3c.github.io/webvr/#window-onvrdisplayblur-event)

Done.

https://codereview.chromium.org/2471433002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:191:
doc->resumeScheduledTasks();
On 2016/11/01 18:11:31, bajones wrote:
> Likewise, let's fire "vrdisplayfocus" here.

Done.

[mthiesse, 2016-11-02 14:53:08]

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

[mthiesse, 2016-11-02 14:53:09]

dcheng@chromium.org: Please review .mojom changes and
third_party/WebKit/Source/core/ changes.

[bshe, 2016-11-02 14:58:40]

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

https://codereview.chromium.org/2471433002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:288: // TODO(mthiesse): Also pause
tracking for all webvr pages.
On 2016/11/02 14:39:02, mthiesse wrote:
> On 2016/11/01 18:11:31, bajones wrote:
> > Is this comment accurate? Seems like we're already doing that?
> 
> As discussed offline, we're going to ignore the non-presenting WebVR case for
> now as it's causing a lot of problems and we may not ship with it.

Does this mean magic window mode won't ship in normal 2d browser? Or do you mean
it won't be available in ChromeVR?

[mthiesse, 2016-11-02 15:03:32]

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

https://codereview.chromium.org/2471433002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:288: // TODO(mthiesse): Also pause
tracking for all webvr pages.
On 2016/11/02 14:58:40, bshe wrote:
> On 2016/11/02 14:39:02, mthiesse wrote:
> > On 2016/11/01 18:11:31, bajones wrote:
> > > Is this comment accurate? Seems like we're already doing that?
> > 
> > As discussed offline, we're going to ignore the non-presenting WebVR case
for
> > now as it's causing a lot of problems and we may not ship with it.
> 
> Does this mean magic window mode won't ship in normal 2d browser? Or do you
mean
> it won't be available in ChromeVR?

It doesn't mean either of those things. It means we haven't decided yet, so I'm
going to hold off on writing the code to pause WebVR for now. Right now we don't
even have a way to know which Device VR Shell is using, so we don't have a way
to find all of the WebVR pages using the same device.

[bajones, 2016-11-02 17:55:06]

LGTM with one non-blocking question.

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

https://codereview.chromium.org/2471433002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:532: glClearColor(0.1f, 0.1f, 0.1f,
0.0f);
What's the reason for clearing to a dark grey instead of black? It's all
transparent, so won't be visible. Just seems like an odd choice.

[mthiesse, 2016-11-02 18:02:53]

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

https://codereview.chromium.org/2471433002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:532: glClearColor(0.1f, 0.1f, 0.1f,
0.0f);
On 2016/11/02 17:55:06, bajones wrote:
> What's the reason for clearing to a dark grey instead of black? It's all
> transparent, so won't be visible. Just seems like an odd choice.

No reason, I'll make it black.

[mthiesse, 2016-11-02 18:03:47]

Patchset #3 (id:40001) has been deleted

[dcheng, 2016-11-04 05:24:46]

https://codereview.chromium.org/2471433002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (left):

https://codereview.chromium.org/2471433002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:489: if (!webvr_mode_) {
Should we get rid of |webvr_mode_|?

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

https://codereview.chromium.org/2471433002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_delegate.cc:9: #include
"device/vr/android/gvr/gvr_device_provider.h"
This include doesn't seem necessary

https://codereview.chromium.org/2471433002/diff/80001/device/vr/android/gvr/g...
File device/vr/android/gvr/gvr_device_provider.cc (right):

https://codereview.chromium.org/2471433002/diff/80001/device/vr/android/gvr/g...
device/vr/android/gvr/gvr_device_provider.cc:114:
base::Bind(&GvrDeviceProvider::OnDisplayBlur, base::Unretained(this)));
Can you explain why this used of Unretained() is safe?

Also, why do we need to bounce threads? There's no comments on this class about
what threads it runs on or the threading requirements.

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

https://codereview.chromium.org/2471433002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:190:
doc->suspendScheduledTasks();
Why do we want to suspend scheduled tasks? This will stop timers and other
things on the page from running.

[mthiesse, 2016-11-04 17:20:36]

(No updated patchset yet, I'm at a conference, just responding to comments)

https://codereview.chromium.org/2471433002/diff/80001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (left):

https://codereview.chromium.org/2471433002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:489: if (!webvr_mode_) {
On 2016/11/04 05:24:46, dcheng wrote:
> Should we get rid of |webvr_mode_|?

Right now it's used to know which mode to return to when we exit menu mode. I'll
probably remove it in a future refactor, but for now it's still useful.

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

https://codereview.chromium.org/2471433002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_delegate.cc:9: #include
"device/vr/android/gvr/gvr_device_provider.h"
On 2016/11/04 05:24:46, dcheng wrote:
> This include doesn't seem necessary

It was being indirectly included by gvr_delegate.h in the header for this file.
We definitely use it in this file, so we should include it here.

https://codereview.chromium.org/2471433002/diff/80001/device/vr/android/gvr/g...
File device/vr/android/gvr/gvr_device_provider.cc (right):

https://codereview.chromium.org/2471433002/diff/80001/device/vr/android/gvr/g...
device/vr/android/gvr/gvr_device_provider.cc:114:
base::Bind(&GvrDeviceProvider::OnDisplayBlur, base::Unretained(this)));
On 2016/11/04 05:24:46, dcheng wrote:
> Can you explain why this used of Unretained() is safe?
> 
> Also, why do we need to bounce threads? There's no comments on this class
about
> what threads it runs on or the threading requirements.

As Brandon wrote this class and I was really just following the pattern of
OnGvrDelegateReady, perhaps he'll give a better answer.

I'm not actually sure base::Unretained() is safe here. What's the best
alternative? Making this class refcounted and using base::RetainedRef()?

I'll add some comments regarding threading in the next patchset, but briefly,
OnDisplayFocus, OnDisplayBlur, and OnGvrDelegateReady are called from an
arbitrary GL thread, and everything else is called from the main thread. We need
to post to the main thread because the VRClientDispatcher can only be used from
the main thread. Whether or not the thread bounding should happen here or at the
callsite is a style question I don't know the correct answer to (maybe you have
strong opinions?).

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

https://codereview.chromium.org/2471433002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:190:
doc->suspendScheduledTasks();
On 2016/11/04 05:24:46, dcheng wrote:
> Why do we want to suspend scheduled tasks? This will stop timers and other
> things on the page from running.

I'll remove this in the next patchset. My thinking was that this page will not
be visible, and one of the reasons for the user hitting the menu button is the
page was providing a bad experience (like using too much of the system resources
and causing low framerates, leading to VR sickness), so suspending the page's
tasks would help with this.

However, I'm not sure this is the right approach, and this is not the right
change to do this in anyways. If I choose to move forward with something like
this I'll post a followup CL.

[mthiesse, 2016-11-08 21:04:09]

dcheng, bajones PTAL

I changed how we pause the render loop, and made VRDeviceProvider refcounted so
we can safely use it across threads.

[dcheng, 2016-11-08 23:05:45]

Also, I don't think I follow why this has to be refcounted now; if the CL
description could be updated to explain this CL's changes in more detail, that
would be good.

Also, +shaobao who's doing a large refactoring around vr display

https://codereview.chromium.org/2471433002/diff/100001/device/vr/vr_device_ma...
File device/vr/vr_device_manager.cc (right):

https://codereview.chromium.org/2471433002/diff/100001/device/vr/vr_device_ma...
device/vr/vr_device_manager.cc:36:
VRDeviceManager::VRDeviceManager(VRDeviceProvider* provider)
This should be a scoped_refptr<VRDeviceProvider>.

https://codereview.chromium.org/2471433002/diff/100001/device/vr/vr_device_ma...
device/vr/vr_device_manager.cc:327: void
VRDeviceManager::RegisterProvider(VRDeviceProvider* provider) {
Ditto: this should also be a scoped_refptr<VRDeviceProvider>.

To avoid extra refs, the ctor and this function should std::move(provider)

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

https://codereview.chromium.org/2471433002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:140: m_framePose = nullptr;
Out of curiosity, why don't we just clear this in onDisplayBlur()?

VRDisplay seems like it's a state machine, so if it's possible to add some more
comments about how the different parts interact, that would help readability.
For someone who's not familiar with the WebVR spec, it can be difficult to
follow the flow of logic.

(Comments and/or pointers to the spec in the implementation can be done in a
followup though)

[mthiesse, 2016-11-08 23:12:25]

On 2016/11/08 23:05:45, dcheng wrote:
> Also, I don't think I follow why this has to be refcounted now; if the CL
> description could be updated to explain this CL's changes in more detail, that
> would be good.

Maybe there's something obvious I'm missing here, but this was to avoid binding
to base::Unretained(this). Is there a good alternative I should be using for
this purpose?

[dcheng, 2016-11-08 23:27:56]

On 2016/11/08 23:12:25, mthiesse wrote:
> On 2016/11/08 23:05:45, dcheng wrote:
> > Also, I don't think I follow why this has to be refcounted now; if the CL
> > description could be updated to explain this CL's changes in more detail,
that
> > would be good.
> 
> Maybe there's something obvious I'm missing here, but this was to avoid
binding
> to base::Unretained(this). Is there a good alternative I should be using for
> this purpose?

Well, I'm not familiar enough with the code to say, but an alternative is using
weak pointers (though this code needs to be careful, since it crosses thread
boundaries). But cancellation might be a more expected default behavior here
(otherwise, having a task posted will keep the object alive, when other things
might have expected it to already have cleaned up?)

[mthiesse, 2016-11-10 00:36:14]

In trying to address your comments, I've realized I'm inadvertently embarking
upon many of the same refactorings shaobo has already done in his patch.

I'm going to wait for shaobo to land his patch, then rebase onto it and continue
from there.

[mthiesse, 2016-11-10 22:10:37]

Description was changed from

==========
Implement WebVR presentation pausing for VR Shell Menu Mode

BUG=644565
==========

to

==========
Implement WebVR presentation pausing for VR Shell Menu Mode

This CL also makes VRDeviceProvider RefCountedThreadSafe, as it was being used
unsafely from various GL threads and main thread.

BUG=644565
==========

[mthiesse, 2016-11-10 22:14:10]

PTAL bajones, dcheng.

bajones and I discussed, and agreed that RefCountedThreadSafe was the right
choice for VRDeviceProvider. It handles events that come in from various GL
threads, and nothing expects it to go away at any specific time. I looked into
using WeakPtrs, but couldn't come up with any way to do so safely.

https://codereview.chromium.org/2471433002/diff/100001/device/vr/vr_device_ma...
File device/vr/vr_device_manager.cc (right):

https://codereview.chromium.org/2471433002/diff/100001/device/vr/vr_device_ma...
device/vr/vr_device_manager.cc:36:
VRDeviceManager::VRDeviceManager(VRDeviceProvider* provider)
On 2016/11/08 23:05:45, dcheng wrote:
> This should be a scoped_refptr<VRDeviceProvider>.

Done.

https://codereview.chromium.org/2471433002/diff/100001/device/vr/vr_device_ma...
device/vr/vr_device_manager.cc:327: void
VRDeviceManager::RegisterProvider(VRDeviceProvider* provider) {
On 2016/11/08 23:05:45, dcheng wrote:
> Ditto: this should also be a scoped_refptr<VRDeviceProvider>.
> 
> To avoid extra refs, the ctor and this function should std::move(provider)

Done.

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

https://codereview.chromium.org/2471433002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:140: m_framePose = nullptr;
On 2016/11/08 23:05:45, dcheng wrote:
> Out of curiosity, why don't we just clear this in onDisplayBlur()?
> 
> VRDisplay seems like it's a state machine, so if it's possible to add some
more
> comments about how the different parts interact, that would help readability.
> For someone who's not familiar with the WebVR spec, it can be difficult to
> follow the flow of logic.
> 
> (Comments and/or pointers to the spec in the implementation can be done in a
> followup though)

I clear it in updatePose simply for code clarity. The spec says to return a null
pose when the display is blurred.

I'll add a few comments for the parts I touched. I'll leave it to bajones@ to
improve readability on the rest, as I myself am not very familiar with spec and
added these changes on his request.

[dcheng, 2016-11-11 00:48:21]

On 2016/11/10 22:14:10, mthiesse wrote:
> PTAL bajones, dcheng.
> 
> bajones and I discussed, and agreed that RefCountedThreadSafe was the right
> choice for VRDeviceProvider. It handles events that come in from various GL
> threads, and nothing expects it to go away at any specific time. I looked into
> using WeakPtrs, but couldn't come up with any way to do so safely.

The important thing about WeakPtrs is that they must be dereferenced and
invalidated on the same thread, so it should be safe to do it here. Since it
sounds like we're posting tasks from any number of threads, we need to eagerly
bind a WeakPtr when we construct the VRDeviceProvider, and save the WeakPtr in a
class member. When tasks are posted, they can take a copy of that WeakPtr (on
any thread), which is OK, as long as the WeakPtr is only derefed on the main
task runner.

From reading the current CL, it's not clear to me why we'd want to keep invoking
these callbacks if the VRDeviceProvider is going away (i.e. the only references
left to it are from posted tasks): it looks like we have to add some null checks
now to make that safe, when we get there for free with WeakPtr cancellation.

> 
>
https://codereview.chromium.org/2471433002/diff/100001/device/vr/vr_device_ma...
> File device/vr/vr_device_manager.cc (right):
> 
>
https://codereview.chromium.org/2471433002/diff/100001/device/vr/vr_device_ma...
> device/vr/vr_device_manager.cc:36:
> VRDeviceManager::VRDeviceManager(VRDeviceProvider* provider)
> On 2016/11/08 23:05:45, dcheng wrote:
> > This should be a scoped_refptr<VRDeviceProvider>.
> 
> Done.
> 
>
https://codereview.chromium.org/2471433002/diff/100001/device/vr/vr_device_ma...
> device/vr/vr_device_manager.cc:327: void
> VRDeviceManager::RegisterProvider(VRDeviceProvider* provider) {
> On 2016/11/08 23:05:45, dcheng wrote:
> > Ditto: this should also be a scoped_refptr<VRDeviceProvider>.
> > 
> > To avoid extra refs, the ctor and this function should std::move(provider)
> 
> Done.
> 
>
https://codereview.chromium.org/2471433002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):
> 
>
https://codereview.chromium.org/2471433002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/vr/VRDisplay.cpp:140: m_framePose = nullptr;
> On 2016/11/08 23:05:45, dcheng wrote:
> > Out of curiosity, why don't we just clear this in onDisplayBlur()?
> > 
> > VRDisplay seems like it's a state machine, so if it's possible to add some
> more
> > comments about how the different parts interact, that would help
readability.
> > For someone who's not familiar with the WebVR spec, it can be difficult to
> > follow the flow of logic.
> > 
> > (Comments and/or pointers to the spec in the implementation can be done in a
> > followup though)
> 
> I clear it in updatePose simply for code clarity. The spec says to return a
null
> pose when the display is blurred.
> 
> I'll add a few comments for the parts I touched. I'll leave it to bajones@ to
> improve readability on the rest, as I myself am not very familiar with spec
and
> added these changes on his request.

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

https://codereview.chromium.org/2471433002/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_delegate.cc:73:
DCHECK(!device_provider_.get());
Nit: no .get()

https://codereview.chromium.org/2471433002/diff/160001/device/vr/vr_device.cc
File device/vr/vr_device.cc (right):

https://codereview.chromium.org/2471433002/diff/160001/device/vr/vr_device.cc...
device/vr/vr_device.cc:75: if (client)
Does this need to null check? I believe that we bind VRdisplayClient as we
create a VRDisplayImpl, so I would not expect this to ever be non-null

(I would expect a display to be removed from the list of displays_ when it's
disconnected, but maybe I'm not understanding something here)

[mthiesse, 2016-11-11 20:17:21]

Patchset #9 (id:180001) has been deleted

[mthiesse, 2016-11-11 20:19:21]

Ah, I finally understand the approach.

I've refactored to use weak pointers. There's a lot more refactoring I want to
do, now that I'm developing an understanding of this code, but this is more than
enough for one CL :P

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

https://codereview.chromium.org/2471433002/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_delegate.cc:73:
DCHECK(!device_provider_.get());
On 2016/11/11 00:48:21, dcheng wrote:
> Nit: no .get()

Done.

https://codereview.chromium.org/2471433002/diff/160001/device/vr/vr_device.cc
File device/vr/vr_device.cc (right):

https://codereview.chromium.org/2471433002/diff/160001/device/vr/vr_device.cc...
device/vr/vr_device.cc:75: if (client)
On 2016/11/11 00:48:21, dcheng wrote:
> Does this need to null check? I believe that we bind VRdisplayClient as we
> create a VRDisplayImpl, so I would not expect this to ever be non-null
> 
> (I would expect a display to be removed from the list of displays_ when it's
> disconnected, but maybe I'm not understanding something here)

Done.

[mthiesse, 2016-11-11 20:20:42]

Description was changed from

==========
Implement WebVR presentation pausing for VR Shell Menu Mode

This CL also makes VRDeviceProvider RefCountedThreadSafe, as it was being used
unsafely from various GL threads and main thread.

BUG=644565
==========

to

==========
Implement WebVR presentation pausing for VR Shell Menu Mode

This CL also passes VRDeviceProvider around as a WeakPtr, removes unsafe thread
hopping, and fixes some races in VR Shell init/teardown.

BUG=644565
==========

[mthiesse, 2016-11-11 20:27:46]

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

[mthiesse, 2016-11-11 20:27:47]

tedchoc@chromium.org: Please review changes in VrShellImpl.java

[Ted C, 2016-11-11 22:00:28]

On 2016/11/11 20:27:47, mthiesse wrote:
> mailto:tedchoc@chromium.org: Please review changes in VrShellImpl.java

VrShellImpl.java - lgtm

[dcheng, 2016-11-12 02:03:31]

mojo LGTM

Just some optional minor cleanups (and please pass WeakPtr by const ref... in
theory we could pass by value and move, but that's not currently used as the
convention)

https://codereview.chromium.org/2471433002/diff/200001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell_delegate.cc (right):

https://codereview.chromium.org/2471433002/diff/200001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_delegate.cc:105: return
reinterpret_cast<GvrNonPresentingDelegate*>(
Nit: static_cast for downcasting (reinterpret_cast is usually for casting
between unrelated types)

https://codereview.chromium.org/2471433002/diff/200001/device/vr/android/gvr/...
File device/vr/android/gvr/gvr_device.h (right):

https://codereview.chromium.org/2471433002/diff/200001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device.h:23: GvrDevice(GvrDeviceProvider* provider,
base::WeakPtr<GvrDelegate> delegate);
Nit: const ref.

https://codereview.chromium.org/2471433002/diff/200001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device.h:39: void
SetDelegate(base::WeakPtr<GvrDelegate> delegate);
Ditto.

https://codereview.chromium.org/2471433002/diff/200001/device/vr/android/gvr/...
File device/vr/android/gvr/gvr_device_provider.cc (right):

https://codereview.chromium.org/2471433002/diff/200001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device_provider.cc:84: base::WeakPtr<GvrDelegate>
delegate) {
Nit: const ref

https://codereview.chromium.org/2471433002/diff/200001/device/vr/android/gvr/...
File device/vr/android/gvr/gvr_gamepad_data_fetcher.cc (right):

https://codereview.chromium.org/2471433002/diff/200001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_gamepad_data_fetcher.cc:40: return
std::unique_ptr<GamepadDataFetcher>(nullptr);
Nit: Just return nullptr_ should be sufficient here (and the next line should be
base::MakeUnique<...>(...) if you're up for a little local cleanup.

https://codereview.chromium.org/2471433002/diff/200001/device/vr/android/gvr/...
File device/vr/android/gvr/gvr_gamepad_data_fetcher.h (right):

https://codereview.chromium.org/2471433002/diff/200001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_gamepad_data_fetcher.h:23:
Factory(base::WeakPtr<GvrDelegate> delegate, unsigned int display_id);
Nit: const base::WeakPtr<>&

https://codereview.chromium.org/2471433002/diff/200001/device/vr/vr_device_ma...
File device/vr/vr_device_manager.cc (right):

https://codereview.chromium.org/2471433002/diff/200001/device/vr/vr_device_ma...
device/vr/vr_device_manager.cc:30: RegisterProvider(base::WrapUnique(new
GvrDeviceProvider()));
Nit: base::MakeUnique<GvrDeviceProvider>()

[mthiesse, 2016-11-14 16:41:39]

https://codereview.chromium.org/2471433002/diff/200001/chrome/browser/android...
File chrome/browser/android/vr_shell/vr_shell_delegate.cc (right):

https://codereview.chromium.org/2471433002/diff/200001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_delegate.cc:105: return
reinterpret_cast<GvrNonPresentingDelegate*>(
On 2016/11/12 02:03:31, dcheng wrote:
> Nit: static_cast for downcasting (reinterpret_cast is usually for casting
> between unrelated types)

Done.

https://codereview.chromium.org/2471433002/diff/200001/device/vr/android/gvr/...
File device/vr/android/gvr/gvr_device.h (right):

https://codereview.chromium.org/2471433002/diff/200001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device.h:23: GvrDevice(GvrDeviceProvider* provider,
base::WeakPtr<GvrDelegate> delegate);
On 2016/11/12 02:03:31, dcheng wrote:
> Nit: const ref.

Done.

https://codereview.chromium.org/2471433002/diff/200001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device.h:39: void
SetDelegate(base::WeakPtr<GvrDelegate> delegate);
On 2016/11/12 02:03:31, dcheng wrote:
> Ditto.

Done.

https://codereview.chromium.org/2471433002/diff/200001/device/vr/android/gvr/...
File device/vr/android/gvr/gvr_device_provider.cc (right):

https://codereview.chromium.org/2471433002/diff/200001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_device_provider.cc:84: base::WeakPtr<GvrDelegate>
delegate) {
On 2016/11/12 02:03:31, dcheng wrote:
> Nit: const ref

Done.

https://codereview.chromium.org/2471433002/diff/200001/device/vr/android/gvr/...
File device/vr/android/gvr/gvr_gamepad_data_fetcher.cc (right):

https://codereview.chromium.org/2471433002/diff/200001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_gamepad_data_fetcher.cc:40: return
std::unique_ptr<GamepadDataFetcher>(nullptr);
On 2016/11/12 02:03:31, dcheng wrote:
> Nit: Just return nullptr_ should be sufficient here (and the next line should
be
> base::MakeUnique<...>(...) if you're up for a little local cleanup.

Done.

https://codereview.chromium.org/2471433002/diff/200001/device/vr/android/gvr/...
File device/vr/android/gvr/gvr_gamepad_data_fetcher.h (right):

https://codereview.chromium.org/2471433002/diff/200001/device/vr/android/gvr/...
device/vr/android/gvr/gvr_gamepad_data_fetcher.h:23:
Factory(base::WeakPtr<GvrDelegate> delegate, unsigned int display_id);
On 2016/11/12 02:03:31, dcheng wrote:
> Nit: const base::WeakPtr<>&

Done.

https://codereview.chromium.org/2471433002/diff/200001/device/vr/vr_device_ma...
File device/vr/vr_device_manager.cc (right):

https://codereview.chromium.org/2471433002/diff/200001/device/vr/vr_device_ma...
device/vr/vr_device_manager.cc:30: RegisterProvider(base::WrapUnique(new
GvrDeviceProvider()));
On 2016/11/12 02:03:31, dcheng wrote:
> Nit: base::MakeUnique<GvrDeviceProvider>()

Done.

[mthiesse, 2016-11-14 20:27:20]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2016-11-14 20:27:21]

The patchset sent to the CQ was uploaded after l-g-t-m from bshe@chromium.org,
bajones@chromium.org, tedchoc@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2471433002/#ps240001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-14 20:28:56]

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, 2016-11-14 22:56:29]

Description was changed from

==========
Implement WebVR presentation pausing for VR Shell Menu Mode

This CL also passes VRDeviceProvider around as a WeakPtr, removes unsafe thread
hopping, and fixes some races in VR Shell init/teardown.

BUG=644565
==========

to

==========
Implement WebVR presentation pausing for VR Shell Menu Mode

This CL also passes VRDeviceProvider around as a WeakPtr, removes unsafe thread
hopping, and fixes some races in VR Shell init/teardown.

BUG=644565
==========

[commit-bot: I haz the power, 2016-11-14 22:56:30]

Committed patchset #11 (id:240001)

[commit-bot: I haz the power, 2016-11-14 22:59:16]

Description was changed from

==========
Implement WebVR presentation pausing for VR Shell Menu Mode

This CL also passes VRDeviceProvider around as a WeakPtr, removes unsafe thread
hopping, and fixes some races in VR Shell init/teardown.

BUG=644565
==========

to

==========
Implement WebVR presentation pausing for VR Shell Menu Mode

This CL also passes VRDeviceProvider around as a WeakPtr, removes unsafe thread
hopping, and fixes some races in VR Shell init/teardown.

BUG=644565

Committed: https://crrev.com/6b438bcc55da335795a7aa4b53ae590cd8c71eee
Cr-Commit-Position: refs/heads/master@{#431939}
==========

[commit-bot: I haz the power, 2016-11-14 22:59:17]

Patchset 11 (id:??) landed as
https://crrev.com/6b438bcc55da335795a7aa4b53ae590cd8c71eee
Cr-Commit-Position: refs/heads/master@{#431939}