Always trigger an input sources change event on session creation #25773

Manishearth · 2020-02-15T09:04:48Z

Fixes our behavior to match the spec, and to specifically do it in a way
that makes sense. This is also what Chromium does currently, though I'm
not sure if it does the scheduling the same way.

The spec for this may change, see immersive-web/webxr#961.

This fix, along with #25770 , makes
three.js content work on servo. Instead of this fix we can also wait for
mrdoob/three.js#18638 to land (which isn't
certain until we figure out more about
immersive-web/webxr#961 )

Even if immersive-web/webxr#961 decides to
choose the option that obviates this patch, we should probably keep it
for now since there's already content out in the wild relying on this
behavior.

r? @jdm @asajeffrey

highfive · 2020-02-15T09:04:54Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/xrsession.rs, components/script/dom/xr.rs, components/script/dom/xrinputsourcearray.rs
@KiChjang: components/script/dom/xrsession.rs, components/script/dom/xr.rs, components/script/dom/xrinputsourcearray.rs

highfive · 2020-02-15T09:04:56Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

jdm · 2020-02-15T11:37:48Z

@bors-servo r+

bors-servo · 2020-02-15T11:37:49Z

📌 Commit 2adab24 has been approved by jdm

bors-servo · 2020-02-15T13:17:16Z

⌛ Testing commit 2adab24 with merge a8fad64...


        Auto merge of #25773 - Manishearth:input-sources-change, r=jdm

@jdm

Always trigger an input sources change event on session creation Fixes our behavior to match the spec, and to specifically do it in a way that makes sense. This is also what Chromium does currently, though I'm not sure if it does the scheduling the same way. The spec for this may change, see immersive-web/webxr#961. This fix, along with #25770 , makes three.js content work on servo. Instead of this fix we can also wait for mrdoob/three.js#18638 to land (which isn't certain until we figure out more about immersive-web/webxr#961 ) Even if immersive-web/webxr#961 decides to choose the option that obviates this patch, we should probably keep it for now since there's already content out in the wild relying on this behavior. r? @jdm @asajeffrey

bors-servo · 2020-02-15T13:50:58Z

💔 Test failed - status-taskcluster

jdm · 2020-02-15T14:01:42Z

  ▶ Unexpected subtest result in /webxr/events_input_source_recreation.https.html:
  │ FAIL [expected PASS] Input sources are re-created when handedness or target ray mode changes
  │   → assert_equals: Added length matches expectations expected 1 but got 0
  │ 
  │ validateAdded/<@https://web-platform.test:8443/webxr/events_input_source_recreation.https.html:81:7
  │ Test.prototype.step@https://web-platform.test:8443/resources/testharness.js:2024:25
  │ validateAdded@https://web-platform.test:8443/webxr/events_input_source_recreation.https.html:79:7
  │ onInputSourcesChange/<@https://web-platform.test:8443/webxr/events_input_source_recreation.https.html:28:9
  │ Test.prototype.step@https://web-platform.test:8443/resources/testharness.js:2024:25
  └ onInputSourcesChange@https://web-platform.test:8443/webxr/events_input_source_recreation.https.html:22:7

Manishearth · 2020-02-15T15:49:31Z

Oh, probably because we needlessly fire an empty inputsourceschange event at the beginning of the session when there aren't any sources


        Always trigger an input sources change event on session creation

Manishearth · 2020-02-15T15:54:22Z

Bingo. Just added an empty-vector early return.

@bors-servo r=jdm

bors-servo · 2020-02-15T15:54:23Z

📌 Commit 09a23b0 has been approved by jdm

bors-servo · 2020-02-15T17:10:49Z

⌛ Testing commit 09a23b0 with merge 075bdcd...


        Auto merge of #25773 - Manishearth:input-sources-change, r=jdm

@jdm

