Request camera permission when AR features are requested. [chromium/src : master]
Iker Jamardo (Gerrit)
Iker Jamardo would like David Dorwin, Bill Orr, Klaus Weidner, Michael Thiessen, Ian Vollick, Yaron Friedman, Biao She and Max Rebuschatis to review this change.
Request camera permission when AR features are requested.
Adds a new XRSessionRequest structure to be passed when the
RequestSession call is made to be able to pass information like
user gesture. The RequestSession now requests camera permission
in ARCoreDevice. The code to ask for camera permission has been
copied and adapted from
src/chrome/browser/media/webrtc/media_stream_devices_controller.cc
and a re-check for the Android OS level camera permission is still
needed.
Bug: 837535, 835037
Add explicit requestSession call to the browser process for WebXR.
Improves the support for requestSession in WebXR mainly focused on
allowing ARCoreDevice to receive a call when the session request
is made.
Also improves the supportSession query with a new ar flag.
Bug: 837538
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_vr
Change-Id: Iecabc3927beb6106a1ada5289f3af3ecffd48ed8
---
M chrome/browser/android/vr/arcore_device/arcore_device.cc
M chrome/browser/android/vr/arcore_device/arcore_device.h
M chrome/browser/vr/service/vr_display_host.cc
M chrome/browser/vr/service/vr_display_host.h
M device/vr/public/mojom/vr_service.mojom
M device/vr/vr_device_base.cc
M device/vr/vr_device_base.h
M device/vr/vr_display_impl.cc
M device/vr/vr_display_impl.h
M third_party/blink/renderer/modules/xr/xr_device.cc
M third_party/blink/renderer/modules/xr/xr_device.h
11 files changed, 279 insertions(+), 30 deletions(-)
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
First push of the camera permission CL using code adapted from the gUM codebase. The OS level recheck needs to happen still, but this CL uses already chromium code to do it.
Iker Jamardo (Gerrit)
Iker Jamardo uploaded patch set #3 to this change.
Request camera permission when AR features are requested.
Adds a new XRSessionRequest structure to be passed when the
RequestSession call is made to be able to pass information like
user gesture. The RequestSession now requests camera permission
in ARCoreDevice. The code to ask for camera permission has been
copied and adapted from
src/chrome/browser/media/webrtc/media_stream_devices_controller.cc
and a re-check for the Android OS level camera permission is still
needed.
Bug: 837535, 835037
---
M chrome/browser/android/vr/arcore_device/arcore_device.cc
M chrome/browser/android/vr/arcore_device/arcore_device.h
M chrome/browser/vr/service/vr_display_host.cc
M chrome/browser/vr/service/vr_display_host.h
M device/vr/public/mojom/vr_service.mojom
M device/vr/vr_device_base.cc
M device/vr/vr_device_base.h
M device/vr/vr_display_impl.cc
M device/vr/vr_display_impl.h
M third_party/blink/renderer/modules/xr/xr_device.cc
M third_party/blink/renderer/modules/xr/xr_device.h
11 files changed, 279 insertions(+), 30 deletions(-)
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
David Dorwin (Gerrit)
Thanks.
22 comments:
Patch Set #6, Line 18:
837535This CL doesn't really fix this issue.
File chrome/browser/android/vr/arcore_device/arcore_device.h:
Patch Set #6, Line 68:
_for_videIs this part necessary? The call is for camera/video.
Patch Set #6, Line 69:
mojom::VRDisplayHost::RequestSessionCallback callback,Is there a reason the callback isn't last?
Patch Set #6, Line 71:
void AndroidCameraPermissionPromptAnswered(nit:
Should this be On...?
Also, is "Answered" commonly used for these? Let's be consistent with others.
I see this is based on MediaStreamDevicesController, but it's not exactly the same, so we can probably change it.Patch Set #6, Line 72:
mojom::VRDisplayHost::RequestSessionCallback callback,ditto on order question
nit: was_
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
Patch Set #6, Line 123:
// TODO(https://crbug.com/837116): Hook up ARCoreDevice to ARCore libraryWe might want to add a comment that ARCore sessions always require the camera permission. (After this line.)
Patch Set #6, Line 124:
RequestCameraPermission(std::move(session_request),In the future, we may wan to add a RequestAppropriatePermission to the base class and always call that from VRDisplayHost. That would force individual devices to consider this. We might also break out AreRequestedOptionsSupported when we have them. We'll see how that goes, but it's something we should keep in mind.
Patch Set #6, Line 182:
videocamera?
I see this is copied from MediaStreamDevicesController::RequestPermissions(), but we can probably do something more appropriate for us here.Patch Set #6, Line 183:
->GetPermissionStatusForFrame(AFAICT, we have to make this call just so we know the value before requesting permission. The following comments are on the RequestPermission API.
This seems like an API problem if clients want to know whether there was a prompt (or what the previous value was) but have to do that manually like this. This is especially true since this requires the caller to presume to know under what conditions the PermissionManager "will_prompt". For example, if Chrome decided to reprompt for sites in some cases, the "will_prompt" here would be incorrect.
Since we are requesting this information, we could just bail if it is false, though we'd still need to handle the Android permission.
If we always want to call RequestPermission, then that method should provide did_prompt in the callback. I guess that would require that it provide a vector of responses to match the ability to request multiple permissions.Patch Set #6, Line 198:
_for_videoShould we just drop this part?
Patch Set #6, Line 201:
just cancel thenit: "abort the" or "There is no reason to"?
it is safe to assume that the Android OS level permission has
// also been approved.Why is that? Just because the previous value was ASK doesn't mean we have the Android permission.
Is the issue in 837535 that PermisisonManager only prompts for Android if it also needs to prompt for the origin? If so, then I understand. However, I think we should be more explicit about that in this comment and in the bug.nit: is?
"was" could imply an actual user decision this time.A
(because it was already granted)
Patch Set #6, Line 216:
be neededneed
File chrome/browser/vr/service/vr_display_host.cc:
Patch Set #6, Line 50:
render_frame_hostIs there a reason we would want to create a device without a rfh? ISTM this object can't be used anyway and we'll run into problems later and with these values.
Can we fail earlier and DCHECK here or make this private and add a Create method that fails?
File device/vr/public/mojom/vr_service.mojom:
Patch Set #6, Line 60:
struct XRSessionRequest {Add description.
Patch Set #6, Line 61:
bool user_gesture;has_...?
Technically, it is a user_activation, though other code may use "gesture".
File device/vr/vr_display_impl.h:
Patch Set #6, Line 77:
int render_process_id_;const
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #6, Line 159:
user_gestureThe method we're calling is using "activation".
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
22 comments:
Patch Set #6, Line 18:
837535This CL doesn't really fix this issue.
I think it does because it proves that the bug is not solvable as the PermissionManager does not handle this correctly and the re-check is in fact needed. I don't know how a non solvable issue is specified but this CL makes the bug unsolvable (for now). I don't think we should be in a position to make changes in the PermissionManager.
File chrome/browser/android/vr/arcore_device/arcore_device.h:
Patch Set #6, Line 68:
yHost::Re
Is this part necessary? The call is for camera/video.
Done
Patch Set #6, Line 69:
ContentSetting content_setting);
Is there a reason the callback isn't last?
Because the method is a callback for the PermissionManager.RequestPermission call so the callback is just a pass through argument.
Patch Set #6, Line 71:
mojom::VRDisplayHost::RequestSessionCallback callback,nit: […]
AndroidOSPromptAnswered is used in MediaStreamDevicesController. Anyway, I will use OnAndroidCameraPermissionPromptResult instead.
Patch Set #6, Line 72:
bool was_android_camera_permission_granted);ditto on order question
ditto on answer.
nit: was_
Patch Set #6, Line 123:
// TODO(https://crbug.com/837116): Hook up ARCoreDevice to ARCore library
We might want to add a comment that ARCore sessions always require the camera permission. […]
Don't fully understand what you want, sorry.
Patch Set #6, Line 124:
RequestCameraPermission(std::move(session_request),
In the future, we may wan to add a RequestAppropriatePermission to the base class and always call th […]
Would you like me to file a bug for this and add a TODO in VRDisplayHost?
camera? […]
Done
Patch Set #6, Line 183:
permission_manager->RequestPermission(AFAICT, we have to make this call just so we know the value before requesting permission. […]
Got rid of it. Let me know if you like this way better.
Should we just drop this part?
I think is an interesting way of solving the problem how it has been resolved in getUserMedia. The other options I can think of are always requesting the permission and always checking if the OS level permission is needed even if the permission request callback states that is allowed. I thought that copying the way getUserMedia does it was the best option as that code seems to have been thoroughly tested and reviewed but happy to change it. Let me know if the way is what you think is best for our use case.
Patch Set #6, Line 201:
need to be requ
nit: "abort the" or "There is no reason to"?
Done
Why is that? Just because the previous value was ASK doesn't mean we have the Android permission. […]
Amazingly, the PermissionManager requestPermission call handles both the Chrome level permission and the OS level permission if the prompt happened but the OS level permission is only handled if the Chrome level permission is not granted and is not blocked. That is why the Android level camera permission needs to be re-requested in case. I am happy to change the behavior but this is how the UserMedia code is handling this. Should not be good enough for our use case too? I made some changes to make this easier to understand (I think).
nit: is? […]
Done
(because it was already granted)
Done
A
Done
Patch Set #6, Line 50:
render_frame_host
Is there a reason we would want to create a device without a rfh? ISTM this object can't be used any […]
Should this be a question for someone else in the Hoverboard team? They may know better why this is the way it is. Maybe file a bug?
File device/vr/public/mojom/vr_service.mojom:
Patch Set #6, Line 60:
// Data passed when requesting a session.Add description.
Done
Patch Set #6, Line 61:
struct XRSessionRequest {has_...? […]
MediaController called it user_gesture.
File device/vr/vr_display_impl.h:
Patch Set #6, Line 77:
const int render_process_id_;const
Done
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #6, Line 159:
has_user_act
The method we're calling is using "activation".
MediaController called it user_gesture. Shouldn't we use the same name?
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
David Dorwin (Gerrit)
LGTM with comments. Thank you.
Patch set 8:Code-Review +1
6 comments:
Patch Set #6, Line 18:
837535
I think it does because it proves that the bug is not solvable as the PermissionManager does not han […]
I think it's fine to reference this CL in the bug, but I wouldn't say this CL is related to the bug. Thus, I think we should drop this one.
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
Don't fully understand what you want, sorry.
I was just saying that we might note why we are calling this - because ARCore exposes data from the camera, we require the camera permission.
Would you like me to file a bug for this and add a TODO in VRDisplayHost?
I filed https://crbug.com/845792.
Patch Set #6, Line 198:
ack).Run(tI think is an interesting way of solving the problem how it has been resolved in getUserMedia. […]
Yes, the fact that it is tested and reviewed is a good point. We could try to find out why it does that. What you have in PS8 seems to be simpler and just potentially call PermissionUpdateInfoBarDelegate::ShouldShowPermissionInfobar() when we don't need to. It's not clear if that is a problem.
It may also be that gUM needs to know which Android permission to ask for - something we do not. That could have been solved by passing slightly different bools and not exiting early.
File chrome/browser/vr/service/vr_display_host.cc:
Patch Set #6, Line 50:
render_frame_host
Should this be a question for someone else in the Hoverboard team? They may know better why this is […]
Filing a bug sounds good. This line already exists.
File device/vr/public/mojom/vr_service.mojom:
nit: "has been"?
I mainly want to avoid implying it was within a user activation, which is not required.
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Michael Thiessen (Gerrit)
1 comment:
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
Patch Set #8, Line 196:
PermissionUpdateInfoBarDelegate::ShouldShowPermissionInfobarI don't think this is quite correct, ShouldShowPermissionInfobar can return false even when you don't have permission, if the WebContents wasn't connected to a window or something.
I think you should change PermissionUpdateInfoBarDelegate::ShouldShowPermissionInfobar to return an enum, with values something like SHOW_PERMISSION_INFOBAR, PERMISSION_INFOBAR_NOT_NEEDED, and CANNOT_SHOW_PERMISSION_INFOBAR.
Then we can only return true when we get PERMISSION_INFOBAR_NOT_NECESSARY.
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Michael Thiessen (Gerrit)
As discussed offline, feel free to land without addressing my comment, but if you do, file a bug to make sure we don't interpret not being able to show a permission infobar as a signal that the permission has been granted.
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Michael Thiessen (Gerrit)
1 comment:
Patch Set #6, Line 50:
render_frame_host
Filing a bug sounds good. This line already exists.
This is purely for testing. It's not possible to mock out a RenderFrameHost, so we just allow a null one for testing.
No need for a bug, unless the bug is to refactor this somehow to test differently or something.
I wouldn't bother TBH.
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
David Dorwin (Gerrit)
1 comment:
Patch Set #6, Line 50:
render_frame_host
This is purely for testing. […]
That sounds sketchy - we have code that only exists to handle testing *and* don't document that. We should either pass parameters or inject an object (as you say, that might not be an option here). Passing parameters makes more sense since we don't actually need the object. I think this is something we should clean up and thus should have a bug to at least review.
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
Iker Jamardo uploaded patch set #10 to this change.
Request camera permission when AR features are requested.
Adds a new XRSessionRequest structure to be passed when the
RequestSession call is made to be able to pass information like
user gesture. The RequestSession now requests camera permission
in ARCoreDevice. The code to ask for camera permission has been
copied and adapted from
src/chrome/browser/media/webrtc/media_stream_devices_controller.cc
and a re-check for the Android OS level camera permission is still
needed.
Bug: 835037
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_vr
Change-Id: Iecabc3927beb6106a1ada5289f3af3ecffd48ed8
---
M chrome/browser/android/vr/arcore_device/arcore_device.cc
M chrome/browser/android/vr/arcore_device/arcore_device.h
M chrome/browser/vr/service/vr_display_host.cc
M chrome/browser/vr/service/vr_display_host.h
M device/vr/public/mojom/vr_service.mojom
M device/vr/vr_device_base.cc
M device/vr/vr_device_base.h
M device/vr/vr_display_impl.cc
M device/vr/vr_display_impl.h
M third_party/blink/renderer/modules/xr/xr_device.cc
10 files changed, 141 insertions(+), 16 deletions(-)
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
Iker Jamardo uploaded patch set #11 to this change.
Request camera permission when AR features are requested.
Adds a new XRSessionRequest structure to be passed when the
RequestSession call is made to be able to pass information like
user gesture. The RequestSession now requests camera permission
in ARCoreDevice. The code to ask for camera permission has been
copied and adapted from
src/chrome/browser/media/webrtc/media_stream_devices_controller.cc
and a re-check for the Android OS level camera permission is still
needed.
Bug: 835037, 845160
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_vr
Change-Id: Iecabc3927beb6106a1ada5289f3af3ecffd48ed8
---
M chrome/browser/android/vr/arcore_device/arcore_device.cc
M chrome/browser/android/vr/arcore_device/arcore_device.h
M chrome/browser/vr/service/vr_display_host.cc
M chrome/browser/vr/service/vr_display_host.h
M device/vr/public/mojom/vr_service.mojom
M device/vr/vr_device_base.cc
M device/vr/vr_device_base.h
M device/vr/vr_display_impl.cc
M device/vr/vr_display_impl.h
M third_party/blink/renderer/modules/xr/xr_device.cc
10 files changed, 141 insertions(+), 16 deletions(-)
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
7 comments:
Patch Set #6, Line 18:
835037I think it's fine to reference this CL in the bug, but I wouldn't say this CL is related to the bug. […]
Done
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
Patch Set #6, Line 123:
// be delayed.I was just saying that we might note why we are calling this - because ARCore exposes data from the […]
Done
Patch Set #6, Line 124:
if (!is_arcore_gl_thread_initialized_) {I filed https://crbug.com/845792.
Done
Yes, the fact that it is tested and reviewed is a good point. […]
I think the current approach is correct as long as the concerns raised by Michael are addressed.
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
Patch Set #8, Line 196:
ontent::RenderFrameHost::FromID(render_process_id, render_frI don't think this is quite correct, ShouldShowPermissionInfobar can return false even when you don' […]
Done
File chrome/browser/vr/service/vr_display_host.cc:
Patch Set #6, Line 50:
render_frame_host
That sounds sketchy - we have code that only exists to handle testing *and* don't document that. […]
Added https://bugs.chromium.org/p/chromium/issues/detail?id=846392
File device/vr/public/mojom/vr_service.mojom:
nit: "has been"? […]
Done
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Michael Thiessen (Gerrit)
1 comment:
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
Patch Set #12, Line 243:
CreateThis is going to fail a DCHECK if you ignore the result CANNOT_SHOW_PERMISSION_INFOBAR.
You should probably run the callback with false if you get CANNOT_SHOW_PERMISSION_INFOBAR.
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
1 comment:
Patch Set #12, Line 243:
CreateThis is going to fail a DCHECK if you ignore the result CANNOT_SHOW_PERMISSION_INFOBAR. […]
You are right. Thanks Michael!
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
Fixes Michael's comment on a possible condition that was not taken into account. Improves consistency in a name of InfoBar.
1 comment:
You are right. […]
Done
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
David Dorwin (Gerrit)
It's not clear why initialization was changed after PS8.
Patch set 14:-Code-Review
13 comments:
File chrome/browser/android/vr/arcore_device/arcore.h:
Patch Set #14, Line 25:
virtual bool IsInitialized() = 0;We don't need this if we handle initialization all at once.
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
Patch Set #14, Line 121:
// TODO(https://crbug.com/846521): If the RequestSession call comes beforeHow likely is this to happen? It seems like it might be if you chain together natvigator.xr.requestDevice().then({ requestSession(); }).
Patch Set #14, Line 129:
// TODO(https://crbug.com/837116): Hook up ARCoreDevice to ARCore library.Is this issue not fixed?
Patch Set #14, Line 141:
RequestSessionnit: This part of the name seems unnecessary.
Patch Set #14, Line 143:
bool success) {nit: success usually comes first.
Patch Set #14, Line 151:
// TODO(https://crbug.com/838954): Check if ARCore is installed and is thenit: empty line above.
Patch Set #14, Line 238:
show_permission_info_bar_stateIt'd probably be better to switch on this to ensure all cases are handled.
File chrome/browser/android/vr/arcore_device/arcore_gl.h:
Patch Set #14, Line 50:
void RequestSession(mojom::VRDisplayHost::RequestSessionCallback callback);This class doesn't know about sessions, and the implementation is one-time initialization (see comments there).
File chrome/browser/android/vr/arcore_device/arcore_gl.cc:
Patch Set #14, Line 133:
void ARCoreGl::RequestSession(This doesn't actually do anything related to requesting a session. It is just one-time initialization. We should name it to reflect that.
Patch Set #14, Line 140:
if (arcore_->IsInitialized()) {Why are we using arcore_ to track whether this function has been run?
nit: empty line after.
Patch Set #14, Line 144:
if (!arcore_->Initialize()) {Why did you separate this from the main initialize method? If you need to block on the permission, why not just defer the entire thing? We wouldn't want to start up a thread before a session is requested anyway.
We really shouldn't even be creating ARCoreDevice unless a session has been requested, but we'll need to do some refactoring for that. However, we could delay real initialization.
I think it'd be better to do initialization all at once and have ARCoreDevice initiate that from requestSession().
Patch Set #14, Line 149:
// Set the texture on ARCore to render the camera.nit: empty line.
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
David Dorwin (Gerrit)
1 comment:
File chrome/browser/media/webrtc/media_stream_devices_controller.cc:
Patch Set #14, Line 213:
// TODO(raymes): We can get here for 2 reasons: (1) android permission hasCan we address this now? If so, we should.
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
Iker Jamardo uploaded patch set #16 to this change.
Request camera permission when AR features are requested.
Adds a new XRSessionRequest structure to be passed when the
RequestSession call is made to be able to pass information like
user gesture. The RequestSession now requests camera permission
in ARCoreDevice. The code to ask for camera permission has been
copied and adapted from
src/chrome/browser/media/webrtc/media_stream_devices_controller.cc
and a re-check for the Android OS level camera permission is still
needed.
Bug: 835037, 845160, 775372
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_vr
Change-Id: Iecabc3927beb6106a1ada5289f3af3ecffd48ed8
---
M chrome/browser/android/vr/arcore_device/arcore.h
M chrome/browser/android/vr/arcore_device/arcore_device.cc
M chrome/browser/android/vr/arcore_device/arcore_device.h
M chrome/browser/android/vr/arcore_device/arcore_gl.cc
M chrome/browser/android/vr/arcore_device/arcore_gl.h
M chrome/browser/android/vr/arcore_device/arcore_impl.cc
M chrome/browser/android/vr/arcore_device/arcore_impl.h
M chrome/browser/android/vr/arcore_device/fake_arcore.cc
M chrome/browser/android/vr/arcore_device/fake_arcore.h
M chrome/browser/geolocation/geolocation_permission_context_android.cc
M chrome/browser/media/webrtc/media_stream_devices_controller.cc
M chrome/browser/permissions/permission_update_infobar_delegate_android.cc
M chrome/browser/permissions/permission_update_infobar_delegate_android.h
M chrome/browser/vr/service/vr_display_host.cc
M chrome/browser/vr/service/vr_display_host.h
M device/vr/public/mojom/vr_service.mojom
M device/vr/vr_device.h
M device/vr/vr_device_base.cc
M device/vr/vr_device_base.h
M device/vr/vr_display_impl.cc
M device/vr/vr_display_impl.h
M third_party/blink/renderer/modules/xr/xr_device.cc
22 files changed, 249 insertions(+), 41 deletions(-)
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Raymes Khoury (Gerrit)
lgtm with nits
Patch set 16:Code-Review +1
3 comments:
File chrome/browser/media/webrtc/media_stream_devices_controller.cc:
Patch Set #16, Line 216:
// when the Android permission isn't present. crbug.com/775372.This TODO/bug can be addressed now. If the value is CANNOT_... we can replace the values in |responses| with CONTENT_SETTING_BLOCK. Doesn't have to be addressed in this CL but it would be nice to do in a followup if you're able.
File chrome/browser/permissions/permission_update_infobar_delegate_android.h:
Patch Set #16, Line 25:
CANNOT_SHOW_PERMISSION_INFOBARCould you add comments explaining what each of these mean?
e.g. the first value implies that there's no need because permissions are always granted. The last means that the infobar can't be shown because the WebContents or WindowAndroid has been destroyed).Patch Set #16, Line 64:
// site by the user.nit: please update the comment
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
17 comments:
Patch Set #14, Line 25:
virtual bool IsInitialized() = 0;We don't need this if we handle initialization all at once.
Replied in other comments. If we handle the session initialization of ARCore in the ARCoreDevice class then I agree that we do not need it.
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
Patch Set #14, Line 121:
// TODO(https://crbug.com/846521): If the RequestSession call comes before
How likely is this to happen? It seems like it might be if you chain together natvigator.xr. […]
Should not be like https://crbug.com/837944 ?
Patch Set #14, Line 129:
// TODO(https://crbug.com/845792): Consider calling a method to ask for the
Is this issue not fixed?
Done
nit: This part of the name seems unnecessary.
I added it to differentiate from the OnRequestCameraPermissionResult. Too many method with similar names but not easy solution.
Patch Set #14, Line 143:
DCHECK(is_arcore_gl_thread_initialized_);
nit: success usually comes first.
How do you change the order? Check the BindOnce above that connected to this method.
Patch Set #14, Line 151:
// correct version.nit: empty line above.
Done
Patch Set #14, Line 238:
se NO_NEED_TO_SHOW_PERMISSION_
It'd probably be better to switch on this to ensure all cases are handled.
Patch Set #14, Line 50:
void RequestSession(mojom::VRDisplayHost::RequestSessionCallback callback);
This class doesn't know about sessions, and the implementation is one-time initialization (see comme […]
Replied to a different comment in the cc file about this.
File chrome/browser/android/vr/arcore_device/arcore_gl.cc:
Patch Set #14, Line 133:
void ARCoreGl::RequestSession(
This doesn't actually do anything related to requesting a session. […]
That is not true. We could change the name of the ARCore::Initialize method to InitializeSession or even RequestSession but that is what it is actually doing. The problem is that the initialization of the ARCoreGL thread is not just that but also the transport bridge. As you mention, all this will change with the refactor but I do not think the current approach is incorrect either.
Patch Set #14, Line 140:
if (arcore_->IsInitialized()) {Why are we using arcore_ to track whether this function has been run?
Are you referring that you would actually prefer to handle that in the ARCoreDevice side?
nit: empty line after.
Done
Why did you separate this from the main initialize method? If you need to block on the permission, w […]
Replied in an earlier review comment.
nit: empty line.
Done
File chrome/browser/media/webrtc/media_stream_devices_controller.cc:
Patch Set #14, Line 213:
case SHOW_PERMISSION_INFOBAR:
Can we address this now? If so, we should.
Done
File chrome/browser/media/webrtc/media_stream_devices_controller.cc:
Patch Set #16, Line 216:
base::BindRepeating(This TODO/bug can be addressed now. If the value is CANNOT_... […]
Resolved in this CL. Could not change the responses container as it is const so found 2 alternatives:
1) Create a new container with the same amount of values as in the original responses container but filled with CONTENT_SETTING_BLOCK values.
2) Run the callback forcing the state to be blocked:
video_setting_ = CONTENT_SETTING_BLOCK;
audio_setting_ = CONTENT_SETTING_BLOCK;
denial_reason_ = content::MEDIA_DEVICE_PERMISSION_DENIED;
RunCallback(false);Implemented 1) but let me know what you think.
File chrome/browser/permissions/permission_update_infobar_delegate_android.h:
Patch Set #16, Line 25:
// No need to show the infobar as the permissions have been already granted.Could you add comments explaining what each of these mean? […]
Done
Patch Set #16, Line 64:
int permission_msg_id,
nit: please update the comment
Done
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
David Dorwin (Gerrit)
The reason for the two-phase initialization still isn't clear. Maybe we should discuss offline.
16 comments:
Patch Set #14, Line 121:
// TODO(https://crbug.com/846521): If the RequestSession call comes before
Should not be like https://crbug. […]
I was just wondering if this is very likely and thus something that needs to be addressed very soon.
I added it to differentiate from the OnRequestCameraPermissionResult. […]
Ack. Hmm, yeah.
Patch Set #14, Line 143:
DCHECK(is_arcore_gl_thread_initialized_);
How do you change the order? Check the BindOnce above that connected to this method.
Oh, nevermind then.
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
Patch Set #17, Line 135:
CameraPermissionRequestRequestCameraPermission
It's odd that the words are switched.
I understand the similarity between Result/Complete, but I don't think this helps.Patch Set #17, Line 240:
break;return is probably correct. You wouldn't want to run code after the switch statement if some was added. Ditto for all.
Patch Set #17, Line 252:
default:default defeats the purpose of compiler checks (not enabled on all platforms) that all all cases are handled.
Instead, return in each case then put the following line after the switch statement.
Patch Set #17, Line 253:
CHECK(false)This will crash the browser process, which is not something we want to do.
Also, if you find yourself doing "assert(false)", there is probably a better macro. In this case, NOTREACHED().
File chrome/browser/android/vr/arcore_device/arcore_gl.cc:
Patch Set #14, Line 133:
void ARCoreGl::RequestSession(
That is not true. […]
AFAIU, ARCore and ARCoreGl do not know anything about XRSessions. ARCoreImpl has an ArSession, but I believe that will be shared by all XRSessions and frames (including tabs). Thus, it's not clear that any method in these two classes should include the word "Session" (unless we plan to change the behavior I described).
I guess you could say that initialization is requesting the ArSession, but that seems more like an implementation detail and more likely to lead to confusion since the two "sessions" are not really related.
Patch Set #14, Line 140:
if (arcore_->IsInitialized()) {
Are you referring that you would actually prefer to handle that in the ARCoreDevice side?
I'm saying that we are tracking whether to run this method inside another class. arcore_ is initialized iff we reach line 150 below, so we can track that entirely here without needing to burden the other class and implementations with this.
Replied in an earlier review comment.
Which comment? Line 133? I don't understand how that applies here.
File chrome/browser/media/webrtc/media_stream_devices_controller.cc:
Patch Set #17, Line 212:
break;Ditto on return, though this one is slightly less obvious
Patch Set #17, Line 228:
CHECK(false) << "Unknown show permission infobar state.";Ditto on default and NOTREACHED.
File chrome/browser/permissions/permission_update_infobar_delegate_android.h:
Patch Set #17, Line 27:
SHOW_PERMISSION_INFOBAR,nit: There are now comments for all but this one. We should probably add a comment, especially since the comment on line 25 could appear to apply to both.
Patch Set #17, Line 67:
// Return an indicator of whether a permission infobar should be shown orWe can simplify this:
Returns an indicator of whether to show a permission infobar or that an infobar cannot be shown.Patch Set #17, Line 68:
indicator may be to shownit: This is a bit hard to parse.
Patch Set #17, Line 68:
infobaseinfobar
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Raymes Khoury (Gerrit)
1 comment:
File chrome/browser/media/webrtc/media_stream_devices_controller.cc:
Patch Set #17, Line 221:
std::vector<ContentSetting> blocked_responses(responses.size());nit: rather than using std::fill you should just be able to pass a value to initialize to into the std::vector constructor, e.g. std::vector<ContentSetting> blocked_responses(responses.size(), CONTENT_SETTING_BLOCK);
(my argument order my be backward but you get the idea)
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
4 comments:
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
Patch Set #17, Line 196:
// The RFH may have been destroyed by the time the request is processed.This may be better documented somewhere else, but it's a bit difficult to understand how the lifetime of VRDisplayImpl and ARCoreDevice are related: when a VRDisplayImpl is destroyed (i.e. by the RFH going away), how much longer can the corresponding ARCoreDevice live?
File chrome/browser/android/vr/arcore_device/arcore_gl.cc:
Patch Set #17, Line 138:
// There can only by one ARCore session at the moment, so if ARCore hasFWIW, it's not really clear to me how this is guaranteed.
File chrome/browser/permissions/permission_update_infobar_delegate_android.h:
Patch Set #17, Line 24:
enum ShowPermissionInfoBarState {I would suggest using scoped enums (i.e. enum class) for all new enums.
File chrome/browser/vr/service/vr_display_host.cc:
Patch Set #17, Line 55:
render_frame_host ? render_frame_host->GetProcess()->GetID() : -1,I'm a bit curious: why do we allow this to be called when there's no RFH?
(In fact, can this actually happen? I read VRServiceImpl, which seems to be a WebContentsObserver that self-deletes once RFH is nulled out. And VRServiceImpl seems to own the various VRDisplayHosts.)
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
19 comments:
File chrome/browser/android/vr/arcore_device/arcore.h:
Patch Set #14, Line 25:
virtual void SetDisplayGeometry(Replied in other comments. […]
Channged to ARCoreGL::InitializeARCoreSession and ARCore::InitializeSession and the state flag is managed by ARCoreGL.
Doing all the initialization when the RequestSession comes implies some changes plus still the flag to identifify if the unique ARCore session has been initialized is required.
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
Patch Set #14, Line 121:
// TODO(https://crbug.com/846521): If the RequestSession call comes before
I was just wondering if this is very likely and thus something that needs to be addressed very soon.
As likely as the bug referenced in my previous comment I guess.
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
Patch Set #17, Line 135:
RequestCameraPermissionRequestCameraPermission […]
Done
Patch Set #17, Line 196:
void ARCoreDevice::RequestCameraPermission(This may be better documented somewhere else, but it's a bit difficult to understand how the lifetim […]
There is a major refactor coming in a registered bug 828321. As for now, the device instance is created as soon as que XR API is referenced in the JS code.
Patch Set #17, Line 240:
// the Android camera permission might still need to be requested, so checkreturn is probably correct. […]
Done
Patch Set #17, Line 252:
// Show the Android camera permission info bar.default defeats the purpose of compiler checks (not enabled on all platforms) that all all cases are […]
Done
Patch Set #17, Line 253:
PermissionUpThis will crash the browser process, which is not something we want to do. […]
Done
File chrome/browser/android/vr/arcore_device/arcore_gl.h:
Replied to a different comment in the cc file about this.
Channged to ARCoreGL::InitializeARCoreSession and ARCore::InitializeSession and the state flag is managed by ARCoreGL.
File chrome/browser/android/vr/arcore_device/arcore_gl.cc:
AFAIU, ARCore and ARCoreGl do not know anything about XRSessions. […]
Channged to ARCoreGL::InitializeARCoreSession and ARCore::InitializeSession and the state flag is managed by ARCoreGL.
Patch Set #14, Line 140:
return true;I'm saying that we are tracking whether to run this method inside another class. […]
Channged to ARCoreGL::InitializeARCoreSession and ARCore::InitializeSession and the state flag is managed by ARCoreGL.
Patch Set #14, Line 144:
mojom::VRDisplayHost::RequestSessionCallback callback) {
Which comment? Line 133? I don't understand how that applies here.
In order to initialize ARCore permissionns need to be granted. The ARCore hook up CL initialized ARCore as soon as the GL thread was created. Thus, the split inn this CL.
File chrome/browser/android/vr/arcore_device/arcore_gl.cc:
FWIW, it's not really clear to me how this is guaranteed.
The check on the flag to see if it is initialized should ensure it, correct?
File chrome/browser/media/webrtc/media_stream_devices_controller.cc:
Patch Set #17, Line 212:
return;
Ditto on return, though this one is slightly less obvious
Done
Patch Set #17, Line 221:
std::vector<ContentSetting> blocked_responses(responses.size(),nit: rather than using std::fill you should just be able to pass a value to initialize to into the s […]
Done
Patch Set #17, Line 228:
#else
Ditto on default and NOTREACHED.
Done
File chrome/browser/permissions/permission_update_infobar_delegate_android.h:
Patch Set #17, Line 27:
// Show the the permission infobar.nit: There are now comments for all but this one. […]
Done
We can simplify this: […]
Done
Patch Set #17, Line 68:
n indicator of whether a
nit: This is a bit hard to parse.
Done
Patch Set #17, Line 68:
uld be sinfobar
Done
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
David Dorwin (Gerrit)
7 comments:
File chrome/browser/android/vr/arcore_device/arcore.h:
Patch Set #19, Line 24:
SessionWhile ARCoreImpl does in fact initialize an ARSession, that isn't a concept in this interface, so it probably doesn't make sense to mention here. It's really just an implementation detail of one of the concrete classes, unless we plan to expose the session concept later.
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
Patch Set #17, Line 196:
void ARCoreDevice::RequestCameraPermission(There is a major refactor coming in a registered bug 828321. […]
Why is this statement true? It appears to be called synchronously on the main thread.
While we will be refactoring, I'm not sure it will resolve this. We may want to track it with a TODO or bug, though I'm not exactly sure what the desired outcome is.
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
Patch Set #19, Line 263:
NOTREACHED() << "Unknown show permission infobar state.";nit: empty line
File chrome/browser/android/vr/arcore_device/arcore_gl.h:
Patch Set #19, Line 51:
SessionIf we don't have this concept in the ARCore interface, we shouldn't have it here either.
File chrome/browser/android/vr/arcore_device/arcore_gl.cc:
Patch Set #19, Line 143:
void ARCoreGl::InitializeARCoreSession(I'm not sure this is necessary or that it buys us a whole lot. This is fine for now (see below), but we could probably also do without delaying ARCore initialization. If we're going to keep it, we should probably add a TODO(https://crbug.com/845792) that notes how this should change.
Before enabling, we'll want to delay the whole mailbox initialization until the first session is requested. We could do that now, though the chain might be complex.
We may eventually move the chain to VRDevice[Base] per https://crbug.com/845792, so the chain would be handled there. We may even handle one-time initialization there - or we could potentially delay creating the entire class until that point.
Patch Set #19, Line 148:
at the momentDoes this mean in the current implementation? If so, you might clarify this by saying "Currently, ..." as this could be interpreted as "at a time." Also, AFAIU, we don't plan to change this, so you might remove this altogether.
File chrome/browser/media/webrtc/media_stream_devices_controller.cc:
Patch Set #19, Line 227:
NOTREACHED() << "Unknown show permission infobar state.";ditto
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
13 comments:
File chrome/browser/android/vr/arcore_device/arcore.h:
Patch Set #19, Line 24:
ime andWhile ARCoreImpl does in fact initialize an ARSession, that isn't a concept in this interface, so it […]
There is just Initialize now.
File chrome/browser/android/vr/arcore_device/arcore.h:
Patch Set #14, Line 25:
// If successful, the runtime must be paused when this method returns.Channged to ARCoreGL::InitializeARCoreSession and ARCore::InitializeSession and the state flag is ma […]
Put back to a single Initialize method.
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
nit: empty line
Done
File chrome/browser/android/vr/arcore_device/arcore_gl.h:
Patch Set #19, Line 51:
SessionIf we don't have this concept in the ARCore interface, we shouldn't have it here either.
Channged to ARCoreGL::InitializeARCoreSession and ARCore::InitializeSession and the state flag is ma […]
Put back to a single Initialize call.
File chrome/browser/android/vr/arcore_device/arcore_gl.cc:
Channged to ARCoreGL::InitializeARCoreSession and ARCore::InitializeSession and the state flag is ma […]
ARCoreGL and ARCore's Initialize now initializes everything. The GL thread does not initialize anything at all apart from the thread itself. The first RequestSession handles que ARCore initialization.
Patch Set #14, Line 140:
return true;
Channged to ARCoreGL::InitializeARCoreSession and ARCore::InitializeSession and the state flag is ma […]
Put back to a single Initialize call.
Patch Set #14, Line 144:
mojom::VRDisplayHost::RequestSessionCallback callback) {
In order to initialize ARCore permissionns need to be granted. […]
Put back to be one Initialize method.
File chrome/browser/android/vr/arcore_device/arcore_gl.cc:
Patch Set #19, Line 143:
void ARCoreGl::InitializeARCoreSession(
I'm not sure this is necessary or that it buys us a whole lot. […]
Changed it to be all one initialization.
Patch Set #19, Line 148:
at the moment
Does this mean in the current implementation? If so, you might clarify this by saying "Currently, .. […]
Yes and no. The main refactor should include the concept of a session in the native side too. Although all the native session might share the same ARCore session underneath, they should include some differences. For example, as we are seeing with pause/resume, they should handle their pause and resume states independently and they need to govern ARCore underneath. But more than that, most likely they may need to handle poses and anchors differently (each session should have its own origin and set of anchors, planes). Of course, internally, they may use the same session, but have the correct algorithms to treat each session independently: use the inverse of the pose matrix at the moment of creation, have their own near far values for projection matrix creation, have their own anchor and plane lists, etc.
Anyway, in this case, I have modified the comment.
File chrome/browser/media/webrtc/media_stream_devices_controller.cc:
Patch Set #19, Line 227:
NOTREACHED() << "Unknown show permission infobar state.";ditto
Done
File chrome/browser/permissions/permission_update_infobar_delegate_android.h:
Patch Set #17, Line 24:
enum class ShowPermissionInfoBarState {
I would suggest using scoped enums (i.e. enum class) for all new enums.
Done
File chrome/browser/vr/service/vr_display_host.cc:
Patch Set #17, Line 55:
device->GetDevice(), std::move(service_client), std::move(display_info),I'm a bit curious: why do we allow this to be called when there's no RFH? […]
Added a comment with a link to a bug. Basically, the RFH can be null because it was needed for enabling a test case. A previous CL that just landed addressed this same concern.
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
David Dorwin (Gerrit)
I think we've added complexity that moves us further from what we want long term. Let's discuss.
18 comments:
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
We've moved the resuming of ARCore from the initialization path to RequestSession. Long term, that is not what we want, and it makes the logic around pausing a bit harder to reason about (as reflected in my comments below).
The following code seems to be fixing a bug unrelated to requesting permission. It should be in a separate CL.
Or is this related to the change in ARCoreGl::Pause()? If so, then I'm concerned about this and we may just need to wait for the refactoring.Patch Set #21, Line 147:
// As ARCoreDevice is effectively a single instance for multiple tabs. ThereEither this is an incomplete sentence or "As" should be removed.
Patch Set #21, Line 148:
the initial focus event is still not wired"Initial" - when? On tab switch or ARCoreDevice creation?
Is that really the problem or would switching tabs also be a problem? Are you only solving for the starting of a session and not the continued use on tab switching?Patch Set #21, Line 151:
Force to resumeThis violates the purpose of the focus enforcement. While we require a gesture to request a session, that gesture could have happened at any time. Thus, a backgrounded tab could request a session much after it is backgrounded if it ever had a gesture.
Patch Set #21, Line 152:
// TODO(): RefactorNeeds a bug number. Also, if we landed this, it is much more than a refactor - we would need to not be working around focus. That may depend on a refactor.
Patch Set #21, Line 153:
is_paused_ = false;Don't you need to call ResumeTracking instead?
I guess RunRequestSessionCallback does that. Is the issue that we could again be paused before that runs? This should be explained in a comment.and what?
Patch Set #21, Line 156:
// callback should be in fact a call to the RunRequestSessionCallback.This doesn't really explain things. Am I supposed to know why RunRequestSessionCallback is relevant? Maybe its name should describe what it does rather than when it is called.
Patch Set #21, Line 157:
callback = base::BindOnce(&ARCoreDevice::RunRequestSessionCallback,Don't change an input parameter like this. Use a different name.
Patch Set #21, Line 160:
// TODO(https://crbug.com/846521): If the RequestSession call comes beforeCan we do this before all the new code above? We don't need to unpause (already unpaused) or call RunRequestSessionCallback if the thread is not initialized.
I think that would allow us to at least move the reference to RunRequestSessionCallback to the permission callback as I've commented elsewhere. Ideally, we'd move the unpause there too, though there may be a race condition until we can remove that in the refactor. I think that might be fine/preferable as long as it's clearly documented and tracked.
Patch Set #21, Line 189:
// TODO(https://crbug.com/838954): Check if ARCore is installed and is theWhy is this TODO here? Shouldn't that be ARCoreImpl's responsibility?
I wonder whether this check should before or after the permission check. For Android permissions, we ask for Chrome's permission first, which makes sense. But if the user doesn't have ARCore, giving the site permission doesn't make sense.
Patch Set #21, Line 191:
PostTaskToGlThread(base::BindOnce(I think it would be better if we wrapped this and some other stuff (including the the call back to RunRequestSessionCallback) up in an FinishInitializationIfNecessary method.
Maybe we can move the is_initialized_ check from ARCoreGl::Initialize() here too since this is the class that's introducing that problem. Ditto for Pause/ResumeTracking, which already check is_arcore_gl_thread_initialized_. They could check a new is_arcore_initialized_ instead.
Patch Set #21, Line 313:
void ARCoreDevice::RunRequestSessionCallback(This should also be moved to the other CL I believe.
File chrome/browser/android/vr/arcore_device/arcore_gl.cc:
Yes and no. The main refactor should include the concept of a session in the native side too. […]
Hmm, I wasn't aware of all that. Is it documented anywhere? We should probably record it in a bug to make sure it is not lost.
File chrome/browser/android/vr/arcore_device/arcore_gl.cc:
Patch Set #21, Line 115:
std::move(callback).Run(false);The majority of this method could be simplified if it was in a helper (i.e., move 112-155 to private InitializeARCore) that was called from Initialize. Initialize would then run the callback on failure or continue on to initialize the class.
Patch Set #21, Line 284:
if (is_initialized_) {This adds complexity that we won't want long-term and forces some new understanding between this class and its caller that didn't exist before.
File chrome/browser/android/vr/arcore_device/arcore_gl_thread.cc:
Can we remove this parameter now? Or can it fail elsewhere?
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
22 comments:
File chrome/browser/android/vr/arcore_device/arcore.h:
Patch Set #14, Line 25:
// If successful, the runtime must be paused when this method returns.Put back to a single Initialize method.
Done
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
Patch Set #21, Line 139:
mojom::VRDisplayHost::RequestSessionCallback callback) {We've moved the resuming of ARCore from the initialization path to RequestSession. […]
I do not fully agree with this. ARCore's initialization is happening in the RequestSession so that is why the resume should happen then. I might be misunderstanding something from your comment.
Patch Set #21, Line 146:
std::move(callback).Run(false);The following code seems to be fixing a bug unrelated to requesting permission. […]
IMO the only code that is solving the bug is the is_paused_ = false in the RequestSession. Everything else is related to the ARCore initialization change that needs to happen after camera permission request. I agree that is_paused_ = false might not be the best solution but it was just a step while the major refactor comes to resolve the problem of having multiple tabs with multiple sessions.
Patch Set #21, Line 147:
return;
Either this is an incomplete sentence or "As" should be removed.
You are right. Sorry. Fixed.
"Initial" - when? On tab switch or ARCoreDevice creation? […]
Is only for new tabs. Once a tab is created and the wiring is done to the ARCoreDevice, the focus events are received correctly, so tab switching is correct. But when a new tab is created and still there is no reference to using WebXR, there is no way to receive the focus event. If there was a previous tab using WebXR, a pause event would have been fired and when the new RequestSession arrives, is_paused_ would be = true. I think I explained this in the comment above.
Patch Set #21, Line 151:
n(true);This violates the purpose of the focus enforcement. […]
I know this is not "correct" but happy to discuss alternatives. Unless we handle different sessions differently with different pause/resume states, I cannot see other solution (I might be missing it). Also, this kind of aligns with even the initial assumption we made that is_paused_ is false at the beginning so if you don't think is correct here, it won't be correct for the first ReauestSession either (without forcing it here). I hope I explained that correctly in a way that is understandable.
Patch Set #21, Line 152:
return;Needs a bug number. […]
Yes, just wanted to push a CL for review to see if this approach could even be possible.
Don't you need to call ResumeTracking instead? […]
No, because of 2 things:
a) ResumeTracking does not force is_paused_ to be false and that is what this is intending to do.
b) ARCore could not be initialized. yet so the resume would still need to happen afterwards. The whole codepath in this method will take care of it.I tried to explain my train of thought earlier in a reply to a review comment.
and what?
Sorry, I will try to be more thorough with comments from now on.
Patch Set #21, Line 156:
// is no concept of a session and the initial focus event is still not wiredThis doesn't really explain things. […]
Done
Patch Set #21, Line 157:
// to be received in the PauseTracking/ResumeTracking for a new tab, if one
Don't change an input parameter like this. Use a different name.
Done
Patch Set #21, Line 160:
// TODO(): RefactorCan we do this before all the new code above? We don't need to unpause (already unpaused) or call Ru […]
Agreed. Nice catch. Thanks.
Patch Set #21, Line 189:
CreateMainThreadCallback<bool>(Why is this TODO here? Shouldn't that be ARCoreImpl's responsibility? […]
ARCoreImpl is in the GL thread and the check for ARCore APK should happen in the main thread.
About the order, I agree that the ARCoreAPK check should be before. I will change it.
Patch Set #21, Line 191:
// resuming of ARCore if needed, the callback should be in fact aI think it would be better if we wrapped this and some other stuff (including the the call back to R […]
I played around with the idea of is_arcore_initialized flag (nice that we thought the same on this one) but what I did not like is that then all the internal checks in ARCoreGL need to disappear and leave the correct handling in the hands of ARCoreDevice. WDYT?
Patch Set #21, Line 313:
NOTREACHED() << "Unknown show permission infobar state.";
This should also be moved to the other CL I believe.
I do not agree. This is related to the change of having to Initialize ARCore in RequestSession that is also related to having to add camera permission checks before the ARCore initialization, thus, is relevant to this CL.
File chrome/browser/android/vr/arcore_device/arcore_gl.cc:
Patch Set #14, Line 133:
if (gl::GetGLImplementation() == gl::kGLImplementationNone &&ARCoreGL and ARCore's Initialize now initializes everything. […]
Done
Patch Set #14, Line 140:
gl::init::CreateOffscreenGLSurface(gfx::Size());
Put back to a single Initialize call.
Done
Put back to be one Initialize method.
Done
File chrome/browser/android/vr/arcore_device/arcore_gl.cc:
Hmm, I wasn't aware of all that. […]
Contacted the Anna Offenwanger so she is aware of all this in the refactor design and all of this is captured correctly.
File chrome/browser/android/vr/arcore_device/arcore_gl.cc:
Patch Set #21, Line 115:
if (!arcore_->Initialize()) {The majority of this method could be simplified if it was in a helper (i.e. […]
Done
This adds complexity that we won't want long-term and forces some new understanding between this cla […]
The comment is fair enough. Using a is_arcore_initialized_ member attribute in ARCoreDevice would solve that but then it would make this class not to check for incorrect calls when ARCore is not initialized (see all of the CHECK(is_initialized_)-s in all of the methods). Are we prepared for the trade off? I could see 3 possible solutions (maybe more):
1. This class keeps the flag and add it to ARCoreDevice too.
2. The IsInitialized is added to the ARCore interface.
3. We keep it as it is right now.
I will go for 1 for now.
File chrome/browser/android/vr/arcore_device/arcore_gl_thread.cc:
Can we remove this parameter now? Or can it fail elsewhere?
I agree that removing it makes sense.
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
David Dorwin (Gerrit)
I didn't get to ARCoreGl yet.
12 comments:
Patch Set #21, Line 139:
mojom::VRDisplayHost::RequestSessionCallback callback) {
I do not fully agree with this. […]
I think long-term we want something else to initialize this class on the first RequestSession. This class wouldn't need to know that or support the partially-initialized state.
Patch Set #21, Line 151:
n(true);
I know this is not "correct" but happy to discuss alternatives. […]
The initial value of is_paused_ is related to the initialization process. This can happen multiple times and well after initialization.
Patch Set #21, Line 191:
// resuming of ARCore if needed, the callback should be in fact a
I played around with the idea of is_arcore_initialized flag (nice that we thought the same on this o […]
Is this reply outdated? It looks like you did add is_arcore_gl_initialized_.
I think my goal was for proper use of ARCoreGL to not require any checks. The caller would be expected to not call it until initialization is complete rather than for it to try to handle that.
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
Patch Set #22, Line 95:
!is_arcore_gl_thread_initialized_ ||This is redundant, right?
Patch Set #22, Line 110:
!is_arcore_gl_thread_initialized_ ||ditto
Patch Set #22, Line 151:
std::move(callback).Run(true);These two lines prevent us from requesting the permission for a different origin. I think you need to move this to one of the callbacks.
initial focus event is still not wired
// to be received in the PauseTracking/ResumeTracking for a new tab
This seems like a tractable problem that is unrelated to refactoring. I'm curious, though, are we not receiving the event or is it not reaching PauseTracking/ResumeTracking or are those exiting early?
However, I don't think this is sufficient. We need to resume only when a tab with an active ARCore session is in focus. That responsibility probably belongs outside this class.
Perhaps VRDisplayImpl::RequestSession() should be wrapping the callback with its own that will call ResumeTracking if focused. (To avoid breaking VR in the interim, we could check ShouldPauseTrackingWhenFrameDataRestricted() like we do for focus today.) We might need to change the initial value of is_paused_ in this case (and maybe rename it to swap polarity).
Patch Set #22, Line 160:
// TODO(): RefactorTODO(<BUG>): Resume only when a tab with an active ARCore session is in focus, possibly outside this class.
Patch Set #22, Line 164:
// correct version.Does it make sense to add the following?
This must happen on the main thread.Patch Set #22, Line 187:
PostTaskToGlThread(base::BindOnce(I think it would be nice to just call something like FinishInitializationIfNecessary() here. That makes it clear what we're doing and that it has nothing to do with the permission - that we just chained them together. (That name doesn't work too well if we are going to do resume there, but I think we might want to avoid that anyway.)
Alternatively, that method could have been passed as the callback, wrapping the existing one, and this code would just call it.
In such a method, I think we should probably check is is_arcore_gl_initialized_. See also my comment below.
Patch Set #22, Line 188:
ARCoreGl::InitializeI think we only want to call this if !is_arcore_gl_initialized_. Otherwise, we should probably call RunRequestSessionCallbackAndResumeARCoreIfNotPaused directly. Also note that these may need to be different callbacks since we don't always need success.
Patch Set #22, Line 322:
is_arcore_gl_initialized_ = success;We really shouldn't be setting this on every session request. Nor should we be initializing ARCoreGL each time.
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
David Dorwin (Gerrit)
Finished PS22.
3 comments:
File chrome/browser/android/vr/arcore_device/arcore_gl.cc:
The comment is fair enough. […]
I think 1 is fine.
is_initialized_ was already only used for DCHECKs. Most importantly, we don't need to worry about multiple states or whether the pause/resume was deferred in this class, which is mostly a wrapper around ARCore, at least for pause/resume.
File chrome/browser/android/vr/arcore_device/arcore_gl.cc:
Patch Set #22, Line 105:
if (is_initialized_) {This should probably still be a DCHECK. As I noted in other comments, I don't think we should call this for every session request.
Patch Set #22, Line 121:
is_initialized_ = true;nit: Do this right before running the callback for consistency and defensively in case someone adds more steps to this method.
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
Rebasing to master solved the focus event handling on new tabs. Addressed the comments on the last review too.
22 comments:
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
Patch Set #14, Line 121:
// MailboxToSurfaceBridge's destructor's call to DestroyContext must
As likely as the bug referenced in my previous comment I guess.
Done
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
Patch Set #21, Line 139:
bool has_user_activation,I think long-term we want something else to initialize this class on the first RequestSession. […]
Agreed. I am going to file a big to provide more context about this to whoever is responsible to fix it in the future.
IMO the only code that is solving the bug is the is_paused_ = false in the RequestSession. […]
Fixed by the latest rebase.
Patch Set #21, Line 148:
ized, the resolution of the request shouldIs only for new tabs. […]
Resolved in the latest rebase.
Patch Set #21, Line 151:
n(false);
The initial value of is_paused_ is related to the initialization process. […]
This has been resolved in the latest rebase.
Patch Set #21, Line 152:
return;
Yes, just wanted to push a CL for review to see if this approach could even be possible.
Done
Patch Set #21, Line 152:
return;
Yes, just wanted to push a CL for review to see if this approach could even be possible.
Done
No, because of 2 things: […]
Resolved by the latest rebase.
Is this reply outdated? It looks like you did add is_arcore_gl_initialized_. […]
Yes, I think this comment was not up to date. How can you do what you mention in your last comment of not having to perform any checks? Make a GL thread call to both instantiate and initialize ARCodeGL?
Patch Set #21, Line 313:
GetWeakPtr(), std::move(callback)))));I do not agree. […]
Done
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
This is redundant, right?
Correct. Made the same mistake as in the past. Thanks for catching it.
ditto
Done
Patch Set #22, Line 151:
std::move(callback).Run(false);These two lines prevent us from requesting the permission for a different origin. […]
You are right. Thanks for catching it.
questSessionPreconditionsComplete,
GetWeakPtr(), std::move(callback));This seems like a tractable problem that is unrelated to refactoring. […]
Found why on a new tab, the ResumeTracking is not being fired. Resolved it in VRDisplayImpl.
On a new tab, when XR is referenced, a new VRDisplayImpl is instantiated and the focus state is passed in the constructor. So no "event" firing. But of course, the device does not know about it. In construction, a device has is_focused_frame = false and then the constructor changes it and when that happens, the device should also be notified.
Patch Set #22, Line 160:
// correct version. This must happen in the main thread.TODO(<BUG>): Resume only when a tab with an active ARCore session is in focus, possibly outside this […]
Resolved the problem (I think) in VRDisplayImpl.
Patch Set #22, Line 164:
// ARCore sessions require camera permission.Does it make sense to add the following? […]
Done
Patch Set #22, Line 187:
display::Display::Rotation display_rotation,I think it would be nice to just call something like FinishInitializationIfNecessary() here. […]
Added a function to handle the completion of the preconditions (camera permission and ARCore APK check in the future) and the final initialization of ARCore.
Patch Set #22, Line 188:
om::VRMagicWindowProI think we only want to call this if !is_arcore_gl_initialized_. […]
Replied to why I chose this path in the commend below. If multiple RequestSessions are made from the render process before the previous ones are finished, there would be a problem. The current codepath resolves this scenario correctly.
Patch Set #22, Line 322:
if (!success) {We really shouldn't be setting this on every session request. […]
We are not. ARCoreGL takes care of that. I followed this pattern because my paranoid mind thought that it could happen to receive more than one RequestSession from the render process before the previous ones have resolved the callback. Is that possible? If so, the current codepath resolves the situation gracefully.
File chrome/browser/android/vr/arcore_device/arcore_gl.cc:
Patch Set #21, Line 284:
// hit-test promise resolves immediately prior to the frame for which it isI think 1 is fine. […]
Done
File chrome/browser/android/vr/arcore_device/arcore_gl.cc:
Patch Set #22, Line 105:
// Do not DCHECK !is_initialized to allow multiple calls to correctly
This should probably still be a DCHECK. […]
As I mentioned in different comment on ARCoreDevice, if more than one RequestSession come from the renderer, this whole code path resolves all of the requests gracefully. That is why I did not add the CHECK.
Added some comments to explain this.
Patch Set #22, Line 121:
return;nit: Do this right before running the callback for consistency and defensively in case someone adds […]
Done
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
David Dorwin (Gerrit)
Looking good. Some minor stuff and comments about what we might want to do in the future.
11 comments:
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
questSessionPreconditionsComplete,
GetWeakPtr(), std::move(callback));Found why on a new tab, the ResumeTracking is not being fired. Resolved it in VRDisplayImpl. […]
FTR, this ended up being fixed by the rebase where such change had already been made.
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
Patch Set #23, Line 16:
#include "chrome/browser/permissions/permission_manager.h"This is a lot of dependencies and code unrelated to actual ARCore. We may want to consider refactoring this out as part of https://crbug.com/845792.
Patch Set #23, Line 309:
PostTaskToGlThread(base::BindOnce(Note: I think we could avoid this and just call the callback directly if is_arcore_gl_initialized_. As you've noted, it wouldn't eliminate all redundant calls if there are are multiple calls in succession. However, it makes a bit more sense and is a slight optimization. On the other hand, it adds another code branch.
Since we want to get away from this anyway (https://crbug.com/849568), this is probably fine for now.
Patch Set #23, Line 329:
success &&Not needed.
Patch Set #23, Line 330:
PostTaskToGlThread(base::BindOnce(Because we always execute this path even if ARCoreGl is already [in the process of being] initialized, we will call ARCoreGl::Resume every time a session is requested. This should be okay due to ordering of calls, but it is less than ideal.
File chrome/browser/android/vr/arcore_device/arcore_gl.cc:
Patch Set #23, Line 105:
// Do not DCHECK !is_initialized to allow multiple calls to correctlyI think we want to explain why this can happen:
// This method may be called multiple times if a subsequent session request occurs before the first one completes and the callback is called.
// TODO(https://crbug.com/849568): This may not be necessary after addressing this issue.
File chrome/browser/android/vr/arcore_device/arcore_impl.h:
Patch Set #23, Line 73:
bool Initialize() override;nit: Revert this file since there are no related changes.
File chrome/browser/vr/service/vr_display_host.h:
Patch Set #23, Line 43:
void RequestPresent(device::mojom::VRSubmitFrameClientPtr client,nit: Revert this file as there are no related changes.
File chrome/browser/vr/service/vr_display_host.cc:
Patch Set #23, Line 70:
bool has_user_activation = options->has_user_activation;nit: Do we need this local?
File device/vr/vr_display_impl.cc:
Is there a reason that we pass this object?
We only use it to call public methods to get the render_process_id_ and render_frame_id_ members. We could instead pass those in and eliminate the public methods.Furthermore, we only need those values to get a RenderFrameHost. Maybe we should instead pass that in. We obtained these values from a RenderFrameHost before passing them to this class's constructor. We'd need to double check the lifetime guarantees.
It's a bit weird to pass _any_ of these here, but that would be resolved by making the permission call explicit: https://crbug.com/845792.
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #23, Line 181:
Frame::HasTransientUserActivation(doc ? doc->GetFrame() : nullptr);It might be a bit better to make this a local bool then create the Mojo object below where it's needed (and where it is in the base revision).
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
the ipc side lgtm (and thanks for explaining the RFH issues and filing the bug).
I have a few more comments, but they are non-blocking: some are just suggestions to simplify the C++, while I also have a design question for WebXR with respect to workers/documents.
Patch set 23:Code-Review +1
3 comments:
File chrome/browser/android/vr/arcore_device/arcore_device.h:
Patch Set #23, Line 72:
base::OnceCallback<void(void)> callbackNit: just write base::OnceClosure here.
Alternatively, it's probably possible to write a variadic template here so we don't need both variants:
template <typename... Args>
static void RunCallbackOnTaskRunner(
// Note: const ref to avoid refcount churn, since this doesn't take ownership.
// T* using base::RetainedRef() if needed would also be acceptable.
const scoped_refptr<base::TaskRunner>& t task_runner,
base::OnceCallback<void(Args...) callback,
Args&&... args) {
task_runner->PostTask(FROM_HERE,
base::BindOnce(std::move(callback), std::forward<Args>(args)...);
}
File device/vr/vr_display_impl.cc:
Is there a reason that we pass this object? […]
I think the issue is that the device_ might be doing things asynchronously, so at some point, we're going to have to turn the RFH into a routing ID pair and use it as a "weak" pointer.
That being said, VRDisplayImpl itself should be able to hold RFH directly (it can't outlive the RFH, if I'm reading this code correctly)--though due to the unit test exception, we'd also have to make it handle the null case.
We should just fix the unit test ;-)
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #23, Line 181:
Frame::HasTransientUserActivation(doc ? doc->GetFrame() : nullptr);
Btw, I'm a bit surprised: how can we get here with no document? My understanding (from ddorwin) is that WebXR isn't exposed to workers right now.
(If it is exposed to workers, what is the plan for all the things currently gated on frames?)
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Bill Orr (Gerrit)
I think the issue is that the device_ might be doing things asynchronously, so at some point, we're […]
+1 to David's comment. We can pass the id's directly as needed.
We shouldn't have VRDisplayImpl hold the rfh, because VRDisplayImpl may not always be in the browser process moving forwards.
Longer term, we should request permissions in browser code before telling the device we want to create a session.
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #23, Line 181:
Frame::HasTransientUserActivation(doc ? doc->GetFrame() : nullptr);
Btw, I'm a bit surprised: how can we get here with no document? My understanding (from ddorwin) is t […]
detached navigator?
Patch Set #23, Line 181:
Frame::HasTransientUserActivation(doc ? doc->GetFrame() : nullptr);
It might be a bit better to make this a local bool then create the Mojo object below where it's need […]
also, we check above for exclusive. if we are always checking, may as well do it only once.
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
1 comment:
Patch Set #23, Line 181:
Frame::HasTransientUserActivation(doc ? doc->GetFrame() : nullptr);
detached navigator?
Does it even make sense to continue the request in that case? The browser side state will be (or already has been) torn down.
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
13 comments:
File chrome/browser/android/vr/arcore_device/arcore_device.h:
Patch Set #23, Line 72:
const scoped_refptr<base::TaskRunner>&Nit: just write base::OnceClosure here. […]
I tried different approaches to try to avoid having 2 additional methods but nothing works as the main issue is handling void as a parameter and how is handled by templates. But I am sure I might be missing something here. Tried your proposal (with some fixes) and does not seem to compile. Some help here would be much appreciated.
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
Patch Set #21, Line 191:
// TODO(ijamardo): Do we need to queue requests to avoid breakingYes, I think this comment was not up to date. […]
Marking this as resolved. Feel free to make more comments if needed.
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
Patch Set #23, Line 16:
#include "chrome/browser/permissions/permission_manager.h"
This is a lot of dependencies and code unrelated to actual ARCore. […]
What is the right way to address this? Add a comment with a bug reference in here?
Patch Set #23, Line 309:
OnARCoreGlInitializationComplete(std::move(callback), true);Note: I think we could avoid this and just call the callback directly if is_arcore_gl_initialized_. […]
I think is a good idea to avoid a round trip to the GL thread so I added the check but not calling the callback directly as it could be important to perform the resume of ARCore if it was paused.
Not needed.
Done
Because we always execute this path even if ARCoreGl is already [in the process of being] initialize […]
You are right but cannot think of a good solution for now.
File chrome/browser/android/vr/arcore_device/arcore_gl.cc:
Patch Set #17, Line 138:
if (gl::GetGLImplementation() == gl::kGLImplementationNone &&
The check on the flag to see if it is initialized should ensure it, correct?
Marking this as resolved, Please, feel free to add more comments if needed.
File chrome/browser/android/vr/arcore_device/arcore_gl.cc:
Patch Set #23, Line 105:
// Do not DCHECK !is_initialized to allow multiple calls to correctly
I think we want to explain why this can happen: […]
Done
File chrome/browser/android/vr/arcore_device/arcore_impl.h:
Patch Set #23, Line 73:
bool Initialize() override;nit: Revert this file since there are no related changes.
Removed an empty line so all the overridden methods are together.
File chrome/browser/vr/service/vr_display_host.h:
Patch Set #23, Line 43:
void RequestPresent(device::mojom::VRSubmitFrameClientPtr client,nit: Revert this file as there are no related changes.
Removed an empty line to have all the overridden methods together.
File chrome/browser/vr/service/vr_display_host.cc:
Patch Set #23, Line 70:
bool has_user_activation = options->has_user_activation;nit: Do we need this local?
Yes because the InternalSupportsSession takes the ownership of the options argument. If you can think of a different way to resolve this, happy to change it.
File device/vr/vr_display_impl.cc:
+1 to David's comment. We can pass the id's directly as needed. […]
Done
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #23, Line 181:
// TODO(https://crbug.com/828321): Use session options instead.
also, we check above for exclusive. if we are always checking, may as well do it only once.
Created a local variable and moved the instantiation of the XRSessionOptions instance to before making the call to the browser process.
I think the exclusive check merge to avoid having multiple conditionals should be resolved in a different CL.
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
David Dorwin (Gerrit)
LGTM with a few comments and resolution of the template suggestion.
Patch set 24:Code-Review +1
5 comments:
Patch Set #23, Line 16:
#include "chrome/browser/permissions/permission_manager.h"
What is the right way to address this? Add a comment with a bug reference in here?
I think updating the bug is sufficient. I did so.
File chrome/browser/vr/service/vr_display_host.cc:
Patch Set #23, Line 70:
bool has_user_activation = options->has_user_activation;
Yes because the InternalSupportsSession takes the ownership of the options argument. […]
Oh, I missed that. Thanks.
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #23, Line 181:
// TODO(https://crbug.com/828321): Use session options instead.
Does it even make sense to continue the request in that case? The browser side state will be (or alr […]
I agree that this is odd and we shouldn't continue.
This was the same as line 170, which was added by mustaq@ in https://crrev.com/c/737435/4/third_party/WebKit/Source/modules/vr/latest/VRDevice.cpp. I imagine that CL might have been a bit mechanical and thus defensive. We should reach out separately and determine if we can simplify this. For now, you might add a TODO(ijamardo) to determine whether we should just exit.
Patch Set #23, Line 181:
// TODO(https://crbug.com/828321): Use session options instead.
Created a local variable and moved the instantiation of the XRSessionOptions instance to before maki […]
I think it is reasonable to make this minor improvement in this CL. See the comment with specifics in PS24.
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #24, Line 177:
bool has_user_activation =If we move this new code up to line 160, we can use it at line 170.
We end up calling this method in non-exclusive VR mode, but that seems okay. (Also, we'd end up exiting if !doc if we decide to change that in the future, but that also seems reasonable.)
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
3 comments:
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #23, Line 181:
// All AR sessions require a user gesture.I agree that this is odd and we shouldn't continue. […]
Done
Patch Set #23, Line 181:
// All AR sessions require a user gesture.I think it is reasonable to make this minor improvement in this CL. […]
Done
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #24, Line 177:
DOMException::Create(kSecurityError, kRequestRequiresUserActivation));If we move this new code up to line 160, we can use it at line 170. […]
Done
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Michael Thiessen (Gerrit)
slgtm
3 comments:
File chrome/browser/android/vr/arcore_device/arcore_device.h:
Patch Set #24, Line 74:
base::BindOncenit: I don't think this BindOnce is necessary.
template <typename DataType>
static void RunCallbackOnTaskRunner(
const scoped_refptr<base::TaskRunner>& task_runner,
base::OnceCallback<void(DataType)> callback,
DataType data) {
task_runner->PostTask(FROM_HERE,
base::BindOnce(std::move(callback), std::move(data)));
}
static void RunCallbackOnTaskRunnerVoid(
const scoped_refptr<base::TaskRunner>& task_runner,
base::OnceClosure callback) {
task_runner->PostTask(FROM_HERE, base::BindOnce(std::move(callback)));
}
template <typename DataType>
base::OnceCallback<void(DataType)> CreateMainThreadCallback(
base::OnceCallback<void(DataType)> callback) {
return base::BindOnce(&ARCoreDevice::RunCallbackOnTaskRunner<DataType>,
main_thread_task_runner_, std::move(callback));
}
base::OnceCallback<void(void)> CreateMainThreadCallbackVoid(
base::OnceCallback<void(void)> callback) {
return base::BindOnce(&ARCoreDevice::RunCallbackOnTaskRunnerVoid,
main_thread_task_runner_, std::move(callback));
}nit: I think it would be better if these helper functions were in the .cc, inside an anonymous namespace. Weird to have a bunch of private functions implemented in the header.
Feel free to do in a followup.
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
if (arcore_gl_thread_)
arcore_gl_thread_->Stop();Unrelated to your CL, but this isn't necessary. The thread destructor will automatically call stop for you.
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
2 comments:
nit: I don't think this BindOnce is necessary.
Done
template <typename DataType>
static void RunCallbackOnTaskRunner(
const scoped_refptr<base::TaskRunner>& task_runner,
base::OnceCallback<void(DataType)> callback,
DataType data) {
task_runner->PostTask(FROM_HERE,
base::BindOnce(std::move(callback), std::move(data)));
}
static void RunCallbackOnTaskRunnerVoid(
const scoped_refptr<base::TaskRunner>& task_runner,
base::OnceClosure callback);
template <typename DataType>
base::OnceCallback<void(DataType)> CreateMainThreadCallback(
base::OnceCallback<void(DataType)> callback) {
return base::BindOnce(&ARCoreDevice::RunCallbackOnTaskRunner<DataType>,
main_thread_task_runner_, std::move(callback));
}
base::OnceCallback<void(void)> CreateMainThreadCallbackVoid(
base::OnceCallback<void(void)> callback);
void PostTaskToGlThread(base::OnceClosure task);
bool IsOnMainThread();nit: I think it would be better if these helper functions were in the . […]
Done
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Michael Thiessen (Gerrit)
1 comment:
template <typename DataType>
static void RunCallbackOnTaskRunner(
const scoped_refptr<base::TaskRunner>& task_runner,
base::OnceCallback<void(DataType)> callback,
DataType data) {
task_runner->PostTask(FROM_HERE,
base::BindOnce(std::move(callback), std::move(data)));
}
static void RunCallbackOnTaskRunnerVoid(
const scoped_refptr<base::TaskRunner>& task_runner,
base::OnceClosure callback);
template <typename DataType>
base::OnceCallback<void(DataType)> CreateMainThreadCallback(
base::OnceCallback<void(DataType)> callback) {
return base::BindOnce(&ARCoreDevice::RunCallbackOnTaskRunner<DataType>,
main_thread_task_runner_, std::move(callback));
}
base::OnceCallback<void(void)> CreateMainThreadCallbackVoid(
base::OnceCallback<void(void)> callback);
void PostTaskToGlThread(base::OnceClosure task);
bool IsOnMainThread();
Done
Well that's not exactly what I was suggesting. I meant putting them next to CreateVRDisplayInfo in the .cc file, with the header not knowing of their existence.
To view, visit change 1055677. To unsubscribe, or for help writing mail filters, visit settings.
Ian Vollick (Gerrit)
Patch set 30:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Rebase" https://chromium-review.googlesource.com/c/1055677/30
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/1055677/30
Bot data: {"action": "start", "triggered_at": "2018-06-07T05:19:30.0Z", "cq_cfg_revision": "c5e58f70d145a56376881fa216a229aaeb1aa6be", "revision": "c647a7cba554272999065e72e32e3e6e64a7ab93"}
Ian Vollick (Gerrit)
Patch set 30:Commit-Queue +1
Iker Jamardo (Gerrit)
Patch set 31:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Replies to nit review comments." https://chromium-review.googlesource.com/c/1055677/31
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/1055677/31
Bot data: {"action": "start", "triggered_at": "2018-06-07T14:26:58.0Z", "cq_cfg_revision": "c5e58f70d145a56376881fa216a229aaeb1aa6be", "revision": "fa3317d5a1ed9a383fa4e27d03e2b9ee4d6650ae"}
Commit Bot (Gerrit)
Commit Bot merged this change.
Request camera permission when AR features are requested.
Adds a new XRSessionRequest structure to be passed when the
RequestSession call is made to be able to pass information like
user gesture. The RequestSession now requests camera permission
in ARCoreDevice. The code to ask for camera permission has been
copied and adapted from
src/chrome/browser/media/webrtc/media_stream_devices_controller.cc
and a re-check for the Android OS level camera permission is still
needed.
Bug: 835037, 845160, 775372
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_vr
Change-Id: Iecabc3927beb6106a1ada5289f3af3ecffd48ed8
Reviewed-on: https://chromium-review.googlesource.com/1055677
Commit-Queue: Iker Jamardo <ijam...@chromium.org>
Reviewed-by: David Dorwin <ddo...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Reviewed-by: Bill Orr <bil...@chromium.org>
Reviewed-by: Raymes Khoury <ray...@chromium.org>
Reviewed-by: Michael Thiessen <mthi...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565347}
---
M chrome/browser/android/vr/arcore_device/arcore_device.cc
M chrome/browser/android/vr/arcore_device/arcore_device.h
M chrome/browser/android/vr/arcore_device/arcore_gl.cc
M chrome/browser/android/vr/arcore_device/arcore_gl.h
M chrome/browser/android/vr/arcore_device/arcore_gl_thread.cc
M chrome/browser/android/vr/arcore_device/arcore_gl_thread.h
M chrome/browser/android/vr/arcore_device/arcore_impl.h
M chrome/browser/geolocation/geolocation_permission_context_android.cc
M chrome/browser/media/webrtc/media_stream_devices_controller.cc
M chrome/browser/permissions/permission_update_infobar_delegate_android.cc
M chrome/browser/permissions/permission_update_infobar_delegate_android.h
M chrome/browser/vr/service/vr_display_host.cc
M chrome/browser/vr/service/vr_display_host.h
M device/vr/public/mojom/vr_service.mojom
M device/vr/test/mock_vr_display_impl.cc
M device/vr/test/mock_vr_display_impl.h
M device/vr/vr_device.h
M device/vr/vr_device_base.cc
M device/vr/vr_device_base.h
M device/vr/vr_device_base_unittest.cc
M device/vr/vr_display_impl.cc
M device/vr/vr_display_impl.h
M device/vr/vr_display_impl_unittest.cc
M third_party/blink/renderer/modules/xr/xr_device.cc
24 files changed, 371 insertions(+), 87 deletions(-)