Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2301633002:
Refactor Vr activity into ChromeTabbedActivity. (Closed)
|
Created:
4 years, 3 months ago by mthiesse Modified:
4 years, 3 months ago Reviewers:
klausw, David Trainor- moved to gerrit, klausw (use chromium instead), Ted C, no sievers, bshe, cjgrant CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor Vr activity into ChromeTabbedActivity.
This CL implements the following:
1. Deletes VrActivity, integrating VrShell into ChromeTabbedActivity.
2. Adds toggle for entering and exiting VR, simulating how NFC trigger
will work.
Known issues:
1. The current Compositor surface is displayed over top of the VR view.
To test this, add "enable_vr_shell=true" in your gn args.
These changes are pulled from our working experimental 'branch' found here: https://codereview.chromium.org/2248183002/
BUG=638642
Committed: https://crrev.com/6a9e4415a09b41e101d84f8c129222cec0ecceb7
Cr-Commit-Position: refs/heads/master@{#418255}
Patch Set 1 #Patch Set 2 : Clean up vr_util #
Total comments: 37
Patch Set 3 : Created VrShellDelegate, addressed comments #
Total comments: 16
Patch Set 4 : Address Ted's comments #
Total comments: 6
Patch Set 5 : Address comments #
Total comments: 56
Patch Set 6 : Fix some typos #Patch Set 7 : Address comments and rebase #
Total comments: 39
Patch Set 8 : Addressed comments. #
Total comments: 6
Patch Set 9 : Add comments #Patch Set 10 : rebase onto ToT #Patch Set 11 : Fix crash introduced by rebase. #Patch Set 12 : UiElements are now structs #
Total comments: 2
Messages
Total messages: 51 (13 generated)
mthiesse@chromium.org changed reviewers: + bshe@chromium.org, cjgrant@chromium.org, klausw@chromium.org
PTAL before I send this out to others.
The CQ bit was checked by mthiesse@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mthiesse@chromium.org changed reviewers: + sievers@chromium.org, tedchoc@chromium.org
I Daniel and Ted, PTAL, this refactors VrActivity into ChromeTabbedActivity, as discussed yesterday. On top of Biao's work, I've added a toggle to enter and exit VR without launching from the daydream launcher (note that this still won't work without a daydream-ready device).
Description was changed from ========== Refactor Vr activity into ChromeTabbedActivity. This CL implements the following: 1. Deletes VrActivity, integrating VrShell into ChromeTabbedActivity. 2. Adds toggle for entering and exiting VR, simulating how NFC trigger will work. Known issues: 1. The current Compositor surface is displayed over top of the VR view. To test this, add "enable_vr_shell=true" in your gn args. BUG=638642 ========== to ========== Refactor Vr activity into ChromeTabbedActivity. This CL implements the following: 1. Deletes VrActivity, integrating VrShell into ChromeTabbedActivity. 2. Adds toggle for entering and exiting VR, simulating how NFC trigger will work. Known issues: 1. The current Compositor surface is displayed over top of the VR view. To test this, add "enable_vr_shell=true" in your gn args. These changes are pulled from our working experimental 'branch' found here: https://codereview.chromium.org/2248183002/ BUG=638642 ==========
https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:382: if (isVrIntent(getIntent())) enterVR(); Should this be an 'else if'? enterVR() returns immediately if mInVr is true, but this could prevent an unnecessary function call.
https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:181: boolean showVrMenuItem = DEBUG_VR && mActivity.isVrShellEnabled(); This requires a code change to see the menu item, correct? Or is there some way to set DEBUG_VR from a build flag? https://codereview.chromium.org/2301633002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2301633002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:107: // preferred overall screen height while maintaining the aspect ratio. Could be just me, but I don't understand this comment. It doesn't look like a width is passed in and I don't see how the aspect ratio is maintained here? It looks like it just multiplies both dimensions by a constant desktop_height_. https://codereview.chromium.org/2301633002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:136: UpdateTransforms(screen_width, screen_width, screenTilt); Are the first two params both supposed to be width or should one of them be height? https://codereview.chromium.org/2301633002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.h (right): https://codereview.chromium.org/2301633002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:57: // Content rect in world coordinates. Height and width are currently supplied Is this comment still valid? I don't see any height or width supplied to the DrawFrame method.
I focused more on the Activity integration vs the rendering, and I hope sievers@ could look at that. https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1627: } else if (id == R.id.enter_vr_id) { move this into ChromeTabbedActivity's menu handling code. enterVR() should be just there. https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1643: public boolean isVrShellEnabled() { Forgive me for being ignorant, but what is VR Shell as a concept? Shell in chrome is a somewhat overloaded term to me, but I think of something that is not fully capable (i.e. content shell where it is just the bare minimum required stuff to show web content). Is that what this means here? Is the goal to replace all of this with a full[er] VR implementation at some point? What is the long term vision with this? Is shell the long term naming for this? Just wondering why it's not something like isVrModeCapable/supportsVrMode or something? https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:223: Class<?> mVrShellClass = null; should probably be private, also should this be "? extends VrShellInterface" null and false are defaults, so we let's not set them (this class doesn't really do a good job with that though). https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:958: maybeFindVrShell(); this seems unnecessary here, I would just have this be a first time first use sort of thing. https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1594: public boolean isVrShellEnabled() { I'd vote for doing a tri-state boolean like accessibility mode (capital B Boolean). if (mVrShellEnabled != null) return mVrShellEnabled; mVrShellEnabled = maybeFindVrShell() != null; return mVrShellEnabled; Doing reflection every time for something that will always either be true or false seems bad to me. https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1598: return mVrShellClass != null; looking at this a bit more, I think we should move all of this code into a VrShellDelegate/VrShellHelper. The code in this class seems like it could be very much smaller (pass R.id.content as a parent container to make it a bit cleaner). I think having this class just have to call some VrShellDelegate for enter/exit/pause/resume would be nice https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1655: } catch (Exception ex) { don't catch generic exception, catch the specific ones. And you can | the exception types together to avoid a bunch of catches. https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1656: ex.printStackTrace(); Use chrome logging for this instead of just bare stack trace printing. https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1658: mVrShellFrameLayout = (FrameLayout) mVrShellView; Put this in the body of the try (casting null won't NPE, but this just reads oddly to me). Also, instead of having to cast to both a VrShellInterface and FrameLayout, I think it would be cleaner to expose: FrameLayout getContainer(); on VrShellInterface, and call that (and that method would just look like "return this;"). https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1659: StrictMode.setThreadPolicy(oldPolicy); put this in a finally in your try catch block. https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1663: WindowManager.LayoutParams params = new WindowManager.LayoutParams( Hmm...do you actually need a WindowManager.LayoutParams? android.R.id.content is a FrameLayout, and I wouldn't think that it would use anything defined here except for width and height. Wondering if we should just use a vanilla ViewGroup.LayoutParams or ViewGroup.MarginLayoutParams. https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1704: String vrExtra = (String) mVrShellClass.getField("VR_EXTRA").get(null); cache this value as well. we wouldn't want to introduce reflection for each intent call https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1707: e.printStackTrace(); same comment about using chrome logging https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:181: boolean showVrMenuItem = DEBUG_VR && mActivity.isVrShellEnabled(); On 2016/09/01 20:50:40, amp wrote: > This requires a code change to see the menu item, correct? Or is there some way > to set DEBUG_VR from a build flag? For now, it seems like you could just have isVrShellEnabled since that is only enabled if you have the proper gn flags. Long term since this should never be enabled in prod builds, I would just do CommandLine.hasSwitch or something to make it settable via command line args
Thanks for the reviews! https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1627: } else if (id == R.id.enter_vr_id) { On 2016/09/01 22:38:37, Ted C wrote: > move this into ChromeTabbedActivity's menu handling code. enterVR() should be > just there. Done. https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1643: public boolean isVrShellEnabled() { On 2016/09/01 22:38:37, Ted C wrote: > Forgive me for being ignorant, but what is VR Shell as a concept? > > Shell in chrome is a somewhat overloaded term to me, but I think of something > that is not fully capable (i.e. content shell where it is just the bare minimum > required stuff to show web content). > > Is that what this means here? Is the goal to replace all of this with a > full[er] VR implementation at some point? > > What is the long term vision with this? Is shell the long term naming for this? > Just wondering why it's not something like isVrModeCapable/supportsVrMode or > something? VR Shell has been a highly-debated term. Shell in this instance has no conceptual relation to other shells like the content shell, and more refers to the holding or enclosing of VR content. https://docs.google.com/document/d/16vHXI5_s-Eqttsa6YKnCf-2Tj0yIOTUay1eNyBPkF... defines the terms we use to describe various concepts and components. The reason I use isVrShellEnabled is because the compile-time flag for enabling VR Shell support is 'ENABLE_VR_SHELL', and this function tests compile-time support. I'm open to changing it, but we have to be careful to make a distinction between WebVR support and VR Shell support (WebVR support will be much broader). Terms like HUD are already taken in VR, Chrome VR is too confusing and overloaded, and VR Mode is too ambiguous. https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:223: Class<?> mVrShellClass = null; On 2016/09/01 22:38:37, Ted C wrote: > should probably be private, also should this be "? extends VrShellInterface" > > null and false are defaults, so we let's not set them (this class doesn't really > do a good job with that though). Done. https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:382: if (isVrIntent(getIntent())) enterVR(); On 2016/09/01 20:24:03, bsheedy wrote: > Should this be an 'else if'? enterVR() returns immediately if mInVr is true, but > this could prevent an unnecessary function call. Done. https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:958: maybeFindVrShell(); On 2016/09/01 22:38:37, Ted C wrote: > this seems unnecessary here, I would just have this be a first time first use > sort of thing. Done. https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1594: public boolean isVrShellEnabled() { On 2016/09/01 22:38:37, Ted C wrote: > I'd vote for doing a tri-state boolean like accessibility mode (capital B > Boolean). > > if (mVrShellEnabled != null) return mVrShellEnabled; > mVrShellEnabled = maybeFindVrShell() != null; > return mVrShellEnabled; > > Doing reflection every time for something that will always either be true or > false seems bad to me. Done. https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1598: return mVrShellClass != null; On 2016/09/01 22:38:37, Ted C wrote: > looking at this a bit more, I think we should move all of this code into a > VrShellDelegate/VrShellHelper. > > The code in this class seems like it could be very much smaller (pass > R.id.content as a parent container to make it a bit cleaner). > > I think having this class just have to call some VrShellDelegate for > enter/exit/pause/resume would be nice Done. https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1655: } catch (Exception ex) { On 2016/09/01 22:38:37, Ted C wrote: > don't catch generic exception, catch the specific ones. And you can | the > exception types together to avoid a bunch of catches. Done. https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1656: ex.printStackTrace(); On 2016/09/01 22:38:37, Ted C wrote: > Use chrome logging for this instead of just bare stack trace printing. Done. https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1658: mVrShellFrameLayout = (FrameLayout) mVrShellView; On 2016/09/01 22:38:37, Ted C wrote: > Put this in the body of the try (casting null won't NPE, but this just reads > oddly to me). > > Also, instead of having to cast to both a VrShellInterface and FrameLayout, I > think it would be cleaner to expose: > > FrameLayout getContainer(); > > on VrShellInterface, and call that (and that method would just look like "return > this;"). Done. https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1659: StrictMode.setThreadPolicy(oldPolicy); On 2016/09/01 22:38:37, Ted C wrote: > put this in a finally in your try catch block. Done. https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1663: WindowManager.LayoutParams params = new WindowManager.LayoutParams( On 2016/09/01 22:38:37, Ted C wrote: > Hmm...do you actually need a WindowManager.LayoutParams? > > android.R.id.content is a FrameLayout, and I wouldn't think that it would use > anything defined here except for width and height. > > Wondering if we should just use a vanilla ViewGroup.LayoutParams or > ViewGroup.MarginLayoutParams. Done. https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1704: String vrExtra = (String) mVrShellClass.getField("VR_EXTRA").get(null); On 2016/09/01 22:38:37, Ted C wrote: > cache this value as well. we wouldn't want to introduce reflection for each > intent call Done. https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1707: e.printStackTrace(); On 2016/09/01 22:38:37, Ted C wrote: > same comment about using chrome logging Done. https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2301633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:181: boolean showVrMenuItem = DEBUG_VR && mActivity.isVrShellEnabled(); On 2016/09/01 22:38:37, Ted C wrote: > On 2016/09/01 20:50:40, amp wrote: > > This requires a code change to see the menu item, correct? Or is there some > way > > to set DEBUG_VR from a build flag? > > For now, it seems like you could just have isVrShellEnabled since that is only > enabled if you have the proper gn flags. > > Long term since this should never be enabled in prod builds, I would just do > CommandLine.hasSwitch or something to make it settable via command line args Done. https://codereview.chromium.org/2301633002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2301633002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:107: // preferred overall screen height while maintaining the aspect ratio. On 2016/09/01 20:50:41, amp wrote: > Could be just me, but I don't understand this comment. > > It doesn't look like a width is passed in and I don't see how the aspect ratio > is maintained here? It looks like it just multiplies both dimensions by a > constant desktop_height_. More stale comments, sorry. https://codereview.chromium.org/2301633002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:136: UpdateTransforms(screen_width, screen_width, screenTilt); On 2016/09/01 20:50:40, amp wrote: > Are the first two params both supposed to be width or should one of them be > height? Good catch, that was a typo. https://codereview.chromium.org/2301633002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.h (right): https://codereview.chromium.org/2301633002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:57: // Content rect in world coordinates. Height and width are currently supplied On 2016/09/01 20:50:41, amp wrote: > Is this comment still valid? I don't see any height or width supplied to the > DrawFrame method. Ah, nope stale comment.
Looks much better this way in my opinion. A few remaining bits to tie up and I think we're good While I'd still argue that supportVrBrowserUi (or something) is better than Vr Shell, I shan't continue down that path as that is way outside the scope of this change (since it would have much broader impacts). https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:375: } else if (mVrShellDelegate.isVrIntent(getIntent())) { When exactly would this happen? What happens if Chrome is started with a VR intent, then you exit VR, and then leave Chrome and come back? Would this still have getIntent() as VR mode and would it be forcing you into VR? Should this be in onNewIntentWithNative? Or, should we check that getSavedInstanceState() == null and some in memory boolean flag to make sure we are not resuming from a non-killed state. And should you handle a new intent if Chrome is in the foreground non-paused state? (i.e. via an adb am start command)? If so, you need to handle this in onNewIntentWithNative as you are not guaranteed that onresume will be triggered. Or maybe this doesn't matter at all. https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1593: * See VrShellDelegate#isInVRShellMode() I think we should just have ChromeTabbedActivity call directly into the delegate instead of this helper methods. I think the cases that check for this could have delegate methods like: boolean resumeVRIfNecessary() { if (!isInVRShellMode()) return false; resumeVR(); return true; } boolean exitVRIfNecessary() { if (!isInVRShellMode()) return false; exitVR(); return true; } ... Then CTA only has: mVrShellDelegate.exitVrIfNecessary() and all the logic is elsewhere. Only isVrShellEnabled() is the one that remains in my mind. https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java (right): https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:68: class FrameListener implements OnFrameAvailableListener { why pull out this class? Seems like more redirection than necessary IMO (and you end up poking the internals in onDrawFrame anyway). https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellInterface.java (right): https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellInterface.java:13: public abstract interface VrShellInterface { abstract isn't needed https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellInterface.java:17: public abstract void onNativeLibraryReady(); public isn't needed on interfaces (they are inherently public and you can only restrict their visibility by changing the visibility of the interface itself)
sievers@ is OOO until next week, but we haven't touched any compositing code, so as long as you're happy with the ChromeTabbedActivity changes Ted, I'll get the somebody from my team to review the native changes here and we should be good? https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:375: } else if (mVrShellDelegate.isVrIntent(getIntent())) { On 2016/09/06 17:47:55, Ted C wrote: > When exactly would this happen? > > What happens if Chrome is started with a VR intent, then you exit VR, and then > leave Chrome and come back? Would this still have getIntent() as VR mode and > would it be forcing you into VR? > > Should this be in onNewIntentWithNative? Or, should we check that > getSavedInstanceState() == null and some in memory boolean flag to make sure we > are not resuming from a non-killed state. > > And should you handle a new intent if Chrome is in the foreground non-paused > state? (i.e. via an adb am start command)? If so, you need to handle this in > onNewIntentWithNative as you are not guaranteed that onresume will be triggered. > > Or maybe this doesn't matter at all. You're right that Chrome would force you back into VR, and while there are a number of ways to fix that, onNewIntentWithNative does look like the best option. (Probably the next best solution is to just remove the VR extra key from the getIntent() intent when we handle it) https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1593: * See VrShellDelegate#isInVRShellMode() On 2016/09/06 17:47:55, Ted C wrote: > I think we should just have ChromeTabbedActivity call directly into the delegate > instead of this helper methods. > > I think the cases that check for this could have delegate methods like: > > boolean resumeVRIfNecessary() { > if (!isInVRShellMode()) return false; > resumeVR(); > return true; > } > > boolean exitVRIfNecessary() { > if (!isInVRShellMode()) return false; > exitVR(); > return true; > } > > ... > > > Then CTA only has: > > mVrShellDelegate.exitVrIfNecessary() and all the logic is elsewhere. > > Only isVrShellEnabled() is the one that remains in my mind. Done. https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java (right): https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:68: class FrameListener implements OnFrameAvailableListener { On 2016/09/06 17:47:55, Ted C wrote: > why pull out this class? Seems like more redirection than necessary IMO (and > you end up poking the internals in onDrawFrame anyway). This is here for two reasons: 1. In the next CL will have two SurfaceTextures to manage in this class, so VrShell can't extend OnFrameAvailableListener. 2. I pulled this CL out of our prototype branch and undoing these changes didn't seem worth the effort. If you prefer, I can revert this part temporarily, but it'll be back in the next CL I'll be sending out shortly :P https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellInterface.java (right): https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellInterface.java:13: public abstract interface VrShellInterface { On 2016/09/06 17:47:55, Ted C wrote: > abstract isn't needed Done. https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellInterface.java:17: public abstract void onNativeLibraryReady(); On 2016/09/06 17:47:55, Ted C wrote: > public isn't needed on interfaces (they are inherently public and you can only > restrict their visibility by changing the visibility of the interface itself) Seems like the sort of thing the presubmit should check for. Good to know.
https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java (right): https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:68: class FrameListener implements OnFrameAvailableListener { On 2016/09/06 19:08:39, mthiesse wrote: > On 2016/09/06 17:47:55, Ted C wrote: > > why pull out this class? Seems like more redirection than necessary IMO (and > > you end up poking the internals in onDrawFrame anyway). > > This is here for two reasons: > 1. In the next CL will have two SurfaceTextures to manage in this class, so > VrShell can't extend OnFrameAvailableListener. > 2. I pulled this CL out of our prototype branch and undoing these changes didn't > seem worth the effort. > > If you prefer, I can revert this part temporarily, but it'll be back in the next > CL I'll be sending out shortly :P Will the two surfacetextures be rendering at the same time (i.e. will you need to listen to them both simultaneously)? Also, should this class be private? Potentially static as well (just need to pass in the SurfaceView as well)? I'm asking because we do ideally keep our number of classes to a minimum so if we could define interfaces at the class level that is preferred. But I might just need to wait for the follow up to be clearer. https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:69: SurfaceTexture mSurfaceTexture; can this be final? https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:80: mGlSurfaceView.queueEvent(new Runnable() { You should create this Runnable in the constructor to avoid the allocation every frame. Assuming the queue allows enqueuing multiple of the same runnable. https://codereview.chromium.org/2301633002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2301633002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:420: mVrShellDelegate.enterVRIfNecessary(); I think you'll want this check in CTA#initializeState...probably around here: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... Probably something like: if ((mIsOnFirstRun || getSavedInstanceState() == null) && intent != null) { if (mVrShellDelegate.isVrIntent(intent)) { // vr stuff } else if (!mIntentHandler.shouldIgnoreIntent(ChromeTabbedActivity.this, intent)) { // normal stuff } } Thing to note is that you might not have any tabs at this point. So, something you'd want to test in the eventual state. We might also want to enhance the intenthandler/delegate interface to handle VR intents, but let's not worry about that now. https://codereview.chromium.org/2301633002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1171: mVrShellDelegate.exitVRIfNecessary(); My suggestion of the "IfNecessary" naming was to avoid having to check the isInVR here. It would return a boolean of whether it did anything, so you could just do: if (mVrShellDelegate.exitVRIfNecessary()) return true; If you don't want to do that, then I'd drop the IfNecessary from the naming as you're doing the necessary (no pun intended) checks already here. https://codereview.chromium.org/2301633002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1590: // VR shell related code. probably not the most needed comment now :-)
https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java (right): https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:68: class FrameListener implements OnFrameAvailableListener { On 2016/09/06 22:00:03, Ted C wrote: > On 2016/09/06 19:08:39, mthiesse wrote: > > On 2016/09/06 17:47:55, Ted C wrote: > > > why pull out this class? Seems like more redirection than necessary IMO > (and > > > you end up poking the internals in onDrawFrame anyway). > > > > This is here for two reasons: > > 1. In the next CL will have two SurfaceTextures to manage in this class, so > > VrShell can't extend OnFrameAvailableListener. > > 2. I pulled this CL out of our prototype branch and undoing these changes > didn't > > seem worth the effort. > > > > If you prefer, I can revert this part temporarily, but it'll be back in the > next > > CL I'll be sending out shortly :P > > Will the two surfacetextures be rendering at the same time (i.e. will you need > to > listen to them both simultaneously)? > > Also, should this class be private? Potentially static as well (just need to > pass > in the SurfaceView as well)? > > I'm asking because we do ideally keep our number of classes to a minimum so if > we could define interfaces at the class level that is preferred. But I might > just need to wait for the follow up to be clearer. Yes, the two surfacetextures will be rendering at the same time. Static and private both make sense. If you'd like to see hazy visions of the future, you can look at https://codereview.chromium.org/2248183002/ where we have everything working, though not quite in a landable state. However, the delta between these two CLs is pretty large and probably not worth your time parsing. https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:69: SurfaceTexture mSurfaceTexture; On 2016/09/06 22:00:03, Ted C wrote: > can this be final? Done. https://codereview.chromium.org/2301633002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:80: mGlSurfaceView.queueEvent(new Runnable() { On 2016/09/06 22:00:03, Ted C wrote: > You should create this Runnable in the constructor to avoid the allocation every > frame. Assuming the queue allows enqueuing multiple of the same runnable. Done. https://codereview.chromium.org/2301633002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2301633002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:420: mVrShellDelegate.enterVRIfNecessary(); On 2016/09/06 22:00:03, Ted C wrote: > I think you'll want this check in CTA#initializeState...probably around here: > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > Probably something like: > if ((mIsOnFirstRun || getSavedInstanceState() == null) && intent != null) { > if (mVrShellDelegate.isVrIntent(intent)) { > // vr stuff > } else if (!mIntentHandler.shouldIgnoreIntent(ChromeTabbedActivity.this, > intent)) { > // normal stuff > } > } > > Thing to note is that you might not have any tabs at this point. So, something > you'd want to test in the eventual state. We might also want to enhance the > intenthandler/delegate interface to handle VR intents, but let's not worry about > that now. Good point, we'll want to handle it in initializeState as well, though it feels wrong that onNewIntent doesn't fire because technically this is a new intent even though it caused Chrome to initialize. I've left a TODO to do better with the transition to VR here, because the initial chrome initialization flow when coming from another VR app is very jarring and transitions out of then back into VR. Right now it's not an issue that we don't have any tabs as we don't show anything in VR yet, but this is definitely something we need to take into account in the future. Further to that point, the new tab page doesn't render in VR so we'll have to explicitly do something about that particular case. https://codereview.chromium.org/2301633002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1171: mVrShellDelegate.exitVRIfNecessary(); On 2016/09/06 22:00:03, Ted C wrote: > My suggestion of the "IfNecessary" naming was to avoid having to check the > isInVR here. It would return a boolean of whether it did anything, so you could > just do: > > if (mVrShellDelegate.exitVRIfNecessary()) return true; > > If you don't want to do that, then I'd drop the IfNecessary from the naming as > you're doing the necessary (no pun intended) checks already here. Done. https://codereview.chromium.org/2301633002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1590: // VR shell related code. On 2016/09/06 22:00:03, Ted C wrote: > probably not the most needed comment now :-) Done.
tedchoc@chromium.org changed reviewers: + dtrainor@chromium.org
java lgtm Adding dtrainor@ to take a look at the C++ as we passed some his way already :-) ... when it rains, it pours!
https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/ui_elements.cc (right): https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.cc:21: if (denom == 0) { Means that the line and plane are parallel, but they could either never meet or the line could be contained in the plane. Is one never expected to happen/does the caller not care? https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.cc:36: void ReversibleTransform::SetIdentity() { Might want to consider renaming for clarity. SetIdentity() sounds like it's a mutator that takes an argument. Something like ResetToIdentity() is a bit longer, but clearer IMO.
I mainly gave this a style pass. I also would suggest taking a look at ui/gfx/geometry/* and ui/gfx/transform. They might already do a lot of what you need to do here. If you can't use them for performance reasons I understand. I did a high level look at the math and it seemed fine, but there's a lot and I don't remember all of the matrix algorithms offhand, so I'm happy to defer the low level detailed review of the util methods to someone on your team if they're more up to date on that stuff :). If I miss this for over a day again feel free to just ping me outright. Sorry for the delays! https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/ui_elements.cc (right): https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.cc:16: float getRayPlaneIntersection(gvr::Vec3f rayOrigin, get -> Get. Fix naming scheme on variables too abc_def instead of abcDef. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.cc:36: void ReversibleTransform::SetIdentity() { On 2016/09/07 16:47:06, bsheedy_google wrote: > Might want to consider renaming for clarity. SetIdentity() sounds like it's a > mutator that takes an argument. Something like ResetToIdentity() is a bit > longer, but clearer IMO. MakeIdentity()? That's what gfx::Transform does. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.cc:71: const gvr::Vec3f origin = {0.0f, 0.0f, 0.0f}; kOrigin https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.cc:76: const gvr::Vec3f normalOrig = {0.0f, 0.0f, -1.0f}; kNormalOrig https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.cc:86: ContentRectangle::ContentRectangle() {} = default https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.cc:88: ContentRectangle::~ContentRectangle() {} = default https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/ui_elements.h (right): https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.h:25: YTOP, = 0 for consistency? https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.h:31: class ReversibleTransform { Just want to make sure you've looked at the ui/gfx classes to see if we could reuse any of those. Maybe not since this seems to use the gvr:: subtypes, but wanted to make sure. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.h:41: gvr::Mat4f mToWorld; to_world_ https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.h:42: gvr::Mat4f mFromWorld; from_world_ https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.h:53: // WorldObject* mParent = nullptr; Remove? https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.h:54: ReversibleTransform mTransform; transform_ https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.h:69: int id; Fix member variable names to match above comments. Maybe a few more comments on some of the variables here. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:206: std::thread t([this] { gvr_api_->RefreshViewerProfile(); }); Do we use std::thread anywhere else in content/ or chrome/? Are we supposed to be using base::Thread? I think it'll be harder to get around the disk access thing though if you want to do this all synchronously. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.h (right): https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:44: void UpdateTransforms(float screenWidthMeters, fix variable names, same for others. Will stop nitting on this :). https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:52: static constexpr long kPredictionTimeWithoutVsyncNanos = 50000000; Should these be in an anonymous namespace in the cc file? Why public in the header? https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_renderer.cc (right): https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_renderer.cc:120: TexturedQuadRenderer::~TexturedQuadRenderer() {} = default? Same with other destructor https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_renderer.h (right): https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_renderer.h:38: static constexpr float kHalfHeight = 0.5f; Any reason we pulled these to the h file instead of anonymous namespace in cc? https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_util.cc (right): https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_util.cc:192: // Invert a 4 x 4 matrix using Cramer's Rule End these comments with ., first letter upper case :). https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_util.h (right): https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_util.h:23: typedef struct Recti { Just bringing up the gfx version of these things as well, but if it's for performance reasons I'm fine with this. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_util.h:37: void setIdentityM(gvr::Mat4f& mat); setIdentity -> SetIdentity, same with all other methods. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_util.h:58: gvr::Vec3f MatrixVectorMul(const gvr::Mat4f& m, gvr::Vec3f v); const & for the vecs as well? https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_util.h:61: gvr::Mat4f invertM(gvr::Mat4f mat); const & https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_util.h:69: gvr::Vec3f getForwardVector(gvr::Mat4f matrix); const & https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_util.h:70: gvr::Vec3f getTranslation(gvr::Mat4f matrix); const &
David, on using gfx::Transform and similar - I'll see if there's any history in having our own custom implementation. As we build a transform, we also generate the inverse as we go. I'm not sure it's making things any more efficient though.
Klaus, can you please review the math in vr_util.cc? https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/ui_elements.cc (right): https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.cc:16: float getRayPlaneIntersection(gvr::Vec3f rayOrigin, On 2016/09/08 05:49:29, David Trainor wrote: > get -> Get. Fix naming scheme on variables too abc_def instead of abcDef. Done. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.cc:21: if (denom == 0) { On 2016/09/07 16:47:06, bsheedy_google wrote: > Means that the line and plane are parallel, but they could either never meet or > the line could be contained in the plane. Is one never expected to happen/does > the caller not care? I don't think we care as especially in 3DOF world this is impossible (we have no edge-on UI). I left a TODO for now. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.cc:36: void ReversibleTransform::SetIdentity() { On 2016/09/07 16:47:06, bsheedy_google wrote: > Might want to consider renaming for clarity. SetIdentity() sounds like it's a > mutator that takes an argument. Something like ResetToIdentity() is a bit > longer, but clearer IMO. Done. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.cc:71: const gvr::Vec3f origin = {0.0f, 0.0f, 0.0f}; On 2016/09/08 05:49:29, David Trainor wrote: > kOrigin Done. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.cc:76: const gvr::Vec3f normalOrig = {0.0f, 0.0f, -1.0f}; On 2016/09/08 05:49:29, David Trainor wrote: > kNormalOrig Done. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.cc:86: ContentRectangle::ContentRectangle() {} On 2016/09/08 05:49:29, David Trainor wrote: > = default Done. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.cc:88: ContentRectangle::~ContentRectangle() {} On 2016/09/08 05:49:29, David Trainor wrote: > = default Done. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/ui_elements.h (right): https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.h:25: YTOP, On 2016/09/08 05:49:30, David Trainor wrote: > = 0 for consistency? Done. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.h:31: class ReversibleTransform { On 2016/09/08 05:49:30, David Trainor wrote: > Just want to make sure you've looked at the ui/gfx classes to see if we could > reuse any of those. Maybe not since this seems to use the gvr:: subtypes, but > wanted to make sure. Yes, we've looked at it. The main problem is that the row/column layout is inverted so we would have to convert back and forth a *lot*. We've talked to the GVR team about exposing a matrix math library as part of libgvr.so (it seems all gvr users right now are just copy/pasting the necessary math functions into their own code base), and they're willing to do so, but it's not a high priority for them right now. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.h:41: gvr::Mat4f mToWorld; On 2016/09/08 05:49:30, David Trainor wrote: > to_world_ Done. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.h:42: gvr::Mat4f mFromWorld; On 2016/09/08 05:49:30, David Trainor wrote: > from_world_ Done. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.h:53: // WorldObject* mParent = nullptr; On 2016/09/08 05:49:30, David Trainor wrote: > Remove? Done. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.h:54: ReversibleTransform mTransform; On 2016/09/08 05:49:30, David Trainor wrote: > transform_ Done. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/ui_elements.h:69: int id; On 2016/09/08 05:49:30, David Trainor wrote: > Fix member variable names to match above comments. Maybe a few more comments on > some of the variables here. Done. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:206: std::thread t([this] { gvr_api_->RefreshViewerProfile(); }); On 2016/09/08 05:49:30, David Trainor wrote: > Do we use std::thread anywhere else in content/ or chrome/? Are we supposed to > be using base::Thread? I think it'll be harder to get around the disk access > thing though if you want to do this all synchronously. I've just added a strict mode exception for this (in VrShell.java), which is probably a better solution. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.h (right): https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:44: void UpdateTransforms(float screenWidthMeters, On 2016/09/08 05:49:30, David Trainor wrote: > fix variable names, same for others. Will stop nitting on this :). haha sorry about that. I've scoured through the code; hopefully I caught all of them. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:52: static constexpr long kPredictionTimeWithoutVsyncNanos = 50000000; On 2016/09/08 05:49:30, David Trainor wrote: > Should these be in an anonymous namespace in the cc file? Why public in the > header? There's no reason to have these in the header, rather than the cc file, other than style preferences. I've moved them to the cc file. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_renderer.cc (right): https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_renderer.cc:120: TexturedQuadRenderer::~TexturedQuadRenderer() {} On 2016/09/08 05:49:30, David Trainor wrote: > = default? Same with other destructor Done. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_renderer.h (right): https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_renderer.h:38: static constexpr float kHalfHeight = 0.5f; On 2016/09/08 05:49:30, David Trainor wrote: > Any reason we pulled these to the h file instead of anonymous namespace in cc? Nope, moved back. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_util.cc (right): https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_util.cc:192: // Invert a 4 x 4 matrix using Cramer's Rule On 2016/09/08 05:49:30, David Trainor wrote: > End these comments with ., first letter upper case :). Done. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_util.h (right): https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_util.h:23: typedef struct Recti { On 2016/09/08 05:49:30, David Trainor wrote: > Just bringing up the gfx version of these things as well, but if it's for > performance reasons I'm fine with this. Yes, these structs can easily just be cast and copied into gpu memory. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_util.h:37: void setIdentityM(gvr::Mat4f& mat); On 2016/09/08 05:49:30, David Trainor wrote: > setIdentity -> SetIdentity, same with all other methods. Done. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_util.h:58: gvr::Vec3f MatrixVectorMul(const gvr::Mat4f& m, gvr::Vec3f v); On 2016/09/08 05:49:30, David Trainor wrote: > const & for the vecs as well? Done. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_util.h:61: gvr::Mat4f invertM(gvr::Mat4f mat); On 2016/09/08 05:49:30, David Trainor wrote: > const & Done. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_util.h:69: gvr::Vec3f getForwardVector(gvr::Mat4f matrix); On 2016/09/08 05:49:30, David Trainor wrote: > const & Done. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_util.h:70: gvr::Vec3f getTranslation(gvr::Mat4f matrix); On 2016/09/08 05:49:30, David Trainor wrote: > const & Done.
klausw@google.com changed reviewers: + klausw@google.com
lgtm Math code LGTM overall, minor nitpicks below. Maybe add a toplevel comment that this code isn't intended to be general purpose and may not work outside the specific use cases for the VR code, and a TODO to investigate replacing it with a general-purpose math library down the road? https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_util.cc (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_util.cc:190: gvr::Mat4f InvertM(const gvr::Mat4f& mat) { This appears to be unused. Since inverting a 4x4 matrix is inefficient and can be numerically unstable, consider shelving this method unless there's a use case where it's unavoidable? https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_util.cc:368: * Make sure to invert it if ever used to compute the basis of a right-handed Please update or delete this sentence - OpenGL is already right-handed (x=right, y=up, z=towards camera). Assume you mean be careful if assuming +z = away from camera elsewhere.
On 2016/09/08 14:19:43, cjgrant wrote: > David, on using gfx::Transform and similar - I'll see if there's any history in > having our own custom implementation. As we build a transform, we also generate > the inverse as we go. I'm not sure it's making things any more efficient > though. Ah good point. Maybe the ReversibleTransform could store two gfx::Transforms internally. wdyt? Thanks for checking up on having custom implementations! Let me know what you find.
On 2016/09/08 20:38:00, David Trainor wrote: > On 2016/09/08 14:19:43, cjgrant wrote: > > David, on using gfx::Transform and similar - I'll see if there's any history > in > > having our own custom implementation. As we build a transform, we also > generate > > the inverse as we go. I'm not sure it's making things any more efficient > > though. > > Ah good point. Maybe the ReversibleTransform could store two gfx::Transforms > internally. wdyt? Thanks for checking up on having custom implementations! > Let me know what you find. So Klaus did ReversibleTransform, with the intent of avoiding an inverse calculation (slow and unstable). We could use two gfx::Transforms, but this would add a second "matrix" type, where elsewhere, we use gvr::Mat4f (the GVR library's simple struct). That also means new type conversions. When this code goes cross-platform, we'll have to refactor use of the GVR types. Maybe that's a better time to consider gfx::Transform? If you feel strongly about this, hopefully I could do the investigation in a separate change, so as not to block this one. Let us know...
On 2016/09/08 20:38:00, David Trainor wrote: > On 2016/09/08 14:19:43, cjgrant wrote: > > David, on using gfx::Transform and similar - I'll see if there's any history > in > > having our own custom implementation. As we build a transform, we also > generate > > the inverse as we go. I'm not sure it's making things any more efficient > > though. > > Ah good point. Maybe the ReversibleTransform could store two gfx::Transforms > internally. wdyt? Thanks for checking up on having custom implementations! > Let me know what you find. So Klaus did ReversibleTransform, with the intent of avoiding an inverse calculation (slow and unstable). We could use two gfx::Transforms, but this would add a second "matrix" type, where elsewhere, we use gvr::Mat4f (the GVR library's simple struct). That also means new type conversions. When this code goes cross-platform, we'll have to refactor use of the GVR types. Maybe that's a better time to consider gfx::Transform? If you feel strongly about this, hopefully I could do the investigation in a separate change, so as not to block this one. Let us know...
Last few nits on style I think. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.h (right): https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:44: void UpdateTransforms(float screenWidthMeters, On 2016/09/08 17:44:41, mthiesse wrote: > On 2016/09/08 05:49:30, David Trainor wrote: > > fix variable names, same for others. Will stop nitting on this :). > > haha sorry about that. I've scoured through the code; hopefully I caught all of > them. No worries! I do the same thing especially moving from Java to Native :). https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:52: static constexpr long kPredictionTimeWithoutVsyncNanos = 50000000; On 2016/09/08 17:44:41, mthiesse wrote: > On 2016/09/08 05:49:30, David Trainor wrote: > > Should these be in an anonymous namespace in the cc file? Why public in the > > header? > > There's no reason to have these in the header, rather than the cc file, other > than style preferences. I've moved them to the cc file. Yeah sorry for the nit about this. For me it makes it a bit easier to see how the class works without the internal detail constants. https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_util.h (right): https://codereview.chromium.org/2301633002/diff/80001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_util.h:23: typedef struct Recti { On 2016/09/08 17:44:41, mthiesse wrote: > On 2016/09/08 05:49:30, David Trainor wrote: > > Just bringing up the gfx version of these things as well, but if it's for > > performance reasons I'm fine with this. > > Yes, these structs can easily just be cast and copied into gpu memory. Makes sense. Do we push a lot of these? https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/ui_elements.h (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/ui_elements.h:70: int content_texture_handle; Need trailing _ for these https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:32: } // namespace
https://codereview.chromium.org/2301633002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:112: if (mContentFrameListener.mFirstTex) { is mFirstTex still necessary? I am not sure I understand why do you want to call updateTextImage here? I recall the boolean exist because we use to manually release tex image. But we no longer do that in the code. https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/ui_elements.cc (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/ui_elements.cc:10: #include "base/logging.h" seems like unnecessary? https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/ui_elements.h (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/ui_elements.h:41: gvr::Mat4f to_world_; remove trailing _ for public members. https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/ui_elements.h:42: gvr::Mat4f from_world_; ditto. https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/ui_elements.h:48: gvr::Quatf orientation_ = {0.0f, 0.0f, 0.0f, 1.0f}; ditto https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:56: const base::android::JavaParamRef<jobject>& obj) { looks like you missed this one. remove base::android https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:263: float zAnchorTranslate = rect->anchor_z ? desktop_position_.z : 0; nit: z_anchor_translate https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell.h (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.h:48: // samplerExternalOES texture data for desktop content image. desktop might be misleading. Perhaps remove "desktop" and change to "content area image". https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell_renderer.cc (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell_renderer.cc:30: const char* GetShaderSource(vr_shell::ShaderID shader) { if you dont move the vr_shell namespace definition, you shouldn't need to add vr_shell namespace in this function. Was there any specific reason you want to move this anonymous namespace out of vr_shell? https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_util.cc (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_util.cc:368: * Make sure to invert it if ever used to compute the basis of a right-handed On 2016/09/08 19:47:04, klausw wrote: > Please update or delete this sentence - OpenGL is already right-handed (x=right, > y=up, z=towards camera). Assume you mean be careful if assuming +z = away from > camera elsewhere. also: comment should be moved to .h file https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_util.cc:386: */ ditto https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_util.cc:519: // const float x = quat.qx; remove these comments? https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_util.h (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_util.h:15: #define makeRectangularTextureBuffer(left, right, bottom, top) \ see here: https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros We need to conform with the coding style if we decide to use this macros
https://codereview.chromium.org/2301633002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:112: if (mContentFrameListener.mFirstTex) { On 2016/09/09 14:42:10, bshe wrote: > is mFirstTex still necessary? I am not sure I understand why do you want to call > updateTextImage here? > I recall the boolean exist because we use to manually release tex image. But we > no longer do that in the code. The reason I've kept it is because we're not guaranteed to get an onFrameAvailable call until we do something that updates the texture, like scrolling. Currently we're changing the size when we switch to VR, so we're probably getting an onFrameAvailable anyways, but that's an artifact of our current implementation, and could potentially change in the future. https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/ui_elements.cc (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/ui_elements.cc:10: #include "base/logging.h" On 2016/09/09 14:42:10, bshe wrote: > seems like unnecessary? Good catch https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/ui_elements.h (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/ui_elements.h:41: gvr::Mat4f to_world_; On 2016/09/09 14:42:10, bshe wrote: > remove trailing _ for public members. Done. https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/ui_elements.h:42: gvr::Mat4f from_world_; On 2016/09/09 14:42:10, bshe wrote: > ditto. Done. https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/ui_elements.h:48: gvr::Quatf orientation_ = {0.0f, 0.0f, 0.0f, 1.0f}; On 2016/09/09 14:42:10, bshe wrote: > ditto Done. https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/ui_elements.h:70: int content_texture_handle; On 2016/09/09 06:28:29, David Trainor wrote: > Need trailing _ for these These are public members, expected to be accessed directly (this class is effectively just a struct with a complex constructor), so shouldn't they leave out the trailing _? https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:32: } On 2016/09/09 06:28:29, David Trainor wrote: > // namespace Done. https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:56: const base::android::JavaParamRef<jobject>& obj) { On 2016/09/09 14:42:10, bshe wrote: > looks like you missed this one. remove base::android Done. https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.cc:263: float zAnchorTranslate = rect->anchor_z ? desktop_position_.z : 0; On 2016/09/09 14:42:10, bshe wrote: > nit: z_anchor_translate Done. https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell.h (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell.h:48: // samplerExternalOES texture data for desktop content image. On 2016/09/09 14:42:10, bshe wrote: > desktop might be misleading. Perhaps remove "desktop" and change to "content > area image". Done. https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_shell_renderer.cc (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_shell_renderer.cc:30: const char* GetShaderSource(vr_shell::ShaderID shader) { On 2016/09/09 14:42:10, bshe wrote: > if you dont move the vr_shell namespace definition, you shouldn't need to add > vr_shell namespace in this function. Was there any specific reason you want to > move this anonymous namespace out of vr_shell? Style preferences, anonymous namespaces are usually outside of other namespaces. I can move it back if you prefer. https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_util.cc (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_util.cc:190: gvr::Mat4f InvertM(const gvr::Mat4f& mat) { On 2016/09/08 19:47:04, klausw wrote: > This appears to be unused. Since inverting a 4x4 matrix is inefficient and can > be numerically unstable, consider shelving this method unless there's a use case > where it's unavoidable? Done. https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_util.cc:368: * Make sure to invert it if ever used to compute the basis of a right-handed On 2016/09/09 14:42:10, bshe wrote: > On 2016/09/08 19:47:04, klausw wrote: > > Please update or delete this sentence - OpenGL is already right-handed > (x=right, > > y=up, z=towards camera). Assume you mean be careful if assuming +z = away from > > camera elsewhere. > > also: comment should be moved to .h file As you say Klaus, this comment doesn't make sense, and this function is clearly working correctly in our openGL prototype, so I'll just remove it. https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_util.cc:386: */ On 2016/09/09 14:42:10, bshe wrote: > ditto Done. https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_util.cc:519: // const float x = quat.qx; On 2016/09/09 14:42:10, bshe wrote: > remove these comments? Done. https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_util.h (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/vr_util.h:15: #define makeRectangularTextureBuffer(left, right, bottom, top) \ On 2016/09/09 14:42:10, bshe wrote: > see here: https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros > We need to conform with the coding style if we decide to use this macros Done.
On Sep 9, 2016 07:42, <bshe@chromium.org> wrote: > > > https://codereview.chromium.org/2301633002/diff/120001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java > (right): > > https://codereview.chromium.org/2301633002/diff/120001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:112: > if (mContentFrameListener.mFirstTex) { > is mFirstTex still necessary? I am not sure I understand why do you want > to call updateTextImage here? > I recall the boolean exist because we use to manually release tex image. > But we no longer do that in the code. > > https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... > File chrome/browser/android/vr_shell/ui_elements.cc (right): > > https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... > chrome/browser/android/vr_shell/ui_elements.cc:10: #include > "base/logging.h" > seems like unnecessary? > > > https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... > File chrome/browser/android/vr_shell/ui_elements.h (right): > > https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... > chrome/browser/android/vr_shell/ui_elements.h:41: gvr::Mat4f to_world_; > remove trailing _ for public members. Alternatively consider making this private with a const accessor? Having a user of the class accidentally modify a value could lead to inconsistent state and hard to find bugs. https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... > chrome/browser/android/vr_shell/ui_elements.h:42: gvr::Mat4f > from_world_; > ditto. > > https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... > chrome/browser/android/vr_shell/ui_elements.h:48: gvr::Quatf > orientation_ = {0.0f, 0.0f, 0.0f, 1.0f}; > ditto > > > https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... > File chrome/browser/android/vr_shell/vr_shell.cc (right): > > https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... > chrome/browser/android/vr_shell/vr_shell.cc:56: const > base::android::JavaParamRef<jobject>& obj) { > looks like you missed this one. remove base::android > > https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... > chrome/browser/android/vr_shell/vr_shell.cc:263: float zAnchorTranslate > = rect->anchor_z ? desktop_position_.z : 0; > nit: z_anchor_translate > > https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... > File chrome/browser/android/vr_shell/vr_shell.h (right): > > https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... > chrome/browser/android/vr_shell/vr_shell.h:48: // samplerExternalOES > texture data for desktop content image. > desktop might be misleading. Perhaps remove "desktop" and change to > "content area image". > > https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... > File chrome/browser/android/vr_shell/vr_shell_renderer.cc (right): > > https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... > chrome/browser/android/vr_shell/vr_shell_renderer.cc:30: const char* > GetShaderSource(vr_shell::ShaderID shader) { > if you dont move the vr_shell namespace definition, you shouldn't need > to add vr_shell namespace in this function. Was there any specific > reason you want to move this anonymous namespace out of vr_shell? > > > https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... > File chrome/browser/android/vr_shell/vr_util.cc (right): > > https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... > chrome/browser/android/vr_shell/vr_util.cc:368: * Make sure to invert it > if ever used to compute the basis of a right-handed > On 2016/09/08 19:47:04, klausw wrote: > > Please update or delete this sentence - OpenGL is already right-handed > (x=right, > > y=up, z=towards camera). Assume you mean be careful if assuming +z = > away from > > camera elsewhere. > > also: comment should be moved to .h file > > https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... > chrome/browser/android/vr_shell/vr_util.cc:386: */ > ditto > > https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... > chrome/browser/android/vr_shell/vr_util.cc:519: // const float x = > quat.qx; > remove these comments? > > https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... > File chrome/browser/android/vr_shell/vr_util.h (right): > > https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... > chrome/browser/android/vr_shell/vr_util.h:15: #define > makeRectangularTextureBuffer(left, right, bottom, top) \ > see here: > https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros > We need to conform with the coding style if we decide to use this macros > > https://codereview.chromium.org/2301633002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Well Chris is preparing a follow-up CL that adds the animation framework + tests which is already doing a bunch of refactoring. Let's do this in that CL instead? On Fri, Sep 9, 2016 at 11:22 AM, Klaus Weidner <klausw@google.com> wrote: > On Sep 9, 2016 07:42, <bshe@chromium.org> wrote: > > > > > > https://codereview.chromium.org/2301633002/diff/120001/ > chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java > > File > > chrome/android/java/src/org/chromium/chrome/browser/vr_ > shell/VrShell.java > > (right): > > > > https://codereview.chromium.org/2301633002/diff/120001/ > chrome/android/java/src/org/chromium/chrome/browser/vr_ > shell/VrShell.java#newcode112 > > chrome/android/java/src/org/chromium/chrome/browser/vr_ > shell/VrShell.java:112: > > if (mContentFrameListener.mFirstTex) { > > is mFirstTex still necessary? I am not sure I understand why do you want > > to call updateTextImage here? > > I recall the boolean exist because we use to manually release tex image. > > But we no longer do that in the code. > > > > https://codereview.chromium.org/2301633002/diff/120001/ > chrome/browser/android/vr_shell/ui_elements.cc > > File chrome/browser/android/vr_shell/ui_elements.cc (right): > > > > https://codereview.chromium.org/2301633002/diff/120001/ > chrome/browser/android/vr_shell/ui_elements.cc#newcode10 > > chrome/browser/android/vr_shell/ui_elements.cc:10: #include > > "base/logging.h" > > seems like unnecessary? > > > > > > https://codereview.chromium.org/2301633002/diff/120001/ > chrome/browser/android/vr_shell/ui_elements.h > > File chrome/browser/android/vr_shell/ui_elements.h (right): > > > > https://codereview.chromium.org/2301633002/diff/120001/ > chrome/browser/android/vr_shell/ui_elements.h#newcode41 > > chrome/browser/android/vr_shell/ui_elements.h:41: gvr::Mat4f to_world_; > > remove trailing _ for public members. > > Alternatively consider making this private with a const accessor? Having a > user of the class accidentally modify a value could lead to inconsistent > state and hard to find bugs. > > https://codereview.chromium.org/2301633002/diff/120001/ > chrome/browser/android/vr_shell/ui_elements.h#newcode42 > > chrome/browser/android/vr_shell/ui_elements.h:42: gvr::Mat4f > > from_world_; > > ditto. > > > > https://codereview.chromium.org/2301633002/diff/120001/ > chrome/browser/android/vr_shell/ui_elements.h#newcode48 > > chrome/browser/android/vr_shell/ui_elements.h:48: gvr::Quatf > > orientation_ = {0.0f, 0.0f, 0.0f, 1.0f}; > > ditto > > > > > > https://codereview.chromium.org/2301633002/diff/120001/ > chrome/browser/android/vr_shell/vr_shell.cc > > File chrome/browser/android/vr_shell/vr_shell.cc (right): > > > > https://codereview.chromium.org/2301633002/diff/120001/ > chrome/browser/android/vr_shell/vr_shell.cc#newcode56 > > chrome/browser/android/vr_shell/vr_shell.cc:56: const > > base::android::JavaParamRef<jobject>& obj) { > > looks like you missed this one. remove base::android > > > > https://codereview.chromium.org/2301633002/diff/120001/ > chrome/browser/android/vr_shell/vr_shell.cc#newcode263 > > chrome/browser/android/vr_shell/vr_shell.cc:263: float zAnchorTranslate > > = rect->anchor_z ? desktop_position_.z : 0; > > nit: z_anchor_translate > > > > https://codereview.chromium.org/2301633002/diff/120001/ > chrome/browser/android/vr_shell/vr_shell.h > > File chrome/browser/android/vr_shell/vr_shell.h (right): > > > > https://codereview.chromium.org/2301633002/diff/120001/ > chrome/browser/android/vr_shell/vr_shell.h#newcode48 > > chrome/browser/android/vr_shell/vr_shell.h:48: // samplerExternalOES > > texture data for desktop content image. > > desktop might be misleading. Perhaps remove "desktop" and change to > > "content area image". > > > > https://codereview.chromium.org/2301633002/diff/120001/ > chrome/browser/android/vr_shell/vr_shell_renderer.cc > > File chrome/browser/android/vr_shell/vr_shell_renderer.cc (right): > > > > https://codereview.chromium.org/2301633002/diff/120001/ > chrome/browser/android/vr_shell/vr_shell_renderer.cc#newcode30 > > chrome/browser/android/vr_shell/vr_shell_renderer.cc:30: const char* > > GetShaderSource(vr_shell::ShaderID shader) { > > if you dont move the vr_shell namespace definition, you shouldn't need > > to add vr_shell namespace in this function. Was there any specific > > reason you want to move this anonymous namespace out of vr_shell? > > > > > > https://codereview.chromium.org/2301633002/diff/120001/ > chrome/browser/android/vr_shell/vr_util.cc > > File chrome/browser/android/vr_shell/vr_util.cc (right): > > > > https://codereview.chromium.org/2301633002/diff/120001/ > chrome/browser/android/vr_shell/vr_util.cc#newcode368 > > chrome/browser/android/vr_shell/vr_util.cc:368: * Make sure to invert it > > if ever used to compute the basis of a right-handed > > On 2016/09/08 19:47:04, klausw wrote: > > > Please update or delete this sentence - OpenGL is already right-handed > > (x=right, > > > y=up, z=towards camera). Assume you mean be careful if assuming +z = > > away from > > > camera elsewhere. > > > > also: comment should be moved to .h file > > > > https://codereview.chromium.org/2301633002/diff/120001/ > chrome/browser/android/vr_shell/vr_util.cc#newcode386 > > chrome/browser/android/vr_shell/vr_util.cc:386: */ > > ditto > > > > https://codereview.chromium.org/2301633002/diff/120001/ > chrome/browser/android/vr_shell/vr_util.cc#newcode519 > > chrome/browser/android/vr_shell/vr_util.cc:519: // const float x = > > quat.qx; > > remove these comments? > > > > https://codereview.chromium.org/2301633002/diff/120001/ > chrome/browser/android/vr_shell/vr_util.h > > File chrome/browser/android/vr_shell/vr_util.h (right): > > > > https://codereview.chromium.org/2301633002/diff/120001/ > chrome/browser/android/vr_shell/vr_util.h#newcode15 > > chrome/browser/android/vr_shell/vr_util.h:15: #define > > makeRectangularTextureBuffer(left, right, bottom, top) \ > > see here: > > https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros > > We need to conform with the coding style if we decide to use this macros > > > > https://codereview.chromium.org/2301633002/ > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Of course, this was meant to be an optional suggestion. On Sep 9, 2016 08:36, "Michael Thiessen" <mthiesse@chromium.org> wrote: > Well Chris is preparing a follow-up CL that adds the animation framework + > tests which is already doing a bunch of refactoring. Let's do this in that > CL instead? > > On Fri, Sep 9, 2016 at 11:22 AM, Klaus Weidner <klausw@google.com> wrote: > >> On Sep 9, 2016 07:42, <bshe@chromium.org> wrote: >> > >> > >> > https://codereview.chromium.org/2301633002/diff/120001/chrom >> e/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java >> > File >> > chrome/android/java/src/org/chromium/chrome/browser/vr_shell >> /VrShell.java >> > (right): >> > >> > https://codereview.chromium.org/2301633002/diff/120001/chrom >> e/android/java/src/org/chromium/chrome/browser/vr_shell/ >> VrShell.java#newcode112 >> > chrome/android/java/src/org/chromium/chrome/browser/vr_shell >> /VrShell.java:112: >> > if (mContentFrameListener.mFirstTex) { >> > is mFirstTex still necessary? I am not sure I understand why do you want >> > to call updateTextImage here? >> > I recall the boolean exist because we use to manually release tex image. >> > But we no longer do that in the code. >> > >> > https://codereview.chromium.org/2301633002/diff/120001/chrom >> e/browser/android/vr_shell/ui_elements.cc >> > File chrome/browser/android/vr_shell/ui_elements.cc (right): >> > >> > https://codereview.chromium.org/2301633002/diff/120001/chrom >> e/browser/android/vr_shell/ui_elements.cc#newcode10 >> > chrome/browser/android/vr_shell/ui_elements.cc:10: #include >> > "base/logging.h" >> > seems like unnecessary? >> > >> > >> > https://codereview.chromium.org/2301633002/diff/120001/chrom >> e/browser/android/vr_shell/ui_elements.h >> > File chrome/browser/android/vr_shell/ui_elements.h (right): >> > >> > https://codereview.chromium.org/2301633002/diff/120001/chrom >> e/browser/android/vr_shell/ui_elements.h#newcode41 >> > chrome/browser/android/vr_shell/ui_elements.h:41: gvr::Mat4f to_world_; >> > remove trailing _ for public members. >> >> Alternatively consider making this private with a const accessor? Having >> a user of the class accidentally modify a value could lead to inconsistent >> state and hard to find bugs. >> >> https://codereview.chromium.org/2301633002/diff/120001/chrom >> e/browser/android/vr_shell/ui_elements.h#newcode42 >> > chrome/browser/android/vr_shell/ui_elements.h:42: gvr::Mat4f >> > from_world_; >> > ditto. >> > >> > https://codereview.chromium.org/2301633002/diff/120001/chrom >> e/browser/android/vr_shell/ui_elements.h#newcode48 >> > chrome/browser/android/vr_shell/ui_elements.h:48: gvr::Quatf >> > orientation_ = {0.0f, 0.0f, 0.0f, 1.0f}; >> > ditto >> > >> > >> > https://codereview.chromium.org/2301633002/diff/120001/chrom >> e/browser/android/vr_shell/vr_shell.cc >> > File chrome/browser/android/vr_shell/vr_shell.cc (right): >> > >> > https://codereview.chromium.org/2301633002/diff/120001/chrom >> e/browser/android/vr_shell/vr_shell.cc#newcode56 >> > chrome/browser/android/vr_shell/vr_shell.cc:56: const >> > base::android::JavaParamRef<jobject>& obj) { >> > looks like you missed this one. remove base::android >> > >> > https://codereview.chromium.org/2301633002/diff/120001/chrom >> e/browser/android/vr_shell/vr_shell.cc#newcode263 >> > chrome/browser/android/vr_shell/vr_shell.cc:263: float zAnchorTranslate >> > = rect->anchor_z ? desktop_position_.z : 0; >> > nit: z_anchor_translate >> > >> > https://codereview.chromium.org/2301633002/diff/120001/chrom >> e/browser/android/vr_shell/vr_shell.h >> > File chrome/browser/android/vr_shell/vr_shell.h (right): >> > >> > https://codereview.chromium.org/2301633002/diff/120001/chrom >> e/browser/android/vr_shell/vr_shell.h#newcode48 >> > chrome/browser/android/vr_shell/vr_shell.h:48: // samplerExternalOES >> > texture data for desktop content image. >> > desktop might be misleading. Perhaps remove "desktop" and change to >> > "content area image". >> > >> > https://codereview.chromium.org/2301633002/diff/120001/chrom >> e/browser/android/vr_shell/vr_shell_renderer.cc >> > File chrome/browser/android/vr_shell/vr_shell_renderer.cc (right): >> > >> > https://codereview.chromium.org/2301633002/diff/120001/chrom >> e/browser/android/vr_shell/vr_shell_renderer.cc#newcode30 >> > chrome/browser/android/vr_shell/vr_shell_renderer.cc:30: const char* >> > GetShaderSource(vr_shell::ShaderID shader) { >> > if you dont move the vr_shell namespace definition, you shouldn't need >> > to add vr_shell namespace in this function. Was there any specific >> > reason you want to move this anonymous namespace out of vr_shell? >> > >> > >> > https://codereview.chromium.org/2301633002/diff/120001/chrom >> e/browser/android/vr_shell/vr_util.cc >> > File chrome/browser/android/vr_shell/vr_util.cc (right): >> > >> > https://codereview.chromium.org/2301633002/diff/120001/chrom >> e/browser/android/vr_shell/vr_util.cc#newcode368 >> > chrome/browser/android/vr_shell/vr_util.cc:368: * Make sure to invert >> it >> > if ever used to compute the basis of a right-handed >> > On 2016/09/08 19:47:04, klausw wrote: >> > > Please update or delete this sentence - OpenGL is already right-handed >> > (x=right, >> > > y=up, z=towards camera). Assume you mean be careful if assuming +z = >> > away from >> > > camera elsewhere. >> > >> > also: comment should be moved to .h file >> > >> > https://codereview.chromium.org/2301633002/diff/120001/chrom >> e/browser/android/vr_shell/vr_util.cc#newcode386 >> > chrome/browser/android/vr_shell/vr_util.cc:386: */ >> > ditto >> > >> > https://codereview.chromium.org/2301633002/diff/120001/chrom >> e/browser/android/vr_shell/vr_util.cc#newcode519 >> > chrome/browser/android/vr_shell/vr_util.cc:519: // const float x = >> > quat.qx; >> > remove these comments? >> > >> > https://codereview.chromium.org/2301633002/diff/120001/chrom >> e/browser/android/vr_shell/vr_util.h >> > File chrome/browser/android/vr_shell/vr_util.h (right): >> > >> > https://codereview.chromium.org/2301633002/diff/120001/chrom >> e/browser/android/vr_shell/vr_util.h#newcode15 >> > chrome/browser/android/vr_shell/vr_util.h:15: #define >> > makeRectangularTextureBuffer(left, right, bottom, top) \ >> > see here: >> > https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros >> > We need to conform with the coding style if we decide to use this macros >> > >> > https://codereview.chromium.org/2301633002/ >> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm https://codereview.chromium.org/2301633002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:112: if (mContentFrameListener.mFirstTex) { On 2016/09/09 15:16:38, mthiesse wrote: > On 2016/09/09 14:42:10, bshe wrote: > > is mFirstTex still necessary? I am not sure I understand why do you want to > call > > updateTextImage here? > > I recall the boolean exist because we use to manually release tex image. But > we > > no longer do that in the code. > > The reason I've kept it is because we're not guaranteed to get an > onFrameAvailable call until we do something that updates the texture, like > scrolling. Currently we're changing the size when we switch to VR, so we're > probably getting an onFrameAvailable anyways, but that's an artifact of our > current implementation, and could potentially change in the future. I see. Perhaps add a comment here. https://codereview.chromium.org/2301633002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2301633002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:75: mActivity.setRequestedOrientation(ActivityInfo.SCREEN_ORIENTATION_LANDSCAPE); IIUC perhap change 75~79 to: if (!createVrShell()) return false; mActivity.setRequestedOrientation(ActivityInfo.SCREEN_ORIENTATION_LANDSCAPE); https://codereview.chromium.org/2301633002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:84: mInVr = true; optional: It feels like you can probably move all the above code to createVrShell? So the code could be easier to read.
https://codereview.chromium.org/2301633002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:112: if (mContentFrameListener.mFirstTex) { On 2016/09/09 16:58:42, bshe wrote: > On 2016/09/09 15:16:38, mthiesse wrote: > > On 2016/09/09 14:42:10, bshe wrote: > > > is mFirstTex still necessary? I am not sure I understand why do you want to > > call > > > updateTextImage here? > > > I recall the boolean exist because we use to manually release tex image. But > > we > > > no longer do that in the code. > > > > The reason I've kept it is because we're not guaranteed to get an > > onFrameAvailable call until we do something that updates the texture, like > > scrolling. Currently we're changing the size when we switch to VR, so we're > > probably getting an onFrameAvailable anyways, but that's an artifact of our > > current implementation, and could potentially change in the future. > > I see. Perhaps add a comment here. Done. https://codereview.chromium.org/2301633002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2301633002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:75: mActivity.setRequestedOrientation(ActivityInfo.SCREEN_ORIENTATION_LANDSCAPE); On 2016/09/09 16:58:42, bshe wrote: > IIUC perhap change 75~79 to: > if (!createVrShell()) return false; > mActivity.setRequestedOrientation(ActivityInfo.SCREEN_ORIENTATION_LANDSCAPE); Unfortunately that won't work. VrShell has to be started in landscape mode due to a bug in GVR that causes the render sizes to get completely borked if it's started in portrait. I'll leave a comment. https://codereview.chromium.org/2301633002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:84: mInVr = true; On 2016/09/09 16:58:43, bshe wrote: > optional: It feels like you can probably move all the above code to > createVrShell? So the code could be easier to read. No, creating VR shell is a logically a separate step from the rest of the things so I don't think that would make the code easier to read. enterVR performs all of the actions required to enter VR sequentially, and createVrShell is logically a necessary but not sufficient step.
https://codereview.chromium.org/2301633002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2301633002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:84: mInVr = true; On 2016/09/09 17:40:51, mthiesse wrote: > On 2016/09/09 16:58:43, bshe wrote: > > optional: It feels like you can probably move all the above code to > > createVrShell? So the code could be easier to read. > > No, creating VR shell is a logically a separate step from the rest of the things > so I don't think that would make the code easier to read. > > enterVR performs all of the actions required to enter VR sequentially, and > createVrShell is logically a necessary but not sufficient step. I didn't try to suggest combine the two functions into one. I meant from line 80. :p Some of the code could be in createVrShell such as addVrViews. To me, add vr views and related code is part of createVrShell conceptually. But I am totally fine leaving it as it is.
https://codereview.chromium.org/2301633002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2301633002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:84: mInVr = true; On 2016/09/09 18:02:38, bshe wrote: > On 2016/09/09 17:40:51, mthiesse wrote: > > On 2016/09/09 16:58:43, bshe wrote: > > > optional: It feels like you can probably move all the above code to > > > createVrShell? So the code could be easier to read. > > > > No, creating VR shell is a logically a separate step from the rest of the > things > > so I don't think that would make the code easier to read. > > > > enterVR performs all of the actions required to enter VR sequentially, and > > createVrShell is logically a necessary but not sufficient step. > > I didn't try to suggest combine the two functions into one. I meant from line > 80. :p > Some of the code could be in createVrShell such as addVrViews. To me, add vr > views > and related code is part of createVrShell conceptually. But I am totally fine > leaving it > as it is. Acknowledged.
lgtm % discussion on _. Thanks! https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/ui_elements.h (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/ui_elements.h:41: gvr::Mat4f to_world_; On 2016/09/09 14:42:10, bshe wrote: > remove trailing _ for public members. Hmm are you sure? IIUC the Google style guide requires _ on all class members. On structs you can omit the _. I might be incorrect though! https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/ui_elements.h:70: int content_texture_handle; On 2016/09/09 15:16:38, mthiesse wrote: > On 2016/09/09 06:28:29, David Trainor wrote: > > Need trailing _ for these > > These are public members, expected to be accessed directly (this class is > effectively just a struct with a complex constructor), so shouldn't they leave > out the trailing _? I feel like in that case we should just make the class a struct?
lgtm % discussion on _. Thanks!
https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/ui_elements.h (right): https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/ui_elements.h:41: gvr::Mat4f to_world_; On 2016/09/13 05:43:06, David Trainor wrote: > On 2016/09/09 14:42:10, bshe wrote: > > remove trailing _ for public members. > > Hmm are you sure? IIUC the Google style guide requires _ on all class members. > On structs you can omit the _. I might be incorrect though! Looking back at the style guide. You are right, it didn't require trailing _ only for private member. The example it gives only has private member though, which is confusing. What's more confusing is that for almost all the files that I have seen, I don't remember seeing any public member variable that end with a trailing _. Most of them are defined as private member and then wrapped by a setter/getter. That's probably why I have associated trailing _ with private member name. Perhaps we could just use struct here. https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/ui_elements.h:70: int content_texture_handle; On 2016/09/13 05:43:06, David Trainor wrote: > On 2016/09/09 15:16:38, mthiesse wrote: > > On 2016/09/09 06:28:29, David Trainor wrote: > > > Need trailing _ for these > > > > These are public members, expected to be accessed directly (this class is > > effectively just a struct with a complex constructor), so shouldn't they leave > > out the trailing _? > > I feel like in that case we should just make the class a struct? Agree. See. https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes It looks like some of the above classes should be struct.
On 2016/09/13 13:54:28, bshe wrote: > https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... > File chrome/browser/android/vr_shell/ui_elements.h (right): > > https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... > chrome/browser/android/vr_shell/ui_elements.h:41: gvr::Mat4f to_world_; > On 2016/09/13 05:43:06, David Trainor wrote: > > On 2016/09/09 14:42:10, bshe wrote: > > > remove trailing _ for public members. > > > > Hmm are you sure? IIUC the Google style guide requires _ on all class > members. > > On structs you can omit the _. I might be incorrect though! > > Looking back at the style guide. You are right, it didn't require trailing _ > only > for private member. The example it gives only has private member though, which > is > confusing. What's more confusing is that for almost all the files that I have > seen, > I don't remember seeing any public member variable that end with a trailing _. > Most > of them are defined as private member and then wrapped by a setter/getter. > That's > probably why I have associated trailing _ with private member name. > Perhaps we could just use struct here. > > https://codereview.chromium.org/2301633002/diff/120001/chrome/browser/android... > chrome/browser/android/vr_shell/ui_elements.h:70: int content_texture_handle; > On 2016/09/13 05:43:06, David Trainor wrote: > > On 2016/09/09 15:16:38, mthiesse wrote: > > > On 2016/09/09 06:28:29, David Trainor wrote: > > > > Need trailing _ for these > > > > > > These are public members, expected to be accessed directly (this class is > > > effectively just a struct with a complex constructor), so shouldn't they > leave > > > out the trailing _? > > > > I feel like in that case we should just make the class a struct? > > Agree. See. > https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes > It looks like some of the above classes should be struct. Alright, they're now structs.
The CQ bit was checked by mthiesse@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, klausw@google.com, bshe@chromium.org, dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2301633002/#ps220001 (title: "UiElements are now structs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Refactor Vr activity into ChromeTabbedActivity. This CL implements the following: 1. Deletes VrActivity, integrating VrShell into ChromeTabbedActivity. 2. Adds toggle for entering and exiting VR, simulating how NFC trigger will work. Known issues: 1. The current Compositor surface is displayed over top of the VR view. To test this, add "enable_vr_shell=true" in your gn args. These changes are pulled from our working experimental 'branch' found here: https://codereview.chromium.org/2248183002/ BUG=638642 ========== to ========== Refactor Vr activity into ChromeTabbedActivity. This CL implements the following: 1. Deletes VrActivity, integrating VrShell into ChromeTabbedActivity. 2. Adds toggle for entering and exiting VR, simulating how NFC trigger will work. Known issues: 1. The current Compositor surface is displayed over top of the VR view. To test this, add "enable_vr_shell=true" in your gn args. These changes are pulled from our working experimental 'branch' found here: https://codereview.chromium.org/2248183002/ BUG=638642 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Refactor Vr activity into ChromeTabbedActivity. This CL implements the following: 1. Deletes VrActivity, integrating VrShell into ChromeTabbedActivity. 2. Adds toggle for entering and exiting VR, simulating how NFC trigger will work. Known issues: 1. The current Compositor surface is displayed over top of the VR view. To test this, add "enable_vr_shell=true" in your gn args. These changes are pulled from our working experimental 'branch' found here: https://codereview.chromium.org/2248183002/ BUG=638642 ========== to ========== Refactor Vr activity into ChromeTabbedActivity. This CL implements the following: 1. Deletes VrActivity, integrating VrShell into ChromeTabbedActivity. 2. Adds toggle for entering and exiting VR, simulating how NFC trigger will work. Known issues: 1. The current Compositor surface is displayed over top of the VR view. To test this, add "enable_vr_shell=true" in your gn args. These changes are pulled from our working experimental 'branch' found here: https://codereview.chromium.org/2248183002/ BUG=638642 Committed: https://crrev.com/6a9e4415a09b41e101d84f8c129222cec0ecceb7 Cr-Commit-Position: refs/heads/master@{#418255} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/6a9e4415a09b41e101d84f8c129222cec0ecceb7 Cr-Commit-Position: refs/heads/master@{#418255}
Message was sent while issue was closed.
We need to fix this asap or revert this as it is preventing the menu on older android versions https://codereview.chromium.org/2301633002/diff/220001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2301633002/diff/220001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:230: 👓👓👓 Enter VR 👓👓👓 Just synced and this is crashing when I attempt to open the menu on my KK phone. W/dalvikvm( 9297): JNI WARNING: NewStringUTF input is not valid Modified UTF-8: illegal start byte 0xf0 W/dalvikvm( 9297): string: '👓👓👓 Enter VR 👓👓👓' W/dalvikvm( 9297): in Landroid/content/res/StringBlock;.nativeGetString:(II)Ljava/lang/String; (NewStringUTF) I/dalvikvm( 9297): at android.content.res.StringBlock.nativeGetString(Native Method) I/dalvikvm( 9297): at android.content.res.StringBlock.get(StringBlock.java:82) I/dalvikvm( 9297): at android.content.res.AssetManager.getPooledString(AssetManager.java:275) I/dalvikvm( 9297): at android.content.res.TypedArray.loadStringValueAt(TypedArray.java:727) I/dalvikvm( 9297): at android.content.res.TypedArray.getText(TypedArray.java:97) I/dalvikvm( 9297): at android.view.MenuInflater$MenuState.readItem(MenuInflater.java:348) I/dalvikvm( 9297): at android.view.MenuInflater.parseMenu(MenuInflater.java:160) I/dalvikvm( 9297): at android.view.MenuInflater.inflate(MenuInflater.java:110) I/dalvikvm( 9297): at android.widget.PopupMenu.inflate(PopupMenu.java:159) I/dalvikvm( 9297): at org.chromium.chrome.browser.appmenu.AppMenuHandler.showAppMenu(AppMenuHandler.java:104)
Message was sent while issue was closed.
https://codereview.chromium.org/2301633002/diff/220001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2301633002/diff/220001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:230: 👓👓👓 Enter VR 👓👓👓 On 2016/09/13 16:34:55, Ted C wrote: > Just synced and this is crashing when I attempt to open the menu on my KK phone. > > W/dalvikvm( 9297): JNI WARNING: NewStringUTF input is not valid Modified UTF-8: > illegal start byte 0xf0 > W/dalvikvm( 9297): string: '👓👓👓 Enter VR 👓👓👓' > W/dalvikvm( 9297): in > Landroid/content/res/StringBlock;.nativeGetString:(II)Ljava/lang/String; > (NewStringUTF) > > I/dalvikvm( 9297): at android.content.res.StringBlock.nativeGetString(Native > Method) > I/dalvikvm( 9297): at android.content.res.StringBlock.get(StringBlock.java:82) > I/dalvikvm( 9297): at > android.content.res.AssetManager.getPooledString(AssetManager.java:275) > I/dalvikvm( 9297): at > android.content.res.TypedArray.loadStringValueAt(TypedArray.java:727) > I/dalvikvm( 9297): at > android.content.res.TypedArray.getText(TypedArray.java:97) > I/dalvikvm( 9297): at > android.view.MenuInflater$MenuState.readItem(MenuInflater.java:348) > I/dalvikvm( 9297): at > android.view.MenuInflater.parseMenu(MenuInflater.java:160) > I/dalvikvm( 9297): at android.view.MenuInflater.inflate(MenuInflater.java:110) > I/dalvikvm( 9297): at android.widget.PopupMenu.inflate(PopupMenu.java:159) > I/dalvikvm( 9297): at > org.chromium.chrome.browser.appmenu.AppMenuHandler.showAppMenu(AppMenuHandler.java:104) https://codereview.chromium.org/2337103004/ |
