Improve request session in WebXR [chromium/src : master]
Iker Jamardo (Gerrit)
This change is ready for review.
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
Iker Jamardo would like David Dorwin, Bill Orr, Klaus Weidner, Michael Thiessen, Ian Vollick, Yaron Friedman and Max Rebuschatis to review this change.
Improve request session in WebXR
Improve the support for request session in WebXR mainly focused on
allowing ARCoreDevice to receive a call when the session request
is made. Also improves the support session query with a new ar flag.
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: I8090f64a981114b130e446025ec6299fbb5619e4
---
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/android/gvr/gvr_device.cc
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
M third_party/blink/renderer/modules/xr/xr_session_creation_options.idl
13 files changed, 85 insertions(+), 3 deletions(-)
Michael Thiessen (Gerrit)
4 comments:
File chrome/browser/vr/service/vr_display_host.cc:
bool requires_secure_context =
!kAllowHTTPWebVRWithFlag ||
!base::CommandLine::ForCurrentProcess()->HasSwitch(
switches::kEnableWebVR);
if (requires_secure_context && !IsSecureContext(render_frame_host_)) {Extract a shared function for this.
File device/vr/vr_display_impl.cc:
Patch Set #4, Line 18:
"Access not allowed.";This error message is particularly unhelpful...
Patch Set #4, Line 20:
"No focus."Something less vague, like "Requesting frame does not have focus."
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #4, Line 80:
if (supports_ar_) {This seems wrong. What if a device supports ar and vr?
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
Fixes in patch 5.
4 comments:
display_->RequestSession(std::move(callback));
}
void VRDisplayHost::SetListeningForActivate(bool listening) {
Extract a shared function for this.
Done
File device/vr/vr_display_impl.cc:
Patch Set #4, Line 18:
"No access to the devi
This error message is particularly unhelpful...
I know. I was hoping for some help on this :). How about "No access to the device allowed."?
Patch Set #4, Line 20:
"RequestingSomething less vague, like "Requesting frame does not have focus. […]
Done
File third_party/blink/renderer/modules/xr/xr_device.cc:
This seems wrong. […]
This was just reflecting the current status, not the future one. In the case of ARCore, not specifying ar in the options will still lead to AR. Is that what we want for the moment? @ddorwin
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
David Dorwin (Gerrit)
Thank you. Mostly minor stuff.
30 comments:
Patch Set #5, Line 7:
Improve request session in WebXRThis description is vague.
The most significant thing in this CL is the piping of RequestDevice through to the browser process, so this should probably focus on that.
The second thing (after reverting the IDL change) is the check for AR support, which turned out to be separate from piping RequestDevice. That would ideally be a separate CL, but this is small enough that it's fine. That just means you add something like "Also check whether the device supports AR sessions when the hit-test flag is enabled." You may be able to replace most of the next paragraph.
Patch Set #5, Line 9:
request sessionnit: requestSession
Patch Set #5, Line 11:
support sessionnit: supportsSession
There must be a Bug related to this. ARCore hookup?
File chrome/browser/android/vr/arcore_device/arcore_device.h:
Patch Set #5, Line 28:
// From VRDeviceBasenit: // VRDeviceBase implementation.
File chrome/browser/android/vr/arcore_device/arcore_device.cc:
Patch Set #5, Line 114:
TODO:TODO(ldap and/or bug URL):
We have a bug, so include it somewhere in the comment.
File chrome/browser/vr/service/vr_display_host.h:
Patch Set #5, Line 43:
void RequestSession(RequestSessionCallback callback) override;nit: This should be before RequestPresent.
Patch Set #5, Line 43:
void RequestSession(RequestSessionCallback callback) override;nit: This should be before RequestPresent since they are always called in that order.
Patch Set #5, Line 49:
bool CheckSecureContextRequirement();// TODO(ddorwin): Instead, check before returning this this object at all.
File chrome/browser/vr/service/vr_display_host.cc:
Patch Set #5, Line 21:
const char kSecureContextRequired[] = "A secure context is required.";This should never occur in production, so we don't need to provide a useful message.
Patch Set #5, Line 93:
void VRDisplayHost::RequestSession(RequestSessionCallback callback) {ditto: Above RequestPresent
Patch Set #5, Line 95:
std::move(callback).Run(kSecureContextRequired);We can just fail like at line 65.
Note that the parameter name is named "success", which is the opposite of how we're using it.
Patch Set #5, Line 117:
requires_secure_context &&This is redundant with above.
File device/vr/public/mojom/vr_service.mojom:
Patch Set #5, Line 249:
RequestPresent(VRSubmitFrameClient client,ditto on order
Patch Set #5, Line 255:
string? successWhy are we returning a string?
Also, the lack of a string as an indication of success is a bit unclear, especially since the method above uses a separate bool.
See also comments below.
File device/vr/vr_device_base.h:
Patch Set #5, Line 33:
void OnExitPresent() override;Unrelated: Please move this below IsFallbackDevice after moving it.
Patch Set #5, Line 44:
virtual void RequestSession(ditto on order here and in the next three files.
Patch Set #5, Line 47:
bool IsFallbackDevice() override;unrelated: Please move this up below 35 to group VRDevice overrides and match the VRDevice order.
File device/vr/vr_device_base.cc:
Patch Set #5, Line 74:
std::move(callback).Run(base::Optional<std::string>());// TODO(offenwanger): Implement this for all devices.
File device/vr/vr_display_impl.cc:
Patch Set #5, Line 18:
const char kNotAccessAllowed[] = "No access to the device allowed.";We don't need these. See below.
Patch Set #5, Line 88:
// TODO(ijamardo, https://crbug.com/837538):Provide a short message for the TODO.
Patch Set #5, Line 90:
std::move(callback).Run(kNotAccessAllowed);Note that the use of a string in the interface has complicated things whereas RequestPresent just returns false if either of these conditions are true at line 73.
File third_party/blink/renderer/modules/xr/xr_device.h:
Patch Set #5, Line 96:
void OnRequestSessionComplete(ScriptPromiseResolver* resolver,The comment at line 90 refers only to the two methods above. Thus, there should be an empty line before anything else.
In this case, this method is probably called first, so I would move it up above this block.
Patch Set #5, Line 105:
bool supports_ar_;This should be above line 104 with the other two values that are set by SetXRDisplayInfo.
Even though SetXRDisplayInfo is called in the constructor, I think we might want to set all three to false here just to be clear that they are initialized.
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #5, Line 33:
const char kARNotSupported[] =I'm not sure whether we want to maintain messages for and/or expose the exact reason for failure for every option that might be added.
This might be fine for now since it's behind a flag, but it's also a bit unnecessary since the JS can't ask for AR sessions (see my last comment). Also, we're now down to just one string instead of two in PS4.
bajones@: What are your thoughts?
Patch Set #5, Line 73:
if (options.ar()) {We shouldn't be adding this option (see below).
Instead (copied from line 145):
// TODO(https://crbug.com/828321): Use session options instead.
if (RuntimeEnabledFeatures::WebXRHitTestEnabled()) {Patch Set #5, Line 74:
if (!supports_ar_) {The browser side should prevent support_ar_ without the flag. We could DCHECK this at line 72.
Patch Set #5, Line 168:
fail_messageThis needs to be a DOMException.
See https://cs.chromium.org/chromium/src/third_party/blink/renderer/modules/xr/xr_frame_provider.cc?rcl=d487e411b1ded81eb77bb32795b2785f90473eb8&l=142, which is where failures in line 181 below are handled. Note also that there is only a generic failure message there as well.
Patch Set #5, Line 245:
supports_ar_ = display_info_->capabilities->can_provide_pass_through_images;Good idea! Better than my suggestion.
File third_party/blink/renderer/modules/xr/xr_session_creation_options.idl:
Patch Set #5, Line 11:
boolean ar = false;We don't want to change the web-facing API yet because this hasn't been incubated.
Also, if we were to modify the IDL, we would probably have a separate CL with a layout test.
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
Iker Jamardo uploaded patch set #6 to this change.
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 support session query with a new ar flag.
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: I8090f64a981114b130e446025ec6299fbb5619e4
---
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/android/gvr/gvr_device.cc
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
M third_party/blink/renderer/modules/xr/xr_session_creation_options.idl
13 files changed, 97 insertions(+), 8 deletions(-)
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
Iker Jamardo uploaded patch set #7 to this change.
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.
---
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/android/gvr/gvr_device.cc
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
M third_party/blink/renderer/modules/xr/xr_session_creation_options.idl
13 files changed, 97 insertions(+), 8 deletions(-)
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
Iker Jamardo uploaded patch set #8 to this change.
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
---
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/android/gvr/gvr_device.cc
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
M third_party/blink/renderer/modules/xr/xr_session_creation_options.idl
13 files changed, 97 insertions(+), 8 deletions(-)
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
Replied to David's comments.
30 comments:
Patch Set #5, Line 7:
Add explicit requestSession call to the browser process for WebXR.This description is vague. […]
Done
Patch Set #5, Line 9:
requestSessionnit: requestSession
Done
nit: supportsSession
Done
There must be a Bug related to this. […]
Done
File chrome/browser/android/vr/arcore_device/arcore_device.h:
Patch Set #5, Line 28:
// From VRDeviceBasenit: // VRDeviceBase implementation.
Patch Set #5, Line 114:
TODO:
TODO(ldap and/or bug URL): […]
Done
File chrome/browser/vr/service/vr_display_host.h:
Patch Set #5, Line 43:
void RequestSession(RequestSessionCallback callback) override;
nit: This should be before RequestPresent since they are always called in that order.
Done
Patch Set #5, Line 43:
void RequestSession(RequestSessionCallback callback) override;nit: This should be before RequestPresent.
Done
Patch Set #5, Line 49:
bool CheckSecureContextRequirement();// TODO(ddorwin): Instead, check before returning this this object at all.
Patch Set #5, Line 21:
const char kSecureContextRequired[] = "A secure context is required.";This should never occur in production, so we don't need to provide a useful message.
Done
Patch Set #5, Line 93:
void VRDisplayHost::RequestSession(RequestSessionCallback callback) {ditto: Above RequestPresent
Done
Patch Set #5, Line 95:
std::move(callback).Run(kSecureContextRequired);
We can just fail like at line 65. […]
Done
Patch Set #5, Line 117:
requires_secure_context &&This is redundant with above.
Patch Set #5, Line 249:
RequestPresent(VRSubmitFrameClient client,ditto on order
Done
Patch Set #5, Line 255:
string? success
Why are we returning a string? […]
I seemed important to me to provide an accurate message of why the session request failed. But I might be wrong.
File device/vr/vr_device_base.h:
Patch Set #5, Line 33:
void OnExitPresent() override;Unrelated: Please move this below IsFallbackDevice after moving it.
Done
Patch Set #5, Line 44:
virtual void RequestSession(ditto on order here and in the next three files.
Done
Patch Set #5, Line 47:
bool IsFallbackDevice() override;unrelated: Please move this up below 35 to group VRDevice overrides and match the VRDevice order.
Patch Set #5, Line 74:
std::move(callback).Run(base::Optional<std::string>());// TODO(offenwanger): Implement this for all devices.
Patch Set #4, Line 18:
"No access to the devi
I know. I was hoping for some help on this :). How about "No access to the device allowed. […]
Done
File device/vr/vr_display_impl.cc:
Patch Set #5, Line 18:
const char kNotAccessAllowed[] = "No access to the device allowed.";We don't need these. See below.
Done
Patch Set #5, Line 88:
// TODO(ijamardo, https://crbug.com/837538):Provide a short message for the TODO.
Done
Patch Set #5, Line 90:
std::move(callback).Run(kNotAccessAllowed);
Note that the use of a string in the interface has complicated things whereas RequestPresent just re […]
Done
File third_party/blink/renderer/modules/xr/xr_device.h:
Patch Set #5, Line 96:
void OnRequestSessionComplete(ScriptPromiseResolver* resolver,
The comment at line 90 refers only to the two methods above. […]
Done
Patch Set #5, Line 105:
bool supports_ar_;
This should be above line 104 with the other two values that are set by SetXRDisplayInfo. […]
Done
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #5, Line 73:
if (options.ar()) {We shouldn't be adding this option (see below). […]
Done
Patch Set #5, Line 74:
if (!supports_ar_) {The browser side should prevent support_ar_ without the flag. We could DCHECK this at line 72.
This is what I did initially and Michael said it was incorrect because there could be devices that support VR and AR at the same time. Ok to the DCHECK.
Patch Set #5, Line 168:
fail_message
This needs to be a DOMException. […]
Done
Patch Set #5, Line 245:
supports_ar_ = display_info_->capabilities->can_provide_pass_through_images;Good idea! Better than my suggestion.
Patch Set #5, Line 11:
boolean ar = false;
We don't want to change the web-facing API yet because this hasn't been incubated. […]
So, just remove it? How can I check it if it's not there? I don't really understand wanting to have this flag at this moment.
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
Iker Jamardo uploaded patch set #11 to this change.
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
---
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/android/gvr/gvr_device.cc
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
12 files changed, 97 insertions(+), 10 deletions(-)
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Bill Orr (Gerrit)
4 comments:
!kAllowHTTPWebVRWithFlag ||
!base::CommandLine::ForCurrentProcess()->HasSwitch(
switches::kEnableWebVR);
Maybe add a comment. We require secure connections unless both the webvr flag and the http flag are enabled.
BTW - it seems the AR session should just always require a secure context. Why would we check the webvr flag for webxr APIs? Is http something we want to continue to support moving forward?
File device/vr/android/gvr/gvr_device.cc:
Patch Set #11, Line 107:
can_provide_pass_through_imagesdo we have to set this, or is it default false?
if we have to set it, should we set it for oculus/openvr/orientation/test devices too?
File third_party/blink/renderer/modules/xr/xr_device.h:
Patch Set #11, Line 105:
= falsethese are already set, as the constructor calls SetXRDisplayInfo.
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #11, Line 72:
DCHECKis this dcheck going to hit when you don't have the flag enabled?
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
David Dorwin (Gerrit)
16 comments:
Patch Set #10, Line 13:
Also improves the supportSession query with a new ar flag.Also adds a check to both supportSession and requestSession to ensure the device supports AR capabilities when the hit-test flag is enabled.
Patch Set #10, Line 15:
Bug: 837538While this might address that bug (though it's not quite clear what it meant), the majority of this CL is really for something else. Isn't it laying the groundwork for 835037?
Also, I think 837538 was just for a TODO in code than never landed. Thus, we can probably just close it as WontFix (obsolete) and remove it here.
File chrome/browser/vr/service/vr_display_host.h:
Remove. Mistake in my comment. :)
File chrome/browser/vr/service/vr_display_host.cc:
Patch Set #10, Line 91:
void VRDisplayHost::RequestSession(RequestSessionCallback callback) {nit: move up to line 73.
File device/vr/public/mojom/vr_service.mojom:
Patch Set #10, Line 247:
, string? fail_messageGiven that we didn't provide a message from RequestPresent, I think we can probably live without one here too. Another thing to note is that these detailed messages provide information about the client and/or user configuration, which can be undesirable. Especially if there's nothing the app can do about it, we should prefer not adding the complexity. That also means we don't have good mechanisms to help developers during development. There is a way to do only that (a separate message the only goes to the console), but I'm not sure the existing failure cases warrant that.
Also, we don't appear to be passing any strings, so just remove it?
Patch Set #10, Line 248:
// The returned transport_options is marked optional: it's null fornit: Since we have a comment, add a newline before the comment and after the method.
File device/vr/vr_device_base.h:
Patch Set #10, Line 33:
bool IsFallbackDevice() override;These two should be below SetMagicWindowEnabled to match the base class.
File device/vr/vr_device_base.cc:
Patch Set #10, Line 62:
// TODO(offenwanger): Implement this for all devices.Move above the code.
File device/vr/vr_display_impl.h:
Patch Set #10, Line 63:
bool IsAccessAllowedAndInFocusedFrame();Is there a reason for this helper function? The long combined name is perhaps more confusing than the code itself. It would be better if it was named something like CanStartNewSession. That would allow us to keep the policy separate from the call site and question.
File device/vr/vr_display_impl.cc:
Patch Set #10, Line 77:
if (!device_->IsAccessAllowed(this) || !InFocusedFrame()) {If we are going to have a helper function, we should use it anywhere we check this.
I wonder if this could ever fail in WebXR (or WebVR once we start calling RequestSession).
Patch Set #10, Line 142:
return !device_->IsAccessAllowed(this) || !InFocusedFrame();The logic is backwards.
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #5, Line 74:
return nullptr;This is what I did initially and Michael said it was incorrect because there could be devices that s […]
He's correct. However, all of this is temporary - hence the TODO.
I didn't mean to only DCHECK. I was saying we should do that in addition.
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #10, Line 33:
const char kRequestSessionDenied[] = "Request session was denied.";We don't actually know and/or want to say whether the user denied the session.
How about this?
const char kSessionNotSupported[] = "The specified session configuration is not supported.";// TODO(https://crbug.com/828321): Use session options instead of the flag.
bool is_ar = RuntimeEnabledFeatures::WebXRHitTestEnabled();Patch Set #10, Line 72:
DCHECK(RuntimeEnabledFeatures::WebXRHitTestEnabled() && supports_ar_);I think this should be:
if (is_ar) {
if (!supports_ar_)
return kSessionNotSupported;// TODO(https://crbug.com/828321): Expose information necessary to check combinations.
// For now, exclusive AR is not supported.
if (options.exclusive())
return kSessionNotSupported;
} else {
// TODO(https://crbug.com/828321): Remove this check when properly supporting multiple VRDevice registration.
DCHECK(!supports_ar)
// TODO(https://crbug.com/828321): Check that VR is supported.
}Patch Set #10, Line 165:
kNotAllowedErrorWe may at some point want to return NotSupported in some cases. What does the spec say we should return?
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
Iker Jamardo uploaded patch set #12 to this change.
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 adds a check to both supportSession and requestSession to ensure
the device supports AR capabilities when the hit-test flag is enabled.
Bug: 837538
---
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/android/gvr/gvr_device.cc
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
12 files changed, 97 insertions(+), 10 deletions(-)
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
25 comments:
Patch Set #10, Line 13:
Also adds a check to both supportSession and requestSession to ensureAlso adds a check to both supportSession and requestSession to ensure the device supports AR capabil […]
Done
While this might address that bug (though it's not quite clear what it meant), the majority of this […]
I thought I had to indicate what bugs this CL is resolving. 837538 is gone after this CL AFAIU. Should I also mention partially resolved bugs?
File chrome/browser/vr/service/vr_display_host.h:
Remove. Mistake in my comment. […]
Done
File chrome/browser/vr/service/vr_display_host.cc:
Patch Set #10, Line 91:
web_contents, Mode::kNoVr, false);
nit: move up to line 73.
Patch Set #11, Line 107:
can_provide_pass_through_images
do we have to set this, or is it default false? […]
Done
Patch Set #11, Line 107:
can_provide_pass_through_images
do we have to set this, or is it default false? […]
I think it needs to be set.
File device/vr/public/mojom/vr_service.mojom:
Given that we didn't provide a message from RequestPresent, I think we can probably live without one […]
What should the message for the exception be when the requestSession fails?
nit: Since we have a comment, add a newline before the comment and after the method.
Done
nit: Since we have a comment, add a newline before the comment and after the method.
Done
File device/vr/vr_device_base.h:
Patch Set #10, Line 33:
mojom::VRDisplayInfoPtr GetVRDisplayInfo() final;
These two should be below SetMagicWindowEnabled to match the base class.
Done
File device/vr/vr_device_base.cc:
Patch Set #10, Line 62:
void VRDeviceBase::RequestSession(Move above the code.
Done
File device/vr/vr_display_impl.h:
Patch Set #10, Line 63:
bool CanStartNewSession();Is there a reason for this helper function? The long combined name is perhaps more confusing than th […]
I thought the whole purpose of the bug that led to the creation of this method was to have one single entry point for both RequestPresent and RequestSession. I will change the name.
File device/vr/vr_display_impl.cc:
Patch Set #10, Line 77:
if (!CanStartNewSession()) {
If we are going to have a helper function, we should use it anywhere we check this. […]
I thought I did. My bad.
Patch Set #10, Line 77:
if (!CanStartNewSession()) {If we are going to have a helper function, we should use it anywhere we check this. […]
Done
Patch Set #10, Line 142:
return device_->IsAccessAllowed(this) && InFocusedFrame();The logic is backwards.
Done
File third_party/blink/renderer/modules/xr/xr_device.h:
Patch Set #11, Line 105:
lusive_
these are already set, as the constructor calls SetXRDisplayInfo.
David mentioned that better to ensure them to be set to false here to ensure the call is made.
Patch Set #11, Line 105:
lusive_
these are already set, as the constructor calls SetXRDisplayInfo.
Done
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #5, Line 74:
bool is_ar = RuntimeEnabledFeatures::WebXRHitTestEnabled();He's correct. However, all of this is temporary - hence the TODO. […]
Done
Patch Set #5, Line 74:
bool is_ar = RuntimeEnabledFeatures::WebXRHitTestEnabled();He's correct. However, all of this is temporary - hence the TODO. […]
Done
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #10, Line 33:
const char kSessionNotSupported[] =We don't actually know and/or want to say whether the user denied the session. […]
Done
// TODO(https://crbug.com/828321): Use session options instead of the flag. […]
Done
I think this should be: […]
I think the exclusive not supported if already being handled by the ARCoreDevice stating that does not support exclusive sessions, so re-checking here seems redundant. I will add all of the other checks.
Patch Set #10, Line 165:
y_->RequestSessiWe may at some point want to return NotSupported in some cases. […]
This is what the spec says. My read from it is that there is nothing in the spec about other reasons why the session request could fail. I guess NotSupportError seems the closest to what could be used but I might be wrong. Should we file an issue in the spec repo for this?
--------
When the requestSession(options) method is invoked, the user agent MUST return a new Promise promise and run the following steps in parallel:
Let device be the target XRDevice object.
If the options are not supported by the device device, reject promise with a NotSupportedError and abort these steps.
Let exclusive be the exclusive attribute of the options argument.
If exclusive is true and device’s exclusive session is not null, reject promise with an InvalidStateError and abort these steps.
If exclusive is true and the algorithm is not triggered by user activation, reject promise with a SecurityError and abort these steps.
Let session be a new XRSession.
Initialize the session session with the session description given by options.
If exclusive is true set the device’s exclusive session to session.
Else append session to device’s list of non-exclusive sessions.
Resolve promise with session.
File third_party/blink/renderer/modules/xr/xr_device.cc:
is this dcheck going to hit when you don't have the flag enabled?
Done
is this dcheck going to hit when you don't have the flag enabled?
Done
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
Iker Jamardo uploaded patch set #15 to this change.
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 adds a check to both supportSession and requestSession to ensure
the device supports AR capabilities when the hit-test flag is enabled.
---
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/android/gvr/gvr_device.cc
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
12 files changed, 119 insertions(+), 23 deletions(-)
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
Fixes for some final review comments.
5 comments:
I thought I had to indicate what bugs this CL is resolving. 837538 is gone after this CL AFAIU. […]
Did not close the bug, so not adding anything here.
File chrome/browser/vr/service/vr_display_host.cc:
equestSession.
// We require secure connections unless both the webvr flag and the
// http flag are enabled.Maybe add a comment. […]
@ddorwin: Please advise. Still, at this point this code still does not need to know about AR vs VR, correct?
File device/vr/public/mojom/vr_service.mojom:
What should the message for the exception be when the requestSession fails?
Please, check my approach on this.
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #5, Line 33:
const char kSessionNotSupported[] =I'm not sure whether we want to maintain messages for and/or expose the exact reason for failure for […]
I have taken the string out for now.
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #10, Line 165:
y_->RequestSessiThis is what the spec says. […]
Please, check my approach to resolve this.
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Klaus Weidner (Gerrit)
1 comment:
Patch Set #11, Line 107:
can_provide_pass_through_images
I think it needs to be set.
Mojo member booleans default to false. We depend on this in other places also, i.e. setting up transport options.
I think tt's OK to explicitly set something false if that makes things clearer, i.e. it's an unexpected value for a device type, but we shouldn't add redundant "false" values to unrelated devices. That's just extra maintenance burden and churn.
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
David Dorwin (Gerrit)
Looking good. There are a few remaining things to address.
9 comments:
I thought I had to indicate what bugs this CL is resolving. 837538 is gone after this CL AFAIU. […]
Yes, you should mention bugs that the CL addresses. My point is that this CL does not address the issue. This CL is really _introducing_ the issue.
Issue 837538 issue was originally created in response to this comment: https://crrev.com/c/1014418/23/device/vr/vr_display_impl.cc#75. This CL was extracted from that one, so this is the CL that introduces the issue. I've updated the description of https://crbug.com/837538 to better reflect the intent and current plan.
In any case, there should be an entry for the functionality this CL adds. Is this part of 835037 or something else?
As noted in the PS10 thread, there should be a Bug entry that covers the bulk of this work. It may be 835037 but there might be another more specific one too.
File chrome/browser/vr/service/vr_display_host.h:
Patch Set #17, Line 49:
bool CheckSecureContextRequirement();Add the following (slightly updated to include the bug) back. The "Remove" statement in PS10 was referring to the highlighted extra "this" in the comment.
// TODO(https://crbug.com/837538): Instead, check before returning this object.
File chrome/browser/vr/service/vr_display_host.cc:
Patch Set #17, Line 109:
// TODO(https://crbug.com/837538): Reconcile RequestPresent andSee the comment in PS10's commit message. I think we can just remove this and use the suggested comment in the header file.
File device/vr/public/mojom/vr_service.mojom:
Patch Set #17, Line 257:
ExitPresent();nit: newline before since the comment at line 249-250 only applies to the one method.
File device/vr/vr_display_impl.h:
Patch Set #10, Line 63:
bool CanStartNewSession();
I thought the whole purpose of the bug that led to the creation of this method was to have one singl […]
One single point for the check, yes. See other comments in this reply. However, we're not fixing that in this CL.
File third_party/blink/renderer/modules/xr/xr_device.cc:
I think the exclusive not supported if already being handled by the ARCoreDevice stating that does n […]
Once we support both GVR and ARCore at the same time, supports_exclusive_ will be true on Android. Thus, we do need to check that both AR and exclusive are not requested. Addressing the first TODO will ensure that we can specifically check what is supported (including this combination), so we won't need this platform-independent (but based on Android) check anymore.
Patch Set #10, Line 165:
y_->RequestSessi
Please, check my approach to resolve this.
Your approach LG. Messages are not normatively specified.
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #17, Line 79:
// combinations.As replied in PS10, I think we need to add:
// For now, exclusive AR is not supported.
if (options.exclusive())
return kSessionNotSupported;
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
Iker Jamardo uploaded patch set #18 to this change.
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 adds a check to both supportSession and requestSession to ensure
the device supports AR capabilities when the hit-test flag is enabled.
Bug:
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: I8090f64a981114b130e446025ec6299fbb5619e4
---
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/android/gvr/gvr_device.cc
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
12 files changed, 124 insertions(+), 23 deletions(-)
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
Iker Jamardo uploaded patch set #19 to this change.
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 adds a check to both supportSession and requestSession to ensure
the device supports AR capabilities when the hit-test flag is enabled.
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: I8090f64a981114b130e446025ec6299fbb5619e4
---
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/android/gvr/gvr_device.cc
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
12 files changed, 124 insertions(+), 23 deletions(-)
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
9 comments:
Yes, you should mention bugs that the CL addresses. […]
It is part of 835037 so I will reference it here and in the PermissionRequest CL too.
As noted in the PS10 thread, there should be a Bug entry that covers the bulk of this work. […]
Done
File chrome/browser/vr/service/vr_display_host.h:
Add the following (slightly updated to include the bug) back. […]
Done
File chrome/browser/vr/service/vr_display_host.cc:
Patch Set #17, Line 109:
// We require secure connections unless both the webvr flag and theSee the comment in PS10's commit message. […]
Done
File device/vr/public/mojom/vr_service.mojom:
nit: newline before since the comment at line 249-250 only applies to the one method.
Done
nit: newline before since the comment at line 249-250 only applies to the one method.
Done
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #5, Line 33:
const char kSessionNotSupported[] =
I have taken the string out for now.
Done
File third_party/blink/renderer/modules/xr/xr_device.cc:
Once we support both GVR and ARCore at the same time, supports_exclusive_ will be true on Android. […]
The problem in my opinion is that we are continuing with legacy code. The flags for WebXR should be more complex than just (supports exclusive or not). It should be supports exclusive in non ar: true. I think making this call at this moment is premature and actually shows a problem that a new redesign should overcome as the session options are more complex as they were for WebVR.
Still, I will add the change as requested.
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #17, Line 79:
// TODO(https://crbug.com/828321): Expose information necessary to checkAs replied in PS10, I think we need to add: […]
Done
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
David Dorwin (Gerrit)
LGTM. Thank you.
Patch set 20:Code-Review +1
Bill Orr (Gerrit)
Looks good to me. A few nits/opinions, but nothing blocking.
Patch set 20:Code-Review +1
5 comments:
File chrome/browser/vr/service/vr_display_host.cc:
Patch Set #20, Line 55:
CheckSecureContextRequirementminor/opinion nit (feel free to ignore): Consider renaming to IsSecureContextRequirementSatisfied() or something similar that makes it clear what true/false means in the return value.
File device/vr/android/gvr/gvr_device.cc:
Patch Set #20, Line 107:
can_provide_pass_through_imagesI'm still not sure why we have this here, but not for other devices. We should be consistent.
File device/vr/public/mojom/vr_service.mojom:
Patch Set #20, Line 247:
RequestSessionconsider adding a comment for when this is called (relative to other methods), can we get poses before calling this, what is expected to occur before calling this, etc.
File device/vr/vr_display_impl.cc:
Patch Set #20, Line 69:
RequestSessionI wonder if we should be tracking anywhere that this succeeded, so we know whether or not to allow calls to GetPose, etc.
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #20, Line 170:
RequestSessionshould we stick a RequestSession in webvr too? (currently RequestSession doesn't do anything of interest in the browser process, but moving forwards it may).
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Yaron Friedman (Gerrit)
chrome/browser/android part lgtm but didn't review the reest
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
6 comments:
File chrome/browser/vr/service/vr_display_host.cc:
ttp flag are enabled.
bool requires_secure_context =
!kAllowHTTPWebVRWithFlag ||@ddorwin: Please advise. […]
Done
File chrome/browser/vr/service/vr_display_host.cc:
Patch Set #20, Line 55:
IsSecureContextRequirementSatminor/opinion nit (feel free to ignore): Consider renaming to IsSecureContextRequirementSatisfied() […]
Done
File device/vr/android/gvr/gvr_device.cc:
I'm still not sure why we have this here, but not for other devices. We should be consistent.
Would you be ok adding an = false to the mojom structure and then not have to do this in the devices?
File device/vr/public/mojom/vr_service.mojom:
Patch Set #20, Line 247:
// Request toconsider adding a comment for when this is called (relative to other methods), can we get poses befo […]
Done
File device/vr/vr_display_impl.cc:
Patch Set #20, Line 69:
RequestSession
I wonder if we should be tracking anywhere that this succeeded, so we know whether or not to allow c […]
Sounds like a good idea but I think it should be in arcore_device instead of here, correct?
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #20, Line 170:
RequestSession
should we stick a RequestSession in webvr too? (currently RequestSession doesn't do anything of int […]
Won't this be resolved when Anna's refactor lands?
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Bill Orr (Gerrit)
3 comments:
Would you be ok adding an = false to the mojom structure and then not have to do this in the devices […]
I think mojo booleans are false by default. There shouldn't be a behavior change difference from what you have here - I'm just calling out that we have ~5 device implementations, and only 2 are updated. If we don't pay attention to the other devices, we could regress things. A safer pattern could be updating all of them or only the ones that set the value to true.
File device/vr/vr_display_impl.cc:
Patch Set #20, Line 69:
RequestSession
Sounds like a good idea but I think it should be in arcore_device instead of here, correct?
correct
File third_party/blink/renderer/modules/xr/xr_device.cc:
Patch Set #20, Line 170:
RequestSession
Won't this be resolved when Anna's refactor lands?
potentially
Once we depend on RequestSession being called, we could otherwise have issues with webvr when the ar flag is enabled.
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
1 comment:
Patch Set #20, Line 69:
RequestSessionSounds like a good idea but I think it should be in arcore_device instead of here, correct?
This looks a bit harder than it initially looks as there could be more than one request session. I think Anna's new refactor should take care of this correctly handling each session requests independently (even though internally there will be just one ARCore session).
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Klaus Weidner (Gerrit)
modules/xr/ LGTM
Patch set 21:Code-Review +1
David Dorwin (Gerrit)
6 comments:
File device/vr/public/mojom/vr_service.mojom:
Patch Set #21, Line 97:
= falseThis is now the only member where we're explicitly initializing. Someone said earlier that Mojo initializes bools to false. Presumably, the uint32s below get initialized to 0 too.
We should follow whatever is the best practice for .mojom files.Patch Set #21, Line 249:
shouldmust
Patch Set #21, Line 249:
any other oneIn addition to being informal/imprecise, this is especially so since the methods we're referencing aren't even on this interface.
Patch Set #21, Line 250:
// GetFrameData or the return values could be null.The APIs should make this impossible, and that is what we are moving towards. Maybe add a TODO(https://crbug.com/842025) here.
File device/vr/vr_device_base.cc:
Patch Set #21, Line 65:
offenwanger
File device/vr/vr_display_impl.cc:
Patch Set #20, Line 69:
RequestSession
This looks a bit harder than it initially looks as there could be more than one request session. […]
I think we'll need the device's method to call back into this class to do some work then call the |callback|. We'll do that as part of the refactoring (https://crbug.com/842025).
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
David Dorwin (Gerrit)
1 comment:
ttp flag are enabled.
bool requires_secure_context =
!kAllowHTTPWebVRWithFlag ||
Done
I agree that we ideally shouldn't check this for WebXR, which is always secure. However, we want to use RequestSession() in both paths and tracking which API was called just adds complexity.
Plus, when we move this to providing the interface (https://crbug.com/837538), we won't even be able to check on individual methods.
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
David Dorwin (Gerrit)
1 comment:
File device/vr/public/mojom/vr_service.mojom:
Patch Set #21, Line 249:
// method should be called before calling any other one like GetPose orWebVR does not do this. Ideally, we'd address that before landing this code, but we could probably do that separately. If so, you'll need a TODO(https://crbug.com/842042).
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Biao She (Gerrit)
chrome/browser/vr lgtm with nits
Patch set 21:Code-Review +1
2 comments:
File chrome/browser/vr/service/vr_display_host.h:
Patch Set #21, Line 50:
// TODO(https://crbug.com/837538): Instead, check before returning thisIt feel like this comment doesn't belong here? It feels it belongs to the implementation for RequestSession, where we do this check?
File chrome/browser/vr/service/vr_display_host.cc:
Patch Set #21, Line 110:
// http flag are enabled.Is this shared both by AR and VR? It is a little bit odd that webvr flags could affect AR. Perhaps we should rename to xr flag at some point if this is the case. It is probably out of scope for this CL. But if make sense, would you mind create a crbug for it and reference it here.
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
David Dorwin (Gerrit)
2 comments:
Patch Set #21, Line 50:
// TODO(https://crbug.com/837538): Instead, check before returning this
It feel like this comment doesn't belong here? It feels it belongs to the implementation for Request […]
I agree this is a little odd here. However, the intent is to move the check out of this class to where this class is instantiated. Since it's called from two places, we'd need TODOs in both. This is really an improvement that is unrelated to either call. It also seems better here than and the definition of this method.
File chrome/browser/vr/service/vr_display_host.cc:
Patch Set #21, Line 110:
// http flag are enabled.
Is this shared both by AR and VR? It is a little bit odd that webvr flags could affect AR. […]
Yes, it is, and I agree (see below). No, we don't want to change this to "XR" because the exception only applies to WebVR. WebXR has been secure context only from the beginning.
See also my previous comment on this: https://chromium-review.googlesource.com/c/chromium/src/+/1046107/11/chrome/browser/vr/service/vr_display_host.cc#112
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
11 comments:
Patch Set #21, Line 50:
// TODO(https://crbug.com/837538): Instead, check before returning this
I agree this is a little odd here. […]
Done
File chrome/browser/vr/service/vr_display_host.cc:
Patch Set #21, Line 110:
// http flag are enabled.
Yes, it is, and I agree (see below). […]
Done
File device/vr/public/mojom/vr_service.mojom:
Patch Set #21, Line 97:
his mayThis is now the only member where we're explicitly initializing. […]
Done
Patch Set #21, Line 249:
ly initializeIn addition to being informal/imprecise, this is especially so since the methods we're referencing a […]
Done
Patch Set #21, Line 249:
es ifmust
Done
Patch Set #21, Line 249:
// indicates if the session was successfully initialized or not. This methodWebVR does not do this. […]
Done
Patch Set #21, Line 250:
// must be called before RequestPresent.The APIs should make this impossible, and that is what we are moving towards. […]
Done
File device/vr/vr_display_impl.cc:
Patch Set #20, Line 69:
RequestSession
I think we'll need the device's method to call back into this class to do some work then call the |c […]
Done
Patch Set #20, Line 69:
RequestSession
I think we'll need the device's method to call back into this class to do some work then call the |c […]
Done
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
Please, review this CL. Thank you very much.
Iker Jamardo (Gerrit)
Please, review this CL. Thank you.
David Dorwin (Gerrit)
Thanks.
1 comment:
File device/vr/public/mojom/vr_service.mojom:
Patch Set #23, Line 250:
RequestPresentIt must also be called before magic window methods. We'll make this required by the interfaces soon (842025), but we should somehow note that for now.
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
1 comment:
Patch Set #23, Line 250:
RequestPresent
It must also be called before magic window methods. […]
One thing I'm not really understanding is why we need an explicit RequestSession() call. At face value, it doesn't seem to really pass much information back to the renderer -- just a simple bool true/false.
What will happen if we don't call this method first? And how will the proposed Mojo interface refactoring rearrange this?
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
1 comment:
Patch Set #23, Line 250:
RequestPresent
One thing I'm not really understanding is why we need an explicit RequestSession() call. […]
WebXR, compared to WebVR, requires to be able to request a session, and this operation may fail. WebVR directly assumes that when a device is available, is already initialized.
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
David Dorwin (Gerrit)
Daniel, let me know if you have more questions.
1 comment:
Patch Set #23, Line 250:
RequestPresent
WebXR, compared to WebVR, requires to be able to request a session, and this operation may fail. […]
In the short term, we need a mechanism to request permissions before deciding whether to resolve the promise in Blink. See https://crrev.com/c/1055677. This CL was broken out of a larger CL so we could focus on the complexities of permissions in a smaller CL.
Soon, we will also need to select the correct AR/VR runtime based on information in the request. That will include information like adding is_exclusive and is_ar to the request call.
Currently, this call is only used for permissions for AR, which WebVR doesn't support. Thus, there are no problems with WebVR not calling it. We intend to make WebVR call it, but we need to refactor other interfaces that assume all devices should be provided to the application and don't provide an opportunity to initialize a specific device. This work is tracked by the second bug below. See https://crbug.com/842042.
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Patch set 24:Code-Review +1
1 comment:
File third_party/blink/renderer/modules/xr/xr_device.h:
Patch Set #24, Line 88:
const char* checkSessionSupport(const XRSessionCreationOptions&) const;Please fix the naming of this function in a followup.
To view, visit change 1046107. To unsubscribe, or for help writing mail filters, visit settings.
Iker Jamardo (Gerrit)
Patch set 25: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 to master" https://chromium-review.googlesource.com/c/1046107/25
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/1046107/25
Bot data: {"action": "start", "triggered_at": "2018-05-21T14:34:24.0Z", "cq_cfg_revision": "51d0c88a708dbef53e0e37b949ac3666c7235016", "revision": "cb71d348e6a457997e17b53a8cb74f977ed92642"}
Commit Bot (Gerrit)
Try jobs failed on following builders:
linux_chromium_rel_ng on luci.chromium.try (JOB_FAILED, https://ci.chromium.org/p/chromium/builders/luci.chromium.try/linux_chromium_rel_ng/98343)
David Dorwin (Gerrit)
Patch set 26: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/1046107/26
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/1046107/26
Bot data: {"action": "start", "triggered_at": "2018-05-22T01:25:35.0Z", "cq_cfg_revision": "532569b00f8a3a2da1ba8205a83c9048284fb5c3", "revision": "ccb19fb5bf37989f3a9d6a3d87d468446ae28c3b"}
Commit Bot (Gerrit)
Try jobs failed on following builders:
mac_chromium_rel_ng on luci.chromium.try (JOB_FAILED, https://ci.chromium.org/p/chromium/builders/luci.chromium.try/mac_chromium_rel_ng/51615)
Iker Jamardo (Gerrit)
Patch set 28:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Fixes layout test failure." https://chromium-review.googlesource.com/c/1046107/28
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/1046107/28
Bot data: {"action": "start", "triggered_at": "2018-05-24T05:56:22.0Z", "cq_cfg_revision": "f5647662bbf78a128e2fe6956afaa13fb042b968", "revision": "4c7423d7cb1573b9ac24e2090cf431eb1cdcb67b"}
Commit Bot (Gerrit)
Try jobs failed on following builders:
mac_chromium_rel_ng on luci.chromium.try (JOB_FAILED, https://ci.chromium.org/p/chromium/builders/luci.chromium.try/mac_chromium_rel_ng/54150)
Iker Jamardo (Gerrit)
Patch set 29:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Fix for layout test." https://chromium-review.googlesource.com/c/1046107/29
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/1046107/29
Bot data: {"action": "start", "triggered_at": "2018-05-24T19:27:28.0Z", "cq_cfg_revision": "f5647662bbf78a128e2fe6956afaa13fb042b968", "revision": "7edab320dc6f6921274e985e9fc8f33c46370ce5"}
Commit Bot (Gerrit)
Try jobs failed on following builders:
linux_chromium_rel_ng on luci.chromium.try (JOB_FAILED, https://ci.chromium.org/p/chromium/builders/luci.chromium.try/linux_chromium_rel_ng/102080)
Iker Jamardo (Gerrit)
Patch set 32:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Fixes layout test errors." https://chromium-review.googlesource.com/c/1046107/32
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/1046107/32
Bot data: {"action": "start", "triggered_at": "2018-05-25T22:40:47.0Z", "cq_cfg_revision": "6a25833cb93164fb378d8087f9dae5fc28fc9b1e", "revision": "07c2aea747eed0e275469b11cdb7cbfa24920710"}
Commit Bot (Gerrit)
Commit Bot merged this change.
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 adds a check to both supportSession and requestSession to ensure
the device supports AR capabilities when the hit-test flag is enabled.
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: I8090f64a981114b130e446025ec6299fbb5619e4
Reviewed-on: https://chromium-review.googlesource.com/1046107
Commit-Queue: Iker Jamardo <ijam...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Reviewed-by: Klaus Weidner <kla...@chromium.org>
Reviewed-by: Biao She <bs...@chromium.org>
Reviewed-by: David Dorwin <ddo...@chromium.org>
Reviewed-by: Bill Orr <bil...@chromium.org>
Reviewed-by: Yaron Friedman <yfri...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562044}
---
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.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/WebKit/LayoutTests/xr/resources/xr-device-mocking.js
M third_party/WebKit/LayoutTests/xr/resources/xr-test-utils.js
M third_party/blink/renderer/modules/xr/xr_device.cc
M third_party/blink/renderer/modules/xr/xr_device.h
M third_party/blink/renderer/modules/xr/xr_frame_provider.cc
15 files changed, 159 insertions(+), 35 deletions(-)