Closed Bug 1375816 Opened 7 years ago Closed 7 years ago

displayId for Gamepad of the VRDisplay

Categories

(Core :: WebVR, enhancement)

Product:
Core
Component:
WebVR
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: daoshengmu, Assigned: daoshengmu)

References

(
URL
)

Details

(Keywords: dev-doc-complete)

Attachments

(4 files)

Bug 1375816 - Part 1: Add displayId attribute in Gamepad;
59 bytes, text/x-review-board-request
qdot
: review+
jcristau
: approval-mozilla-beta-
Details
Bug 1375816 - Part 2: DisplayId support in GamepadManager;
59 bytes, text/x-review-board-request
cleu
: review+
jcristau
: approval-mozilla-beta-
Details
Bug 1375816 - Part 3: VRController displayId attribute support;
59 bytes, text/x-review-board-request
kip
: review+
jcristau
: approval-mozilla-beta-
Details
Bug 1375816 - Part 4: VRController displayId attribute testcase;
59 bytes, text/x-review-board-request
kip
: review+
jcristau
: approval-mozilla-beta-
Details
displayId Return the displayId of the VRDisplay this Gamepad is associated with. https://w3c.github.io/webvr/spec/1.1/#interface-gamepad
Assignee: nobody → dmu
Assignee: dmu → nobody
Assignee: nobody → dmu
Comment on attachment 8886525 [details] Bug 1375816 - Part 2: DisplayId support in GamepadManager; https://reviewboard.mozilla.org/r/157332/#review162472
Attachment #8886525 - Flags: review?(cleu) → review+
Comment on attachment 8886524 [details] Bug 1375816 - Part 1: Add displayId attribute in Gamepad; https://reviewboard.mozilla.org/r/157330/#review162632 We're gonna look back at this in a few years and wonder why it's named gamepad. :) (everything looks fine, just find it funny how much is getting bolted on to this)
Attachment #8886524 - Flags: review?(kyle) → review+
Comment on attachment 8886526 [details] Bug 1375816 - Part 3: VRController displayId attribute support; https://reviewboard.mozilla.org/r/157334/#review162654 LGTM, Thanks!
Attachment #8886526 - Flags: review?(kgilbert) → review+
Comment on attachment 8886527 [details] Bug 1375816 - Part 4: VRController displayId attribute testcase; https://reviewboard.mozilla.org/r/157336/#review162656 LGTM, Thanks!
Attachment #8886527 - Flags: review?(kgilbert) → review+
(In reply to Kyle Machulis [:qdot] [:kmachulis] (if a patch has no decent commit message, automatic r-) from comment #10) > Comment on attachment 8886524 [details] > Bug 1375816 - Part 1: Add displayId attribute in Gamepad; > > https://reviewboard.mozilla.org/r/157330/#review162632 > > We're gonna look back at this in a few years and wonder why it's named > gamepad. :) > > (everything looks fine, just find it funny how much is getting bolted on to > this) I agree... We give some strange functions to Gamepad.
(In reply to Daosheng Mu[:daoshengmu] from comment #14) > Comment on attachment 8886527 [details] > Bug 1375816 - Part 4: VRController displayId attribute testcase; > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/157336/diff/2-3/ We seem to have unstable event system for listening 'gamepadconnected' event at our tests. Most of them are non-e10s and Android, they are not the platforms that we mainly focus on, and I have confirmed the displayId attribute works well. So, I am planning to skip these specific platform. kip, could you help me review interdiff 2-3 about the skip-if for some platform timed-out problem before I land? Try looks great currently, https://treeherder.mozilla.org/#/jobs?repo=try&revision=09ed17fc2a6560fdc31b8b6dbb1a6db80b18c2fb
Flags: needinfo?(kgilbert)
(In reply to Daosheng Mu[:daoshengmu] from comment #15) > (In reply to Daosheng Mu[:daoshengmu] from comment #14) > > Comment on attachment 8886527 [details] > > Bug 1375816 - Part 4: VRController displayId attribute testcase; > > > > Review request updated; see interdiff: > > https://reviewboard.mozilla.org/r/157336/diff/2-3/ > > We seem to have unstable event system for listening 'gamepadconnected' event > at our tests. Most of them are non-e10s and Android, they are not the > platforms that we mainly focus on, and I have confirmed the displayId > attribute works well. So, I am planning to skip these specific platform. > > kip, could you help me review interdiff 2-3 about the skip-if for some > platform timed-out problem before I land? > > Try looks great currently, > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=09ed17fc2a6560fdc31b8b6dbb1a6db80b18c2fb Interdiff looks good, thanks Daosheng! When we are ready to turn on Mac and Linux support for WebVR by default, we should review the tests and ensure they are running on at least the supported configurations.
Flags: needinfo?(kgilbert)
Pushed by dmu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e15e060f034c Part 1: Add displayId attribute in Gamepad; r=qdot https://hg.mozilla.org/integration/autoland/rev/fd4517198d6c Part 2: DisplayId support in GamepadManager; r=Lenzak https://hg.mozilla.org/integration/autoland/rev/dd09fc501f90 Part 3: VRController displayId attribute support; r=kip https://hg.mozilla.org/integration/autoland/rev/73619b7ce23d Part 4: VRController displayId attribute testcase; r=kip
(In reply to Daosheng Mu[:daoshengmu] from comment #21) > Comment on attachment 8886527 [details] > Bug 1375816 - Part 4: VRController displayId attribute testcase; > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/157336/diff/3-4/ Fix TEST-UNEXPECTED-PASS Gamepad interface: attribute displayId expected FAIL at /webvr/idlharness.html because we have already implemented it. https://treeherder.mozilla.org/#/jobs?repo=try&revision=afb9e0c21e85d783a53fc1cce9de87362d4c76ea
Flags: needinfo?(dmu)
Pushed by dmu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bec6c4becfd9 Part 1: Add displayId attribute in Gamepad; r=qdot https://hg.mozilla.org/integration/autoland/rev/ab013a0a0ae5 Part 2: DisplayId support in GamepadManager; r=Lenzak https://hg.mozilla.org/integration/autoland/rev/13155b1e7a48 Part 3: VRController displayId attribute support; r=kip https://hg.mozilla.org/integration/autoland/rev/498baf1613db Part 4: VRController displayId attribute testcase; r=kip
Backout by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/f2e76538f49c Backed out changeset 498baf1613db https://hg.mozilla.org/integration/autoland/rev/96e75102b3d8 Backed out changeset 13155b1e7a48 https://hg.mozilla.org/integration/autoland/rev/7a60993c3958 Backed out changeset ab013a0a0ae5 https://hg.mozilla.org/integration/autoland/rev/5654a6049997 Backed out changeset bec6c4becfd9 for bustage in gfxVROculus.cpp on Windows. r=backout
Backed out for bustage in gfxVROculus.cpp on Windows: https://hg.mozilla.org/integration/autoland/rev/5654a60499973490633adb7eb715467a99894622 https://hg.mozilla.org/integration/autoland/rev/7a60993c3958f105960d9f0a63bc05b496745f90 https://hg.mozilla.org/integration/autoland/rev/96e75102b3d8ef5396ba0e8a3dec267edb43cd01 https://hg.mozilla.org/integration/autoland/rev/f2e76538f49cdb8c599f8468f5a0c47603175f85 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=498baf1613db97a5aadab1e9697a5b92e71df0ed&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=115146659&repo=autoland 01:31:52 INFO - gfxVROculus.cpp 01:31:52 INFO - c:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/gfx/vr/gfxVROculus.cpp(1798): error C2065: 'mHMDInfo': undeclared identifier 01:31:52 INFO - c:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/gfx/vr/gfxVROculus.cpp(1798): error C2227: left of '->GetDisplayInfo' must point to class/struct/union/generic type 01:31:52 INFO - c:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/gfx/vr/gfxVROculus.cpp(1798): note: type is 'unknown-type' 01:31:52 INFO - c:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/gfx/vr/gfxVROculus.cpp(1798): error C2228: left of '.GetDisplayID' must have class/struct/union 01:31:52 INFO - c:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/gfx/vr/gfxVROculus.cpp(1798): error C2664: 'mozilla::gfx::impl::VRControllerOculus::VRControllerOculus(mozilla::gfx::impl::VRControllerOculus &)': cannot convert argument 1 from 'mozilla::dom::GamepadHand' to 'mozilla::gfx::impl::VRControllerOculus &' 01:31:52 INFO - c:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/config/rules.mk:1051: recipe for target 'gfxVROculus.obj' failed 01:31:52 INFO - mozmake.EXE[5]: *** [gfxVROculus.obj] Error 2
Flags: needinfo?(dmu)
Furthermore frequently this: TEST-UNEXPECTED-FAIL | dom/vr/test/mochitest/test_vrController_displayId.html | Test timed out. - Test timed out. https://treeherder.mozilla.org/logviewer.html#?job_id=115170720&repo=autoland
Pushed by dmu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7e194151d369 Part 1: Add displayId attribute in Gamepad; r=qdot https://hg.mozilla.org/integration/autoland/rev/a77053bac9a9 Part 2: DisplayId support in GamepadManager; r=Lenzak https://hg.mozilla.org/integration/autoland/rev/8bebd50ee61b Part 3: VRController displayId attribute support; r=kip https://hg.mozilla.org/integration/autoland/rev/61d6383a750a Part 4: VRController displayId attribute testcase; r=kip
(In reply to Daosheng Mu[:daoshengmu] from comment #29) > Comment on attachment 8886526 [details] > Bug 1375816 - Part 3: VRController displayId attribute support; > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/157334/diff/2-3/ Rebase with m-c and confirm it can be built successfully in Windows.
Flags: needinfo?(dmu)
We're sorry - something has gone wrong while rewriting or rebasing your commits. The commits being pushed no longer match what was requested. Please file a bug.
Comment on attachment 8886524 [details] Bug 1375816 - Part 1: Add displayId attribute in Gamepad; Approval Request Comment [Feature/Bug causing the regression]: nope. New feature [User impact if declined]: We will miss displayID in WebVR API [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: nope [List of other uplifts needed for the feature/fix]: [Is the change risky?]: nope [Why is the change risky/not risky?]: This is a new attribute for Gamepad, it will affect our functions. [String changes made/needed]: nope.
Attachment #8886524 - Flags: approval-mozilla-beta?
Comment on attachment 8886525 [details] Bug 1375816 - Part 2: DisplayId support in GamepadManager; Approval Request Comment [Feature/Bug causing the regression]: nope. New feature [User impact if declined]: We will miss displayID in WebVR API [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: nope [List of other uplifts needed for the feature/fix]: [Is the change risky?]: nope [Why is the change risky/not risky?]: This is a new attribute for Gamepad, it will affect our functions. [String changes made/needed]: nope.
Attachment #8886525 - Flags: approval-mozilla-beta?
Comment on attachment 8886526 [details] Bug 1375816 - Part 3: VRController displayId attribute support; Approval Request Comment [Feature/Bug causing the regression]: nope. New feature [User impact if declined]: We will miss displayID in WebVR API [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: nope [List of other uplifts needed for the feature/fix]: [Is the change risky?]: nope [Why is the change risky/not risky?]: This is a new attribute for Gamepad, it will affect our functions. [String changes made/needed]: nope.
Attachment #8886526 - Flags: approval-mozilla-beta?
Comment on attachment 8886527 [details] Bug 1375816 - Part 4: VRController displayId attribute testcase; Approval Request Comment [Feature/Bug causing the regression]: nope. New feature [User impact if declined]: We will miss displayID in WebVR API [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: nope [List of other uplifts needed for the feature/fix]: [Is the change risky?]: nope [Why is the change risky/not risky?]: This is a new attribute for Gamepad, it will affect our functions. [String changes made/needed]: nope.
Attachment #8886527 - Flags: approval-mozilla-beta?
Because WebVR will be enabled in FF 55 and displayId is a part of them. I hope this displayId API can be uplifted to FF 55 as well.
Comment on attachment 8886524 [details] Bug 1375816 - Part 1: Add displayId attribute in Gamepad; We're building the last beta for 55 today, it's too late for new feature work.
Attachment #8886524 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8886525 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8886526 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8886527 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
This is not a blocker for 55 release and should be okay to ride with 56 instead.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: