Clean up some VrShell threading issues and remove unnecessary WeakPtr types.

Clean up some VrShell threading issues and remove unnecessary WeakPtr types.

I originally added the WeakPtrs all over the place so that we could post to the device provider from the GL thread. This CL moves the calls to the UI thread, and removes the WeakPtrs with it.

BUG=671302
Committed: https://crrev.com/2bcae0c61cb693232a4cf83dd4dcd175463e9bd4
Cr-Commit-Position: refs/heads/master@{#438870}

[mthiesse, 2016-12-13 20:01:09]

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

[mthiesse, 2016-12-13 20:01:10]

Klaus, Biao, can you please review this as Brandon's on leave?

[mthiesse, 2016-12-13 20:16:34]

Description was changed from

==========
Clean up some VrShell threading issues and remove unnecessary WeakPtr types.

BUG=671302
==========

to

==========
Clean up some VrShell threading issues and remove unnecessary WeakPtr types.

I originally added the WeakPtrs all over the place so that we could post to the
device provider from the GL thread. This CL moves the calls to the UI thread,
and removes the WeakPtrs with it. 

BUG=671302
==========

[bshe, 2016-12-14 16:06:46]

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

https://codereview.chromium.org/2570553004/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:45: GLThread(VrShell* vr_shell,
const base::WeakPtr<VrShell>& weak_vr_shell,
looks like you can remove vr_shell too?

https://codereview.chromium.org/2570553004/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:265:
base::Bind(&VrShellGl::SetWebVrMode, thread->GetVrShellGl(), enabled));
is thread->GetVrShellGl() evaluated when the task actually runs? If not,
wouldn't it be null and the task is just ignored?

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

https://codereview.chromium.org/2570553004/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:926: void
VrShellGl::SetWebVrMode(bool enabled) {
Is this function ever being called?

[mthiesse, 2016-12-14 16:20:43]

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

https://codereview.chromium.org/2570553004/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:45: GLThread(VrShell* vr_shell,
const base::WeakPtr<VrShell>& weak_vr_shell,
On 2016/12/14 16:06:46, bshe wrote:
> looks like you can remove vr_shell too?

Done.

https://codereview.chromium.org/2570553004/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:265:
base::Bind(&VrShellGl::SetWebVrMode, thread->GetVrShellGl(), enabled));
On 2016/12/14 16:06:46, bshe wrote:
> is thread->GetVrShellGl() evaluated when the task actually runs? If not,
> wouldn't it be null and the task is just ignored?

If the WeakPtr at thread->GetVrShellGl() is null, the task will get ignored. I
think this is what we want in this case.

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

https://codereview.chromium.org/2570553004/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:926: void
VrShellGl::SetWebVrMode(bool enabled) {
On 2016/12/14 16:06:46, bshe wrote:
> Is this function ever being called?

Yes, VrShell::SetWebVrMode posts to it.

[bshe, 2016-12-14 17:36:51]

lgtm

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

https://codereview.chromium.org/2570553004/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:265:
base::Bind(&VrShellGl::SetWebVrMode, thread->GetVrShellGl(), enabled));
On 2016/12/14 16:20:43, mthiesse wrote:
> On 2016/12/14 16:06:46, bshe wrote:
> > is thread->GetVrShellGl() evaluated when the task actually runs? If not,
> > wouldn't it be null and the task is just ignored?
> 
> If the WeakPtr at thread->GetVrShellGl() is null, the task will get ignored. I
> think this is what we want in this case.

For optional tasks, I would prefer to use IsRunning method to make it
more readable. But this is not blocking.

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

