Switch WebVR to handle GvrApi management through VrShellDelegate

Switch WebVR to handle GvrApi management through VrShellDelegate

BUG=389343
Committed: https://crrev.com/d7f480d5099bcb664eba873a5c24f374d12e517d
Cr-Commit-Position: refs/heads/master@{#420181}

[bajones, 2016-09-15 23:40:37]

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

[mthiesse, 2016-09-16 14:26:53]

At a high level I'm really not liking how much this complicates VrShell.

I feel like this would all be much better if you only started VrShell when
presenting. Yes, this means swapping out the GVR API instance, but if forward
vector issues are the only problem we can definitely work around that.

It also means it won't matter if (when?) VrShell becomes its own activity.

There's also the possibility of polyfilling the API when not presenting.

[bajones, 2016-09-16 20:35:11]

Revised and cleaned up. I think to get around bugs in the current version of GVR
we'll need to shut down the non-presenting instance of the API entirely before
allowing VrShell to spin up, but the patch posted here should at least be enough
for some architectural feedback.

[bajones, 2016-09-18 22:19:52]

Latest revision is working pretty well, though I still question it's long term
viability. Fixed the bad initialization values. (They're still there, but when
we switch to WebVR presentation mode they get overwritten with good ones, so
well behaved apps won't notice.) Also worked out some bugs around order of
initialization/destruction so that we don't attempt to use an instance of GvrApi
after it's been destroyed.

Currently I create a non-presenting GvrApi instance and use that any time we're
in magic window mode. When presenting it causes VrShell to be created and begins
using it's instance of GvrApi, then when presentation exits we switch back to
the non-presenting instance. We don't shut down that instance while VrShell is
up, even though we probably could. I haven't seen it cause problems so far.

Please take another look, I'd like to land this ASAP so that we can start
working on getting Klaus' work cleaned up and upstreamed as well.

[mthiesse, 2016-09-18 22:55:27]

lgtm

[bajones, 2016-09-19 01:39:45]

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

[bajones, 2016-09-19 01:39:46]

dtrainor@chromium.org: Please review changes in chrome/android and
chrome/browser/android

[bajones, 2016-09-19 22:14:14]

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

[bajones, 2016-09-19 22:14:15]

kenrb@: Could you review the small .mojom change? (Made one argument nullable)

[kenrb, 2016-09-20 14:29:26]

lgtm

[bajones, 2016-09-20 18:13:04]

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

[bajones, 2016-09-20 18:13:05]

tedchoc@: Seems like David is occupied with Blimpcon this week (that's a thing?)
Could you review chrome/android and chrome/browser/android?

[bajones, 2016-09-20 20:20:33]

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

[bajones, 2016-09-20 20:20:33]

dfalcantara@: Perhaps you could give this a quick look instead? (I know we're
asking for a lot of reviews today?)

[Ted C, 2016-09-20 20:31:11]

https://codereview.chromium.org/2343023002/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java
(right):

https://codereview.chromium.org/2343023002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:65:
public void onNativeLibraryReady(long nativeDelegate) {
try not to pass around raw pointers in java.  instead, prefer to pass the java
object and fetch the corresponding native version in c++

Something akin to this for how we handle WebContents:
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...

https://codereview.chromium.org/2343023002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:211: 
fwiw, we usually don't add spaces between our native methods.

https://codereview.chromium.org/2343023002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2343023002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:59:
public void onNativeLibraryReady() {
all public methods should have some sort of documentation

https://codereview.chromium.org/2343023002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:80:
public boolean enterVRIfNecessary() {
could this just take a boolean param isWebVR?  same for exit?

https://codereview.chromium.org/2343023002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:140:
// exitWebVRIfNecessary) when it's safe to destroy the VrShell.
that is an odd semantics.  should we pull out a distinct destroy call and have
it call that instead of a method meaning two things?

[bajones, 2016-09-20 23:26:51]

Thanks for the review! Addressed everything but one item, which you may have a
suggestion for:

On 2016/09/20 20:31:11, Ted C wrote:
> public void onNativeLibraryReady(long nativeDelegate) {
> try not to pass around raw pointers in java.  instead, prefer to pass the java
> object and fetch the corresponding native version in c++

I am passing the Java object further down the call chain now, but passing it
into the native VrShell poses a problem in that I would then need to call
Java_VrShellDelegate_getNativePointer() in vr_shell.cc, which would require me
to include VrShellDelegate_jni.h, which would then conflict with the
VrShell_jni.h RegisterNativesImpl. Do you know how to resolve that conflict?

[Ted C, 2016-09-21 00:17:01]

On 2016/09/20 23:26:51, bajones wrote:
> Thanks for the review! Addressed everything but one item, which you may have a
> suggestion for:
> 
> On 2016/09/20 20:31:11, Ted C wrote:
> > public void onNativeLibraryReady(long nativeDelegate) {
> > try not to pass around raw pointers in java.  instead, prefer to pass the
java
> > object and fetch the corresponding native version in c++
> 
> I am passing the Java object further down the call chain now, but passing it
> into the native VrShell poses a problem in that I would then need to call
> Java_VrShellDelegate_getNativePointer() in vr_shell.cc, which would require me
> to include VrShellDelegate_jni.h, which would then conflict with the
> VrShell_jni.h RegisterNativesImpl. Do you know how to resolve that conflict?

The web_contents_android link I posted actually shows how we handle that other
places.

In essence, you'd add a static in vr_shell.cc (VrShell::FromJavaVrShell) that
takes a java obj and casts to the native VrShell w/ access to the get
native ptr call.

[bajones, 2016-09-21 00:23:21]

On 2016/09/21 00:17:01, Ted C wrote:
> The web_contents_android link I posted actually shows how we handle that other
> places.
> 
> In essence, you'd add a static in vr_shell.cc (VrShell::FromJavaVrShell) that
> takes a java obj and casts to the native VrShell w/ access to the get
> native ptr call.

I think you missed the bit where the VrShell native code is actually interacting
with native pointers from two different java classes. VrShell and
VrShellDelegate. If I pass the VrShellDelegate java object into VrShell as
suggested we run into conflicts because the JNI headers for both objects have
conflicting definitions of RegisterNativesImpl. I haven't seen another place in
the Chrome code where JNI interfaces from two different objects have to co-exist
like that. As such it seems like we must already have a pattern for that
scenario?

[Ted C, 2016-09-21 00:31:52]

On 2016/09/21 00:23:21, bajones wrote:
> On 2016/09/21 00:17:01, Ted C wrote:
> > The web_contents_android link I posted actually shows how we handle that
other
> > places.
> > 
> > In essence, you'd add a static in vr_shell.cc (VrShell::FromJavaVrShell)
that
> > takes a java obj and casts to the native VrShell w/ access to the get
> > native ptr call.
> 
> I think you missed the bit where the VrShell native code is actually
interacting
> with native pointers from two different java classes. VrShell and
> VrShellDelegate. If I pass the VrShellDelegate java object into VrShell as
> suggested we run into conflicts because the JNI headers for both objects have
> conflicting definitions of RegisterNativesImpl. I haven't seen another place
in
> the Chrome code where JNI interfaces from two different objects have to
co-exist
> like that. As such it seems like we must already have a pattern for that
> scenario?

I didn't know the exact files causing issue, so my example above was slightly
misleading.  You shouldn't ever include JNI headers for two files within a
single C++ file.

I was slightly off in my last example, but instead of VrShell::FromJavaVrShell,
you just need to add VrShellDelegate::FromJavaVrShellDelegate.

VrShell in native gets an opaque java obj for the delegate.  It calls a
static function on VrShellDelegate to get a native VrShellDelegate from a
java obj.

Internally, it just calls Java_VrShellDelegate_getNativePointer on the obj,
then does the reinterpret cast of that.

That's how the WebContents example works above.  Places like Tab.java will
pass a WebContents java object to C++, and then it calls the static method
on WebContents to convert to a native version.

[bajones, 2016-09-21 00:56:28]

Ah, OK! Got it! Thanks for the more detailed explanation. I'll get the CL
updated ASAP.

[bajones, 2016-09-21 17:25:21]

Okay, updated to pass the Java object down to the native code and query the
native pointer from there. Thanks for your patience and sorry for being a bit
dense. :P PTAL!

[Ted C, 2016-09-21 17:59:25]

Absolutely no need to apologize.  It is not reasonable for you to understand all
the android implementation quirks and idiosyncrasies.  This looks much better! 
Some other small things and I think it'll be good to go.

https://codereview.chromium.org/2343023002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2343023002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:64:
mNativeVrShellDelegate = nativeInit();
do you need a check for mVrShellEnabled here?  otherwise, I believe you'll be
attempting to initialize native code that won't exist (when the build flag is
off).

https://codereview.chromium.org/2343023002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:82:
* @Returns Whether or not we are in VR when this function returns.
We should add an @param for inWebVR here

Also if we are here, can you change this to @return instead of @Returns

https://codereview.chromium.org/2343023002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:102:
mVrShell.onNativeLibraryReady(tab, this);
to me, it's weird to have this not in the onNativeLibraryReady call above.  Do
these mean different things now?  Should this method be called initializeForTab
or "something"?

https://codereview.chromium.org/2343023002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:103:
if (inWebVR) {
in java land (if it is readable), you can put the condition and statement on a
single line if it all fits

https://codereview.chromium.org/2343023002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:112:
public boolean exitWebVR() {
does this only get accessed by native?  If so, you can make it private.

https://codereview.chromium.org/2343023002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:114:
mVrShell.setWebVrModeEnabled(false);
Could we just move this line into shutdownVR (is it bad to call in non-webVR
cases)?  Then we could expose that via @CalledByNative and we could get rid of
the need for this method?

Ahh...based on the comment below it looks like exiting WebVR won't be calling
shutdown long term, so maybe my question above isn't right.

https://codereview.chromium.org/2343023002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:139:
* @Returns Whether or not we exited VR.
@return

https://codereview.chromium.org/2343023002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:254:
public long getNativePointer() {
should be private

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

https://codereview.chromium.org/2343023002/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.cc:109: delegate_ =
VrShellDelegate::getNativePointer(env, delegate);
getNativeDelegate seems like a clearer name to me

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

https://codereview.chromium.org/2343023002/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell.h:44: const
base::android::JavaParamRef<jobject>& obj,
align w/ JNIEnv above

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

https://codereview.chromium.org/2343023002/diff/160001/chrome/browser/android...
chrome/browser/android/vr_shell/vr_shell_delegate.cc:34: // Returns true if the
GvrDeviceProvider needs to handle shutdown first.
would this fit better in the header?

[bajones, 2016-09-21 18:37:08]

Done! Thanks again!

[Ted C, 2016-09-21 18:49:11]

On 2016/09/21 18:37:08, bajones wrote:
> Done! Thanks again!

Looks like there are couple open questions in VrShellDelegate.java, maybe
clarify if they are out of scope, should be done in followups, not applicable,
and if applicable and not worth fixing right now, please add TODOs

[bajones, 2016-09-21 19:16:25]

Sorry, meant to bounce the rename off the rest of the team and forgot about it
after addressing the other items.

https://codereview.chromium.org/2343023002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2343023002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:102:
mVrShell.onNativeLibraryReady(tab, this);
On 2016/09/21 17:59:25, Ted C wrote:
> to me, it's weird to have this not in the onNativeLibraryReady call above.  Do
> these mean different things now?  Should this method be called
initializeForTab
> or "something"?

While "something" seems like a great function name, after consulting with the
other VrShell devlopers I went with "initializeNative" instead. ;)

https://codereview.chromium.org/2343023002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:114:
mVrShell.setWebVrModeEnabled(false);
On 2016/09/21 17:59:25, Ted C wrote:
> Could we just move this line into shutdownVR (is it bad to call in non-webVR
> cases)?  Then we could expose that via @CalledByNative and we could get rid of
> the need for this method?
> 
> Ahh...based on the comment below it looks like exiting WebVR won't be calling
> shutdown long term, so maybe my question above isn't right.

I think the comment below illustrates the reason for this. In future CLs we will
want to exit from WebVR back to the VrShell rather than exiting VR entirely. As
such, we want to disambiguate whether we're only exiting the WebVR content or VR
as a whole.

[gone, 2016-09-21 20:39:02]

Description was changed from

==========
Switch WebVR to handle GvrApi management through VrShellDelegate

BUG=389343
==========

to

==========
Switch WebVR to handle GvrApi management through VrShellDelegate

BUG=389343
==========

[gone, 2016-09-21 20:39:02]

dfalcantara@chromium.org changed reviewers:
- dfalcantara@chromium.org

[gone, 2016-09-21 20:39:41]

On 2016/09/21 19:16:25, bajones wrote:
> Sorry, meant to bounce the rename off the rest of the team and forgot about it
> after addressing the other items.
> 
>
https://codereview.chromium.org/2343023002/diff/160001/chrome/android/java/sr...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
> (right):
> 
>
https://codereview.chromium.org/2343023002/diff/160001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:102:
> mVrShell.onNativeLibraryReady(tab, this);
> On 2016/09/21 17:59:25, Ted C wrote:
> > to me, it's weird to have this not in the onNativeLibraryReady call above. 
Do
> > these mean different things now?  Should this method be called
> initializeForTab
> > or "something"?
> 
> While "something" seems like a great function name, after consulting with the
> other VrShell devlopers I went with "initializeNative" instead. ;)
> 
>
https://codereview.chromium.org/2343023002/diff/160001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:114:
> mVrShell.setWebVrModeEnabled(false);
> On 2016/09/21 17:59:25, Ted C wrote:
> > Could we just move this line into shutdownVR (is it bad to call in non-webVR
> > cases)?  Then we could expose that via @CalledByNative and we could get rid
of
> > the need for this method?
> > 
> > Ahh...based on the comment below it looks like exiting WebVR won't be
calling
> > shutdown long term, so maybe my question above isn't right.
> 
> I think the comment below illustrates the reason for this. In future CLs we
will
> want to exit from WebVR back to the VrShell rather than exiting VR entirely.
As
> such, we want to disambiguate whether we're only exiting the WebVR content or
VR
> as a whole.

Removing myself from this review since Ted's handling it.

[Ted C, 2016-09-21 21:01:47]

lgtm

[bajones, 2016-09-21 21:24:42]

The CQ bit was checked by bajones@chromium.org

[bajones, 2016-09-21 21:24:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, kenrb@chromium.org
Link to the patchset: https://codereview.chromium.org/2343023002/#ps200001
(title: "Renamed onNativeLibraryReady to initializeNative")

[commit-bot: I haz the power, 2016-09-21 21:25:21]

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-09-21 22:26:53]

Description was changed from

==========
Switch WebVR to handle GvrApi management through VrShellDelegate

BUG=389343
==========

to

==========
Switch WebVR to handle GvrApi management through VrShellDelegate

BUG=389343
==========

[commit-bot: I haz the power, 2016-09-21 22:26:54]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-09-21 22:29:19]

Description was changed from

==========
Switch WebVR to handle GvrApi management through VrShellDelegate

BUG=389343
==========

to

==========
Switch WebVR to handle GvrApi management through VrShellDelegate

BUG=389343

Committed: https://crrev.com/d7f480d5099bcb664eba873a5c24f374d12e517d
Cr-Commit-Position: refs/heads/master@{#420181}
==========

[commit-bot: I haz the power, 2016-09-21 22:29:21]

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