Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2319863005:
Implement new compositor and ContentViewCore reparenting for VR Shell. (Closed)
DescriptionImplement new compositor and ContentViewCore reparenting for VR Shell.
BUG=638642
Committed: https://crrev.com/5584eb0f2c325d87582fa1f1d5a57cca1522f80b
Cr-Commit-Position: refs/heads/master@{#419927}
Patch Set 1 #
Total comments: 22
Patch Set 2 : Address Comments #
Total comments: 30
Patch Set 3 : Unsolicited cleanup #Patch Set 4 : Address comments #
Total comments: 13
Patch Set 5 : Address comments #Patch Set 6 : Rebase onto ToT #
Total comments: 18
Patch Set 7 : Address comments. #Patch Set 8 : Rebase onto ToT #Dependent Patchsets: Messages
Total messages: 39 (11 generated)
mthiesse@chromium.org changed reviewers: + bshe@chromium.org, cjgrant@chromium.org, klausw@chromium.org
PTAL before I send this out for owners review.
https://codereview.chromium.org/2319863005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java (right): https://codereview.chromium.org/2319863005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:58: private ContentViewCore mContentContentViewCore; ContentContent is really confusing. It might just be a relic of the current naming, but what is the first 'Content' supposed to be referring to? I'm assuming the second is actually part of the 'ContentViewCore', but that doesn't seem to hold for the ViewGroup naming below. https://codereview.chromium.org/2319863005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:95: mContentContentViewCore.disableInput(); I'm not sure of the implications, but wouldn't you want to disable input before removing views etc? Conversely below it feels like enablingInput should be the last thing you do after restoring the views instead of the first thing. If there are specific reasons it works like that feel free to ignore (but I would be interested in knowing why it is like that). https://codereview.chromium.org/2319863005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:144: final int width = mContentContentView.getWidth(); Is this width and height still tied to the physical device dimensions or can/are we overriding them? https://codereview.chromium.org/2319863005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:155: nativeContentSurfaceChanged(mNativeVrShell, 4, width, height, What's the '4' for? Can we get a variable name or constant for it? Also it feels a little odd to call created and then immediately changed, do we have the ability to combine these in some way (like having the created call take the same params)?
https://codereview.chromium.org/2319863005/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/simple_compositor_view.cc (right): https://codereview.chromium.org/2319863005/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/simple_compositor_view.cc:15: SimpleCompositorView::SimpleCompositorView(ui::WindowAndroid* window) Does Google style allow in-class initializers? https://codereview.chromium.org/2319863005/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/simple_compositor_view.cc:31: current_surface_format_ = 0; Consider a constant instead of 0, as suggested elsewhere? https://codereview.chromium.org/2319863005/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/simple_compositor_view.cc:46: current_surface_format_ = 0; (same as above) https://codereview.chromium.org/2319863005/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2319863005/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:37: static constexpr float kScreenWidthMeters = 16.0f / 9.0f; This constant looks misnamed (it appears to be a ratio rather than a value in meters). kScreenWidthHeightRatio? https://codereview.chromium.org/2319863005/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:63: content_core_->GetWebContents() Shouldn't this wrap on two lines rather than four? Is it done this way for readability? https://codereview.chromium.org/2319863005/diff/1/ui/android/java/src/org/chr... File ui/android/java/src/org/chromium/ui/base/VRWindowAndroid.java (right): https://codereview.chromium.org/2319863005/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/base/VRWindowAndroid.java:130: Arrays.fill(grantResults, PackageManager.PERMISSION_DENIED); s/ PackageManager/ PackageManager/ https://codereview.chromium.org/2319863005/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/base/VRWindowAndroid.java:132: return; Unnecessary return?
Thanks guys. https://codereview.chromium.org/2319863005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java (right): https://codereview.chromium.org/2319863005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:58: private ContentViewCore mContentContentViewCore; On 2016/09/09 00:00:27, amp wrote: > ContentContent is really confusing. It might just be a relic of the current > naming, but what is the first 'Content' supposed to be referring to? I'm > assuming the second is actually part of the 'ContentViewCore', but that doesn't > seem to hold for the ViewGroup naming below. This is not so much a 'relic' as a differentiation between the content (3rd party) contentviewcore and the ui (1st party) contentviewcore that will be up for review very soon too. Hopefully I've made this less confusing by deleting mContentContentView, as there was no real reason to hold onto it. I also renamed mOldContentContentView and left a comment. https://codereview.chromium.org/2319863005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:95: mContentContentViewCore.disableInput(); On 2016/09/09 00:00:27, amp wrote: > I'm not sure of the implications, but wouldn't you want to disable input before > removing views etc? > > Conversely below it feels like enablingInput should be the last thing you do > after restoring the views instead of the first thing. > > If there are specific reasons it works like that feel free to ignore (but I > would be interested in knowing why it is like that). You're right that disabling/enabling input should be the first and last things I do. I didn't put to much thought into it because in practice it doesn't make a big difference. With the orientation change also going on, the user's unlikely to be able to tap anything they're intending to tap anyways. https://codereview.chromium.org/2319863005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:144: final int width = mContentContentView.getWidth(); On 2016/09/09 00:00:27, amp wrote: > Is this width and height still tied to the physical device dimensions or can/are > we overriding them? This is tied to the physical dimensions, yes. This CL lays some of the groundwork for decoupling the physical dimensions and content dimensions, but lots of further work will be required. https://codereview.chromium.org/2319863005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:155: nativeContentSurfaceChanged(mNativeVrShell, 4, width, height, On 2016/09/09 00:00:27, amp wrote: > What's the '4' for? Can we get a variable name or constant for it? > > Also it feels a little odd to call created and then immediately changed, do we > have the ability to combine these in some way (like having the created call take > the same params)? I've deleted format, and SurfaceCreated altogether - they were relics of the past that I missed cleaning up. https://codereview.chromium.org/2319863005/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/simple_compositor_view.cc (right): https://codereview.chromium.org/2319863005/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/simple_compositor_view.cc:15: SimpleCompositorView::SimpleCompositorView(ui::WindowAndroid* window) On 2016/09/09 13:56:32, cjgrant wrote: > Does Google style allow in-class initializers? I think so. I'll use them because I also prefer them. https://codereview.chromium.org/2319863005/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/simple_compositor_view.cc:31: current_surface_format_ = 0; On 2016/09/09 13:56:32, cjgrant wrote: > Consider a constant instead of 0, as suggested elsewhere? Done. https://codereview.chromium.org/2319863005/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/simple_compositor_view.cc:46: current_surface_format_ = 0; On 2016/09/09 13:56:32, cjgrant wrote: > (same as above) Done. https://codereview.chromium.org/2319863005/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2319863005/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:37: static constexpr float kScreenWidthMeters = 16.0f / 9.0f; On 2016/09/09 13:56:33, cjgrant wrote: > This constant looks misnamed (it appears to be a ratio rather than a value in > meters). kScreenWidthHeightRatio? Done. https://codereview.chromium.org/2319863005/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:63: content_core_->GetWebContents() On 2016/09/09 13:56:32, cjgrant wrote: > Shouldn't this wrap on two lines rather than four? Is it done this way for > readability? This is just what auto-format does. I'll make it two. https://codereview.chromium.org/2319863005/diff/1/ui/android/java/src/org/chr... File ui/android/java/src/org/chromium/ui/base/VRWindowAndroid.java (right): https://codereview.chromium.org/2319863005/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/base/VRWindowAndroid.java:130: Arrays.fill(grantResults, PackageManager.PERMISSION_DENIED); On 2016/09/09 13:56:33, cjgrant wrote: > s/ PackageManager/ PackageManager/ Done. https://codereview.chromium.org/2319863005/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/base/VRWindowAndroid.java:132: return; On 2016/09/09 13:56:33, cjgrant wrote: > Unnecessary return? Done.
mthiesse@chromium.org changed reviewers: + sievers@chromium.org, tedchoc@chromium.org
+sievers, tedchoc PTAL, this implements the plan we discussed regarding creating a new compositor for VR Shell content. It doesn't decouple width/height from the Android view system yet, but that requires more work and will be done as a followup.
https://codereview.chromium.org/2319863005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2319863005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1602: public void setClankUIVisibilityForVR(int visibility) { nit: Using Clank in function name is probably not a good idea. It is a internal code name. https://codereview.chromium.org/2319863005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java (right): https://codereview.chromium.org/2319863005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:58: private ContentViewCore mContentContentViewCore; It is a little bit uncommon to have two Content in the name. Is there a reason that you don't want to name it mContentViewCore? Or perhaps mActiveContentViewCore since this is the active CVC? https://codereview.chromium.org/2319863005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:61: private VRWindowAndroid mContentWindowAndroid; mVrWindowAndroid to match the class name? https://codereview.chromium.org/2319863005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:85: mContentWindowAndroid = new VRWindowAndroid(mActivity); I suspect that there are some unexpected behaviors when you create a new WindowAndroid in this activity and only tells active CVC. It looks like WindowAndroid is usually associated with a ChromeActivity, and we didn't update it in the ChromeActivity that VrShell belongs to. There are also a few other places that needs to know this WindowAndroid: https://cs.chromium.org/search/?q=getWindowAndroid+file:.*java&sq=package:chr... Most of them are UIs that we probably dont need or have no way to trigger in VR. But still it might worth creating a bug to track potential issues. https://codereview.chromium.org/2319863005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2319863005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:75: Tab tab = mActivity.getActivityTab(); Did you try cold start browser? The active tab might not be ready when you call this function. https://codereview.chromium.org/2319863005/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/simple_compositor_view.h (right): https://codereview.chromium.org/2319863005/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/simple_compositor_view.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. This is no longer a view, perhaps rename to simple_compositor.h/cc? https://codereview.chromium.org/2319863005/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.h (right): https://codereview.chromium.org/2319863005/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:34: content::ContentViewCore* content_core, nit:s/content_core/content_view_core https://codereview.chromium.org/2319863005/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:94: content::ContentViewCore* content_core_; nit:s/content_core_/content_view_core_ https://codereview.chromium.org/2319863005/diff/20001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/VRWindowAndroid.java (right): https://codereview.chromium.org/2319863005/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/VRWindowAndroid.java:1: // Copyright 2013 The Chromium Authors. All rights reserved. s/2013/2016 https://codereview.chromium.org/2319863005/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/VRWindowAndroid.java:3: // found in the LICENSE file. Could this file be moved to chrome/ folder? We could then guard it with vr_shell flag. It would be easier for us to measure binary size later on. https://codereview.chromium.org/2319863005/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/VRWindowAndroid.java:43: protected void registerKeyboardVisibilityCallbacks() { Do we need this override? We probably don't need to detect virtual keyboard as we wont need to use it in VR. https://codereview.chromium.org/2319863005/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/VRWindowAndroid.java:52: public void onVisibilityChanged(boolean visible) { nit: unnecessary override https://codereview.chromium.org/2319863005/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/VRWindowAndroid.java:57: protected void unregisterKeyboardVisibilityCallbacks() { same as registerKeyboardVisibilityCallbacks, do you need this override? https://codereview.chromium.org/2319863005/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/VRWindowAndroid.java:102: keyboardVisibilityPossiblyChanged(UiUtils.isKeyboardShowing(getActivity().get(), v)); If you don't register listener, you probably don't need to implement this. https://codereview.chromium.org/2319863005/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/VRWindowAndroid.java:106: // TODO(mthiesse): Should we add some UI to ask the user to exit VR, then accept the permission? If Chrome use system permission popup, I assume this problem could be solved by GVR? I remember it is on their roadmap.
https://codereview.chromium.org/2319863005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2319863005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1602: public void setClankUIVisibilityForVR(int visibility) { On 2016/09/12 16:39:56, bshe wrote: > nit: Using Clank in function name is probably not a good idea. It is a internal > code name. Done. https://codereview.chromium.org/2319863005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java (right): https://codereview.chromium.org/2319863005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:58: private ContentViewCore mContentContentViewCore; On 2016/09/12 16:39:56, bshe wrote: > It is a little bit uncommon to have two Content in the name. Is there a reason > that you don't want to name it mContentViewCore? Or perhaps > mActiveContentViewCore since this is the active CVC? To distinguish it from the ui ContentViewCore that doesn't exist yet - but will soon (it exists in the prototype). If you prefer I can name it mContentViewCore for now, and rename it again later, but that seems like a waste of time. https://codereview.chromium.org/2319863005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:61: private VRWindowAndroid mContentWindowAndroid; On 2016/09/12 16:39:56, bshe wrote: > mVrWindowAndroid to match the class name? Done. https://codereview.chromium.org/2319863005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:85: mContentWindowAndroid = new VRWindowAndroid(mActivity); On 2016/09/12 16:39:56, bshe wrote: > I suspect that there are some unexpected behaviors when you create a new > WindowAndroid in this activity and only tells active CVC. > It looks like WindowAndroid is usually associated with a ChromeActivity, and we > didn't update it in the ChromeActivity that VrShell belongs to. There are also a > few other places that needs to know this WindowAndroid: > https://cs.chromium.org/search/?q=getWindowAndroid+file:.*java&sq=package:chr... > Most of them are UIs that we probably dont need or have no way to trigger in VR. > But still it might worth creating a bug to track potential issues. I'm not sure what purpose creating a bug would serve, but for now I've updated the WindowAndroid for the current tab, and left a TODO to investigate updating the WindowAndroid in ChromeActivity (though it looks like nothing we care about is using it). In the future when we support multiple tabs we'll probably want to change it in ChromeActivity and propagate to all tabs. https://codereview.chromium.org/2319863005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2319863005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:75: Tab tab = mActivity.getActivityTab(); On 2016/09/12 16:39:56, bshe wrote: > Did you try cold start browser? The active tab might not be ready when you call > this function. Cold start of the browser works just fine for entering VR - it's just not pretty at the moment. Even if the tab isn't ready yet that's fine for now - we just won't enter VR. https://codereview.chromium.org/2319863005/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/simple_compositor_view.h (right): https://codereview.chromium.org/2319863005/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/simple_compositor_view.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/09/12 16:39:56, bshe wrote: > This is no longer a view, perhaps rename to simple_compositor.h/cc? Done. https://codereview.chromium.org/2319863005/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.h (right): https://codereview.chromium.org/2319863005/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:34: content::ContentViewCore* content_core, On 2016/09/12 16:39:56, bshe wrote: > nit:s/content_core/content_view_core Done. https://codereview.chromium.org/2319863005/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.h:94: content::ContentViewCore* content_core_; On 2016/09/12 16:39:56, bshe wrote: > nit:s/content_core_/content_view_core_ Done. https://codereview.chromium.org/2319863005/diff/20001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/VRWindowAndroid.java (right): https://codereview.chromium.org/2319863005/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/VRWindowAndroid.java:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2016/09/12 16:39:56, bshe wrote: > s/2013/2016 Done. https://codereview.chromium.org/2319863005/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/VRWindowAndroid.java:3: // found in the LICENSE file. On 2016/09/12 16:39:56, bshe wrote: > Could this file be moved to chrome/ folder? We could then guard it with vr_shell > flag. It would be easier for us to measure binary size later on. Done. https://codereview.chromium.org/2319863005/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/VRWindowAndroid.java:43: protected void registerKeyboardVisibilityCallbacks() { On 2016/09/12 16:39:56, bshe wrote: > Do we need this override? We probably don't need to detect virtual keyboard as > we wont need to use it in VR. Done. https://codereview.chromium.org/2319863005/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/VRWindowAndroid.java:52: public void onVisibilityChanged(boolean visible) { On 2016/09/12 16:39:56, bshe wrote: > nit: unnecessary override Done. https://codereview.chromium.org/2319863005/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/VRWindowAndroid.java:57: protected void unregisterKeyboardVisibilityCallbacks() { On 2016/09/12 16:39:56, bshe wrote: > same as registerKeyboardVisibilityCallbacks, do you need this override? Done. https://codereview.chromium.org/2319863005/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/VRWindowAndroid.java:102: keyboardVisibilityPossiblyChanged(UiUtils.isKeyboardShowing(getActivity().get(), v)); On 2016/09/12 16:39:56, bshe wrote: > If you don't register listener, you probably don't need to implement this. Done. https://codereview.chromium.org/2319863005/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/VRWindowAndroid.java:106: // TODO(mthiesse): Should we add some UI to ask the user to exit VR, then accept the permission? On 2016/09/12 16:39:56, bshe wrote: > If Chrome use system permission popup, I assume this problem could be solved by > GVR? I remember it is on their roadmap. Added that possibility to the comment.
https://codereview.chromium.org/2319863005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2319863005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:148: mActivity.setUIVisibilityForVR(View.GONE); I read this now as saying it's setting the VR visibility to GONE. How about something like setDefaultUIVisibility? https://codereview.chromium.org/2319863005/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2319863005/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:45: ui::WindowAndroid* content_view_core) Why is the WindowAndroid the content_view_core? Isn't that the param above?
haven't had a chance to fully go through this, but it seems like we could be isolating this more to CTA and VR mode instead of leaking it out elsewhere https://codereview.chromium.org/2319863005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2319863005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1608: mControlContainer.setVisibility(visibility); can your surfaceview sit on top? do you need to hide the toolbar/etc.. in that case? I was under the impression it either needed to be at the very top or very bottom and we don't really need to overlay any android widgets so the top would avoid this. ContentVideoView I "believe" does this. We should look at what they are doing at least and see if we can share the same behavior as they should be doing similar things in my opinion. https://codereview.chromium.org/2319863005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrWindowAndroid.java (right): https://codereview.chromium.org/2319863005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrWindowAndroid.java:32: public VrWindowAndroid(Context context) { Changing the window seems nontrivial...maybe we should create a WindowAndroid specific to CTA that is aware of VR. Then this logic could exist there. Or you could overwrite Activity#onRequestPermissionsResult in CTA and ignore there. https://codereview.chromium.org/2319863005/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2319863005/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:695: public void updateNativeWindowAndroid(WindowAndroid windowAndroid) { this doesn't seem to be used https://codereview.chromium.org/2319863005/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3311: public void disableInput() { this really doesn't belong in the content layer. This is very client specific, so it should be handled up in the chrome layer. If you are in VR mode, do you want to ignore all touch events? Could we just override: https://developer.android.com/reference/android/app/Activity.html#dispatchTou... And drop them all if we are in VR mode?
https://codereview.chromium.org/2319863005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2319863005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1608: mControlContainer.setVisibility(visibility); On 2016/09/12 23:33:11, Ted C wrote: > can your surfaceview sit on top? do you need to hide the toolbar/etc.. in that > case? I was under the impression it either needed to be at the very top or very > bottom and we don't really need to overlay any android widgets so the top would > avoid this. > > ContentVideoView I "believe" does this. We should look at what they are doing > at least and see if we can share the same behavior as they should be doing > similar things in my opinion. So I've duplicated what ContentVideoView does and I still need to hide the views in ChromeTabbedActivity. I'm really unclear as to why this is - I think it's an issue with how GvrLayout adds its views to the view hierarchy, outside of my control. What I see is the vertical center line for the stereo view sitting on top of the compositorViewHolder, with the stereo content sitting behind the compositorViewHolder. The GvrLayout code is here: https://goto.google.com/gvrlayout-code-location. Are they doing something incorrectly? Is this a bug in their library? (If so, it probably won't be fixed any time soon)
Thanks Ted https://codereview.chromium.org/2319863005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2319863005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:148: mActivity.setUIVisibilityForVR(View.GONE); On 2016/09/12 22:36:53, amp wrote: > I read this now as saying it's setting the VR visibility to GONE. > > How about something like setDefaultUIVisibility? I think setUIVisibilityForVR makes sense in the context of ChromeTabbedActivity, whereas setDefaultUIVisibility doesn't really make sense as we're not setting any defaults. https://codereview.chromium.org/2319863005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrWindowAndroid.java (right): https://codereview.chromium.org/2319863005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrWindowAndroid.java:32: public VrWindowAndroid(Context context) { On 2016/09/12 23:33:11, Ted C wrote: > Changing the window seems nontrivial...maybe we should create a WindowAndroid > specific to CTA that is aware of VR. Then this logic could exist there. > > Or you could overwrite Activity#onRequestPermissionsResult in CTA and ignore > there. I'm not sure I understand this comment, is this not exactly what you said there? A WindowAndroid specific to CTA, that is aware of VR? If you mean that you don't want to change the WindowAndroid, I was under the impression that this is a requirement for creating a new compositor and that there's no way around this. Perhaps sievers@ can comment, but I think this is what he wanted us to implement. Also, I think any non-trivial behavior is only taking place while in VR mode; we're restoring all state when exiting VR so there shouldn't be any added complexity to normal CTA behavior. https://codereview.chromium.org/2319863005/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2319863005/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:45: ui::WindowAndroid* content_view_core) On 2016/09/12 22:36:53, amp wrote: > Why is the WindowAndroid the content_view_core? Isn't that the param above? Whoops, typo. https://codereview.chromium.org/2319863005/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2319863005/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:695: public void updateNativeWindowAndroid(WindowAndroid windowAndroid) { On 2016/09/12 23:33:11, Ted C wrote: > this doesn't seem to be used Missed that in cleanup https://codereview.chromium.org/2319863005/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3311: public void disableInput() { On 2016/09/12 23:33:11, Ted C wrote: > this really doesn't belong in the content layer. This is very client specific, > so it should be handled up in the chrome layer. > > If you are in VR mode, do you want to ignore all touch events? > > Could we just override: > https://developer.android.com/reference/android/app/Activity.html#dispatchTou... > > And drop them all if we are in VR mode? dispatchTouchEvent overrides at ChromeTabbedActivity level don't work due to how GVR adds its views. I've placed the CVC inside another framelayout inside of VrShell instead, which overrides dispatchTouchEvent.
tedchoc@chromium.org changed reviewers: + qinmin@chromium.org
+qinmin for ContentVideoView questions. SurfaceView's are definitely not my area of expertise, but as far as I'm aware, you can have a SurfaceView at the far back for front of the view stacking/compositing order of Android. If it's at the back, you can then specify setZOrderMediaOverlay(true), which allows two SurfaceViews to exist at the bottom of the stack, but you can have them draw over each other. I "believe" this is how we handle media controls being drawn by the website while we render our video in a completely separate surface that is actually the bottom Surface. I don't know enough about the VR surface behavior to say how VR should behave (a doc sure would be nice about the longer term plans). Can it render at the bottom? Is it actually a peer to the CompositorView? Does it need to be? If you enter fullscreen video and look in hierarchy viewer, are the other views hidden or visible? Maybe there is some signal we should be using to accomplish what you want that already exists (i.e. entering html5 fullscreen). Again, it's hard for me to say what is right w/o broader context.
https://codereview.chromium.org/2319863005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2319863005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1608: mControlContainer.setVisibility(visibility); On 2016/09/13 14:53:48, mthiesse wrote: > On 2016/09/12 23:33:11, Ted C wrote: > > can your surfaceview sit on top? do you need to hide the toolbar/etc.. in > that > > case? I was under the impression it either needed to be at the very top or > very > > bottom and we don't really need to overlay any android widgets so the top > would > > avoid this. > > > > ContentVideoView I "believe" does this. We should look at what they are doing > > at least and see if we can share the same behavior as they should be doing > > similar things in my opinion. > > So I've duplicated what ContentVideoView does and I still need to hide the views > in ChromeTabbedActivity. I'm really unclear as to why this is - I think it's an > issue with how GvrLayout adds its views to the view hierarchy, outside of my > control. What I see is the vertical center line for the stereo view sitting on > top of the compositorViewHolder, with the stereo content sitting behind the > compositorViewHolder. > > The GvrLayout code is here: https://goto.google.com/gvrlayout-code-location. Are > they doing something incorrectly? Is this a bug in their library? (If so, it > probably won't be fixed any time soon) CompositorView calls setZOrderMediaOverlay(), so it is behind the window, but above the ContentVideoView. So your change is to add a view on top of the CompositorView and one beneath it, and hide the Compositor View?
On 2016/09/13 19:42:33, Ted C wrote: > +qinmin for ContentVideoView questions. > > SurfaceView's are definitely not my area of expertise, but as far as I'm aware, > you can have a SurfaceView at the far back for front of the view > stacking/compositing order of Android. > > If it's at the back, you can then specify setZOrderMediaOverlay(true), which > allows two SurfaceViews to exist at the bottom of the stack, but you can have > them draw over each other. > > I "believe" this is how we handle media controls being drawn by the website > while we render our video in a completely separate surface that is actually the > bottom Surface. > > I don't know enough about the VR surface behavior to say how VR should behave (a > doc sure would be nice about the longer term plans). Can it render at the > bottom? Is it actually a peer to the CompositorView? Does it need to be? > > If you enter fullscreen video and look in hierarchy viewer, are the other views > hidden or visible? Maybe there is some signal we should be using to accomplish > what you want that already exists (i.e. entering html5 fullscreen). Again, it's > hard for me to say what is right w/o broader context. The VR Surface isn't a peer to the CompositorView (if I'm understanding you correctly) - we want the CompositorView and every other Java View gone (not rendering, not consuming resources, etc.). I think even if we manage to get the stereo surface (whose view creation/addition is out of our control) to draw on top of the CompositorView and Control container, we would still want to hide them. If I use the fullscreen manager to enter overlay video mode, the CompositorView does disappear (how this happens is fairly magical - the only thing the fullscreen manager does to it is set its surface pixel format to translucent), but the control container sticks around (I can't figure out what causes it to go away in the VideoView case). Changing the pixel format of the CompositorView (and its native compositor) seem like pure overhead and things we don't actually want to do when entering VR. To try to answer your question about longer term plans and how VR should behave - VR would like to pretend it's not even running on Android. The clank UI should all effectively pause and disappear, to be restored when exiting VR. From Clank's perspective it should behave as though it were paused, and on resume its tab state and WebContents state may have changed.
On 2016/09/14 18:45:24, qinmin wrote: > CompositorView calls setZOrderMediaOverlay(), so it is behind the window, but > above the ContentVideoView. So your change is to add a view on top of the > CompositorView and one beneath it, and hide the Compositor View? My change intends to add views only on top of the CompositorView, but GVR creates a SurfaceView with setZOrderMediaOverlay, and that SurfaceView, while being on top of the CompositorView in the view hierarchy, is being drawn beneath it. So I hide the CompositorView (and ControlContainer) to get the GVR SurfaceView to show up.
tedchoc@chromium.org changed reviewers: + mdjones@chromium.org
+mdjones for the vr compositor logic https://codereview.chromium.org/2319863005/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2319863005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1598: updateWindowAndroid(activity.getWindowAndroid()); can we use this above in detach? then we wouldn't need to change the content layer, in your code, you could do updateWindowAndroid(null), updateWindowAndroid(VrWindow). I think longer term, we are going to want to break down detach and attach to handle this case better where you could use more of this code. If we do end up needing a separate activity, we'll want the ability to detach the window w/o the related mutating the tab model/broadcasting an intent. https://codereview.chromium.org/2319863005/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java (right): https://codereview.chromium.org/2319863005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:57: private ContentViewCoreHolder mContentViewCoreHolder; Container is probably a more common word than Holder for a View construct https://codereview.chromium.org/2319863005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:62: private ContentViewCore mContentCVC; mCVC is all you need https://codereview.chromium.org/2319863005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:63: // The parent of the @ContentView for mContentContentViewCore. Non-standard comment. I think, "The non-VR container view for mContentCVC" is probably better. ContentContentViewParent also is a bit weird. Maybe TabContentViewParent or mOriginalContentViewParent (which would better align with your WindowAndroid naming) https://codereview.chromium.org/2319863005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:65: private VrWindowAndroid mContentVRWindowAndroid; mVRWindowAndroid is sufficient https://codereview.chromium.org/2319863005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:132: mContentCVParent.addView(contentContentView, new FrameLayout.LayoutParams( this is not a safe call to do. You are wiping out the previous layout params and index as part of this. We either need to cache both of these (or figure out a way to use the existing reparenting path). In the multiple activity approach, we'll need to make reparenting work for this, so we might want to see if we can just do it now. If not, cache this and add a TODO to replace with proper reparenting. https://codereview.chromium.org/2319863005/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrWindowAndroid.java (right): https://codereview.chromium.org/2319863005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrWindowAndroid.java:96: extra blank line https://codereview.chromium.org/2319863005/diff/100001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2319863005/diff/100001/content/browser/androi... content/browser/android/content_view_core_impl.cc:280: FOR_EACH_OBSERVER(ContentViewCoreImplObserver, observer_list_, per my other comment, let's try to avoid this by setting it to null as part of the VR path.
https://codereview.chromium.org/2319863005/diff/100001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_compositor.cc (right): https://codereview.chromium.org/2319863005/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_compositor.cc:29: void VrCompositor::SetLayer(content::ContentViewCore* core) { Will this ever be called more than once for this object? If so, the reparenting logic should be somewhere other than the destructor.
Thanks guys. https://codereview.chromium.org/2319863005/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2319863005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1598: updateWindowAndroid(activity.getWindowAndroid()); On 2016/09/20 19:57:06, Ted C wrote: > can we use this above in detach? then we wouldn't need to change the content > layer, in your code, you could do updateWindowAndroid(null), > updateWindowAndroid(VrWindow). > > I think longer term, we are going to want to break down detach and attach to > handle this case better where you could use more of this code. > > If we do end up needing a separate activity, we'll want the ability to detach > the window w/o the related mutating the tab model/broadcasting an intent. Acknowledged. https://codereview.chromium.org/2319863005/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java (right): https://codereview.chromium.org/2319863005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:57: private ContentViewCoreHolder mContentViewCoreHolder; On 2016/09/20 19:57:07, Ted C wrote: > Container is probably a more common word than Holder for a View construct Done. https://codereview.chromium.org/2319863005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:62: private ContentViewCore mContentCVC; On 2016/09/20 19:57:06, Ted C wrote: > mCVC is all you need Done. https://codereview.chromium.org/2319863005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:63: // The parent of the @ContentView for mContentContentViewCore. On 2016/09/20 19:57:07, Ted C wrote: > Non-standard comment. > > I think, "The non-VR container view for mContentCVC" is probably better. > ContentContentViewParent also is a bit weird. Maybe TabContentViewParent or > mOriginalContentViewParent (which would better align with your WindowAndroid > naming) Done. https://codereview.chromium.org/2319863005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:65: private VrWindowAndroid mContentVRWindowAndroid; On 2016/09/20 19:57:06, Ted C wrote: > mVRWindowAndroid is sufficient Done. https://codereview.chromium.org/2319863005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:132: mContentCVParent.addView(contentContentView, new FrameLayout.LayoutParams( On 2016/09/20 19:57:07, Ted C wrote: > this is not a safe call to do. You are wiping out the previous layout params > and index as part of this. > > We either need to cache both of these (or figure out a way to use the existing > reparenting path). In the multiple activity approach, we'll need to make > reparenting work for this, so we might want to see if we can just do it now. > > If not, cache this and add a TODO to replace with proper reparenting. For now I'll cache with a TODO. I briefly looked into properly using reparenting when implementing this patch and it looks like it'll be pretty non-trivial. https://codereview.chromium.org/2319863005/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrWindowAndroid.java (right): https://codereview.chromium.org/2319863005/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrWindowAndroid.java:96: On 2016/09/20 19:57:07, Ted C wrote: > extra blank line Done. https://codereview.chromium.org/2319863005/diff/100001/chrome/browser/android... File chrome/browser/android/vr_shell/vr_compositor.cc (right): https://codereview.chromium.org/2319863005/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/vr_compositor.cc:29: void VrCompositor::SetLayer(content::ContentViewCore* core) { On 2016/09/20 20:29:44, mdjones wrote: > Will this ever be called more than once for this object? If so, the reparenting > logic should be somewhere other than the destructor. This will only ever be called once. I've added an assert to ensure this. https://codereview.chromium.org/2319863005/diff/100001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/2319863005/diff/100001/content/browser/androi... content/browser/android/content_view_core_impl.cc:280: FOR_EACH_OBSERVER(ContentViewCoreImplObserver, observer_list_, On 2016/09/20 19:57:07, Ted C wrote: > per my other comment, let's try to avoid this by setting it to null as part of > the VR path. Done.
lgtm - for the java side changes, please wait for mdjones@ to give it a final look as well
compositor lgtm
The CQ bit was checked by mthiesse@chromium.org
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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, mdjones@chromium.org Link to the patchset: https://codereview.chromium.org/2319863005/#ps140001 (title: "Rebase onto ToT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
vr_shell lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by mthiesse@chromium.org
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.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Implement new compositor and ContentViewCore reparenting for VR Shell. BUG=638642 ========== to ========== Implement new compositor and ContentViewCore reparenting for VR Shell. BUG=638642 Committed: https://crrev.com/5584eb0f2c325d87582fa1f1d5a57cca1522f80b Cr-Commit-Position: refs/heads/master@{#419927} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/5584eb0f2c325d87582fa1f1d5a57cca1522f80b Cr-Commit-Position: refs/heads/master@{#419927} |
