Refactor GvrDelegate ownership into GvrDelegateProvider and fix more threading violations.

Refactor GvrDelegate ownership into GvrDelegateProvider and fix more threading violations.

This CL does two main things: It refactors controlling when the GvrDelegate is created and destroyed into the GvrDelegateProvider, and it refactors out some instances of using the GVR api in a non-threadsafe way.

Some minor functional changes may be visible with this patch - namely that we no longer (possibly) send a DisplayConnected event for a display that isn't ready yet due to the gvr api not being available. This should be an improvement in all cases.

BUG=674594, 655297
Review-Url: https://codereview.chromium.org/2658643003
Cr-Commit-Position: refs/heads/master@{#447643}
Committed: https://chromium.googlesource.com/chromium/src/+/4c6465c3b85e664ca700e1c04bebf637c7846243

[mthiesse, 2017-01-25 20:55:45]

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

[mthiesse, 2017-01-25 20:55:45]

PTAL

[mthiesse, 2017-01-25 20:58:33]

Description was changed from

==========
Refactor GvrDelate ownership into GvrDelegateProvider and fix more threading
violations.



BUG=674594, 655297
==========

to

==========
Refactor GvrDelate ownership into GvrDelegateProvider and fix more threading
violations.

This CL does two main things: It refactors controlling when the GvrDelegate is
created and destroyed into the GvrDelegateProvider, and it refactors out some
instances of using the GVR api in a non-threadsafe way.

Some minor functional changes may be visible with this patch - namely that we no
longer (possibly) send a DisplayConnected event for a display that isn't ready
yet due to the gvr api not being available. This should be an improvement in all
cases.

BUG=674594, 655297
==========

[mthiesse, 2017-01-25 20:59:54]

Description was changed from

==========
Refactor GvrDelate ownership into GvrDelegateProvider and fix more threading
violations.

This CL does two main things: It refactors controlling when the GvrDelegate is
created and destroyed into the GvrDelegateProvider, and it refactors out some
instances of using the GVR api in a non-threadsafe way.

Some minor functional changes may be visible with this patch - namely that we no
longer (possibly) send a DisplayConnected event for a display that isn't ready
yet due to the gvr api not being available. This should be an improvement in all
cases.

BUG=674594, 655297
==========

to

==========
Refactor GvrDelegate ownership into GvrDelegateProvider and fix more threading
violations.

This CL does two main things: It refactors controlling when the GvrDelegate is
created and destroyed into the GvrDelegateProvider, and it refactors out some
instances of using the GVR api in a non-threadsafe way.

Some minor functional changes may be visible with this patch - namely that we no
longer (possibly) send a DisplayConnected event for a display that isn't ready
yet due to the gvr api not being available. This should be an improvement in all
cases.

BUG=674594, 655297
==========

[mthiesse, 2017-01-25 21:36:12]

cc klaus, FYI.

[cjgrant, 2017-01-25 21:45:31]

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

[cjgrant, 2017-01-25 21:45:32]

A couple of nit-grade comments only...

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

https://codereview.chromium.org/2658643003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:481: gvr::BufferViewport
eye_viewport = gvr_api->CreateBufferViewport();
Looking at the near-duplicate blocks for left and right eye, is it worth a "for
(auto eye : {GVR_LEFT_EYE, GVR_RIGHT_EYE}) {" loops?  Looks like one conditional
to pick left_eye/right_eye, and the rest of the code is identical.

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

https://codereview.chromium.org/2658643003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.h:181: // TODO(mthiesse): Remove the
need for this to stored here. crbug.com/674594
s/to/to be/

https://codereview.chromium.org/2658643003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.h:182: gvr_context* gvr_api_;
Could we convince the GVR team to typedef this to GvrContext? :)

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

https://codereview.chromium.org/2658643003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_delegate.h:69: // TODO(mthiesse):
Remove the need for this to stored here. crbug.com/674594
"to be"

[mthiesse, 2017-01-30 19:47:26]

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

https://codereview.chromium.org/2658643003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:481: gvr::BufferViewport
eye_viewport = gvr_api->CreateBufferViewport();
On 2017/01/25 21:45:31, cjgrant wrote:
> Looking at the near-duplicate blocks for left and right eye, is it worth a
"for
> (auto eye : {GVR_LEFT_EYE, GVR_RIGHT_EYE}) {" loops?  Looks like one
conditional
> to pick left_eye/right_eye, and the rest of the code is identical.

Good eye ;)

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

https://codereview.chromium.org/2658643003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.h:181: // TODO(mthiesse): Remove the
need for this to stored here. crbug.com/674594
On 2017/01/25 21:45:31, cjgrant wrote:
> s/to/to be/

Done.

https://codereview.chromium.org/2658643003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.h:182: gvr_context* gvr_api_;
On 2017/01/25 21:45:31, cjgrant wrote:
> Could we convince the GVR team to typedef this to GvrContext? :)

Be my guest ;)

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

https://codereview.chromium.org/2658643003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_delegate.h:69: // TODO(mthiesse):
Remove the need for this to stored here. crbug.com/674594
On 2017/01/25 21:45:31, cjgrant wrote:
> "to be"

Done.

[mthiesse, 2017-01-31 15:17:56]

ping bajones, PTAL

[bajones, 2017-02-01 22:20:33]

On 2017/01/31 15:17:56, mthiesse wrote:
> ping bajones, PTAL

LGTM

[mthiesse, 2017-02-01 22:21:28]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2017-02-01 22:21: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-02-01 23:40:22]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485987688150790,
"parent_rev": "3a5713efd9293f2817784b8c77ffc1e55edad097", "commit_rev":
"4c6465c3b85e664ca700e1c04bebf637c7846243"}

[commit-bot: I haz the power, 2017-02-01 23:41:02]

Description was changed from

==========
Refactor GvrDelegate ownership into GvrDelegateProvider and fix more threading
violations.

This CL does two main things: It refactors controlling when the GvrDelegate is
created and destroyed into the GvrDelegateProvider, and it refactors out some
instances of using the GVR api in a non-threadsafe way.

Some minor functional changes may be visible with this patch - namely that we no
longer (possibly) send a DisplayConnected event for a display that isn't ready
yet due to the gvr api not being available. This should be an improvement in all
cases.

BUG=674594, 655297
==========

to

==========
Refactor GvrDelegate ownership into GvrDelegateProvider and fix more threading
violations.

This CL does two main things: It refactors controlling when the GvrDelegate is
created and destroyed into the GvrDelegateProvider, and it refactors out some
instances of using the GVR api in a non-threadsafe way.

Some minor functional changes may be visible with this patch - namely that we no
longer (possibly) send a DisplayConnected event for a display that isn't ready
yet due to the gvr api not being available. This should be an improvement in all
cases.

BUG=674594, 655297

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

[commit-bot: I haz the power, 2017-02-01 23:41:03]

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