https://codereview.chromium.org/2570553004/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:926: void
VrShellGl::SetWebVrMode(bool enabled) {
On 2016/12/14 16:20:43, mthiesse wrote:
> On 2016/12/14 16:06:46, bshe wrote:
> > Is this function ever being called?
> 
> Yes, VrShell::SetWebVrMode posts to it.

dooh. Missed that...

[mthiesse, 2016-12-14 21:25:37]

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

https://codereview.chromium.org/2570553004/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:265:
base::Bind(&VrShellGl::SetWebVrMode, thread->GetVrShellGl(), enabled));
On 2016/12/14 17:36:51, bshe wrote:
> On 2016/12/14 16:20:43, mthiesse wrote:
> > On 2016/12/14 16:06:46, bshe wrote:
> > > is thread->GetVrShellGl() evaluated when the task actually runs? If not,
> > > wouldn't it be null and the task is just ignored?
> > 
> > If the WeakPtr at thread->GetVrShellGl() is null, the task will get ignored.
I
> > think this is what we want in this case.
> 
> For optional tasks, I would prefer to use IsRunning method to make it
> more readable. But this is not blocking.

Done.

[mthiesse, 2016-12-14 21:26:47]

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

[mthiesse, 2016-12-14 21:26:48]

rockot@chromium.org: Please OWNERS review changes in device/

[Ken Rockot(use gerrit already), 2016-12-14 21:36:01]

Please use an owner from the more specific device/vr/OWNERS list

[mthiesse, 2016-12-14 21:38:23]

mthiesse@chromium.org changed reviewers:
+ kbr@chromium.org
- rockot@chromium.org

[mthiesse, 2016-12-14 21:38:24]

kbr@chromium.org, please OWNERS review device/vr/ changes.

[mthiesse, 2016-12-15 17:10:53]

mthiesse@chromium.org changed reviewers:
+ hendrikw@chromium.org
- kbr@chromium.org

[mthiesse, 2016-12-15 17:10:54]

-kbr who appears to be on vacation.

hendrikw, please OWNERS review device/vr changes.

[bajones, 2016-12-15 17:17:17]

On 2016/12/15 17:10:54, mthiesse wrote:
> -kbr who appears to be on vacation.
> 
> hendrikw, please OWNERS review device/vr changes.

Not sure if Hendrik is in a position to review either. device/VR LGTM.

[mthiesse, 2016-12-15 17:20:33]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2016-12-15 17:20:34]

The patchset sent to the CQ was uploaded after l-g-t-m from bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/2570553004/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-15 17:21:07]

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-12-15 17:50:41]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481822433596520,
"parent_rev": "b65febb51e0ede9ec727268f945721fc99f09dc7", "commit_rev":
"16a78f635da2d44df4303101c837a17ce15437dd"}

[commit-bot: I haz the power, 2016-12-15 17:51:06]

Description was changed from

==========
Clean up some VrShell threading issues and remove unnecessary WeakPtr types.

I originally added the WeakPtrs all over the place so that we could post to the
device provider from the GL thread. This CL moves the calls to the UI thread,
and removes the WeakPtrs with it. 

BUG=671302
==========

to

==========
Clean up some VrShell threading issues and remove unnecessary WeakPtr types.

I originally added the WeakPtrs all over the place so that we could post to the
device provider from the GL thread. This CL moves the calls to the UI thread,
and removes the WeakPtrs with it. 

BUG=671302

Review-Url: https://codereview.chromium.org/2570553004
==========

[commit-bot: I haz the power, 2016-12-15 17:51:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-15 17:53:32]

Description was changed from

==========
Clean up some VrShell threading issues and remove unnecessary WeakPtr types.

I originally added the WeakPtrs all over the place so that we could post to the
device provider from the GL thread. This CL moves the calls to the UI thread,
and removes the WeakPtrs with it. 

BUG=671302

Review-Url: https://codereview.chromium.org/2570553004
==========

to

==========
Clean up some VrShell threading issues and remove unnecessary WeakPtr types.

I originally added the WeakPtrs all over the place so that we could post to the
device provider from the GL thread. This CL moves the calls to the UI thread,
and removes the WeakPtrs with it.

BUG=671302

Committed: https://crrev.com/2bcae0c61cb693232a4cf83dd4dcd175463e9bd4
Cr-Commit-Position: refs/heads/master@{#438870}
==========

[commit-bot: I haz the power, 2016-12-15 17:53:33]

Patchset 4 (id:??) landed as
https://crrev.com/2bcae0c61cb693232a4cf83dd4dcd175463e9bd4
Cr-Commit-Position: refs/heads/master@{#438870}