Closed Bug 1375816 Opened 7 years ago Closed 7 years ago

displayId for Gamepad of the VRDisplay

Categories

(Core :: WebVR, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: daoshengmu, Assigned: daoshengmu)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(4 files)

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

Created:
Updated:
Size: