Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for features, profiles, and mocking ray mode/handedness #119

Merged
merged 3 commits into from Jan 21, 2020

Conversation

@Manishearth
Copy link
Member

Manishearth commented Jan 13, 2020

@Manishearth Manishearth mentioned this pull request Jan 13, 2020
2 of 2 tasks complete
@jdm
Copy link
Member

jdm commented Jan 15, 2020

Copy link
Member

asajeffrey left a comment

Just some minor nits.

RequestSession(SessionMode, Sender<Result<Session, Error>>, Sender<Frame>),
RequestSession(
SessionMode,
SessionInit,

This comment has been minimized.

This comment has been minimized.

@Manishearth

Manishearth Jan 21, 2020

Author Member

That's a quirk of webidl, dictionary arguments with all optional fields must be optional. The bindings generation already takes care of this for us.

impl SessionInit {
/// Helper function for validating a list of requested features against
/// a list of supported features for a given mode
pub fn validate(&self, mode: SessionMode, supported: &[String]) -> Result<Vec<String>, Error> {

This comment has been minimized.

@asajeffrey

asajeffrey Jan 21, 2020

Member

Is it worth converting these strings to atoms to avoid cloning them?

This comment has been minimized.

@Manishearth

Manishearth Jan 21, 2020

Author Member

They're going to be created once per session, that's not a big deal imo

/// a list of supported features for a given mode
pub fn validate(&self, mode: SessionMode, supported: &[String]) -> Result<Vec<String>, Error> {
for f in &self.required_features {
// viewer and local in immersive are granted by default

This comment has been minimized.

@asajeffrey

asajeffrey Jan 21, 2020

Member

spec link?

}

if !supported.contains(f) {
return Err(Error::UnsupportedFeature(f.into()));

This comment has been minimized.

@asajeffrey

asajeffrey Jan 21, 2020

Member

This will only report an error for the first missing feature, is that what's wanted?

This comment has been minimized.

@Manishearth

Manishearth Jan 21, 2020

Author Member

Yeah, JS doesn't see this error anyway, and the spec doesn't expose exactly what features were unsupported (but I have a PR to make it expose what features were granted )

@Manishearth Manishearth force-pushed the Manishearth:features branch from c7887e3 to 39bdd27 Jan 21, 2020
@Manishearth
Copy link
Member Author

Manishearth commented Jan 21, 2020

@bors-servo r=asajeffrey

@bors-servo
Copy link
Contributor

bors-servo commented Jan 21, 2020

📌 Commit 39bdd27 has been approved by asajeffrey

@bors-servo
Copy link
Contributor

bors-servo commented Jan 21, 2020

Testing commit 39bdd27 with merge 9ef71da...

bors-servo added a commit that referenced this pull request Jan 21, 2020
Support for features, profiles, and mocking ray mode/handedness

r? @jdm @asajeffrey

Servo side: servo/servo#25504
@bors-servo
Copy link
Contributor

bors-servo commented Jan 21, 2020

💔 Test failed - checks-travis

@Manishearth
Copy link
Member Author

Manishearth commented Jan 21, 2020

@bors-servo r=asajeffrey

@bors-servo
Copy link
Contributor

bors-servo commented Jan 21, 2020

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
@bors-servo
Copy link
Contributor

bors-servo commented Jan 21, 2020

📌 Commit 39bdd27 has been approved by asajeffrey

bors-servo added a commit that referenced this pull request Jan 21, 2020
Support for features, profiles, and mocking ray mode/handedness

r? @jdm @asajeffrey

Servo side: servo/servo#25504
@bors-servo
Copy link
Contributor

bors-servo commented Jan 21, 2020

Testing commit 39bdd27 with merge 33072cb...

@bors-servo
Copy link
Contributor

bors-servo commented Jan 21, 2020

☀️ Test successful - checks-travis
Approved by: asajeffrey
Pushing 33072cb to master...

@bors-servo bors-servo merged commit 39bdd27 into servo:master Jan 21, 2020
1 of 3 checks passed
1 of 3 checks passed
Travis CI - Pull Request Build Failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
homu Test successful
Details
@Manishearth Manishearth deleted the Manishearth:features branch Jan 21, 2020
bors-servo added a commit to servo/servo that referenced this pull request Jan 21, 2020
Support features in webxr

Based on servo/webxr#119

Todo:
 - [x] gate reference space creation on feature presence
 - [x] Fix the `features_deviceSupport` test to correctly use simulateUserActivation

Fixes #24196, #24270
r? @jdm @asajeffrey
bors-servo added a commit to servo/servo that referenced this pull request Jan 22, 2020
Support features in webxr

Based on servo/webxr#119

Todo:
 - [x] gate reference space creation on feature presence
 - [x] Fix the `features_deviceSupport` test to correctly use simulateUserActivation

Fixes #24196, #24270
r? @jdm @asajeffrey
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.