Always trigger an input sources change event on session creation Fixes our behavior to match the spec, and to specifically do it in a way that makes sense. This is also what Chromium does currently, though I'm not sure if it does the scheduling the same way. The spec for this may change, see immersive-web/webxr#961. This fix, along with #25770 , makes three.js content work on servo. Instead of this fix we can also wait for mrdoob/three.js#18638 to land (which isn't certain until we figure out more about immersive-web/webxr#961 ) Even if immersive-web/webxr#961 decides to choose the option that obviates this patch, we should probably keep it for now since there's already content out in the wild relying on this behavior. r? @jdm @asajeffrey

bors-servo · 2020-02-15T17:54:47Z

💔 Test failed - status-taskcluster

Manishearth · 2020-02-16T08:10:52Z

@bors-servo retry

#24622

bors-servo · 2020-02-16T08:10:58Z

⌛ Testing commit 09a23b0 with merge ea7e753...


        Auto merge of #25773 - Manishearth:input-sources-change, r=jdm

@jdm

Always trigger an input sources change event on session creation Fixes our behavior to match the spec, and to specifically do it in a way that makes sense. This is also what Chromium does currently, though I'm not sure if it does the scheduling the same way. The spec for this may change, see immersive-web/webxr#961. This fix, along with #25770 , makes three.js content work on servo. Instead of this fix we can also wait for mrdoob/three.js#18638 to land (which isn't certain until we figure out more about immersive-web/webxr#961 ) Even if immersive-web/webxr#961 decides to choose the option that obviates this patch, we should probably keep it for now since there's already content out in the wild relying on this behavior. r? @jdm @asajeffrey

bors-servo · 2020-02-16T08:30:49Z

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing ea7e753 to master...

highfive assigned jdm Feb 15, 2020

highfive added the S-awaiting-review label Feb 15, 2020

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 15, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 15, 2020

Always trigger an input sources change event on session creation

Loading status checks…

09a23b0

Manishearth force-pushed the Manishearth:input-sources-change branch from 2adab24 to 09a23b0 Feb 15, 2020

highfive added S-awaiting-review and removed S-tests-failed labels Feb 15, 2020

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 15, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 15, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Feb 16, 2020

bors-servo merged commit 09a23b0 into servo:master Feb 16, 2020
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Feb 16, 2020

Manishearth deleted the Manishearth:input-sources-change branch Feb 16, 2020

servo / servo

Always trigger an input sources change event on session creation #25773

Always trigger an input sources change event on session creation #25773

Manishearth commented Feb 15, 2020

highfive commented Feb 15, 2020

highfive commented Feb 15, 2020

jdm commented Feb 15, 2020

bors-servo commented Feb 15, 2020

bors-servo commented Feb 15, 2020

bors-servo commented Feb 15, 2020

jdm commented Feb 15, 2020

Manishearth commented Feb 15, 2020

Manishearth commented Feb 15, 2020

bors-servo commented Feb 15, 2020

bors-servo commented Feb 15, 2020

bors-servo commented Feb 15, 2020

Manishearth commented Feb 16, 2020

bors-servo commented Feb 16, 2020

bors-servo commented Feb 16, 2020

servo / servo

Join GitHub today

Always trigger an input sources change event on session creation #25773

Always trigger an input sources change event on session creation #25773

Conversation

Manishearth commented Feb 15, 2020

highfive commented Feb 15, 2020

highfive commented Feb 15, 2020

jdm commented Feb 15, 2020

bors-servo commented Feb 15, 2020

bors-servo commented Feb 15, 2020

bors-servo commented Feb 15, 2020

jdm commented Feb 15, 2020

Manishearth commented Feb 15, 2020

Manishearth commented Feb 15, 2020

bors-servo commented Feb 15, 2020

bors-servo commented Feb 15, 2020

bors-servo commented Feb 15, 2020

Manishearth commented Feb 16, 2020

bors-servo commented Feb 16, 2020

bors-servo commented Feb 16, 2020