Closed Bug 1036604 Opened 10 years ago Closed 10 years ago

Add VRDevice interface and getVRDevices

Categories

(Core :: DOM: Device Interfaces, defect)

x86
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: vlad, Assigned: vlad)

References

Details

(Keywords: dev-doc-needed)

Attachments

(5 files, 3 obsolete files)

57.19 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
29.40 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
31.30 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
5.26 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.92 KB, patch
roc
: review+
Details | Diff | Splinter Review
This adds the core interfaces that are currently in use by the VR prototype:  VRDevice, HMDVRDevice, PositionalSensorVRDevice.  It also adds navigator.getVRDevices for obtaining a list of current VR devices, as well as implementing support for the Oculus Rift set of devices in this framework.
Attachment #8453281 - Flags: feedback?(bzbarsky)
This is the machinery for creating the proper VR-setting display item when an element has the VR property set.  That property can be set using the testing xxx method, or (the real way) via requestFullScreen with a vrDevice option (in another bug).
Attachment #8453289 - Flags: feedback?(roc)
Component: DOM → DOM: Device Interfaces
Whoops, attached the wrong patch.  Here's the right one.
Attachment #8453281 - Attachment is obsolete: true
Attachment #8453281 - Flags: feedback?(bzbarsky)
Attachment #8453308 - Flags: feedback?(bzbarsky)
Attachment #8453308 - Flags: feedback?(bugs)
I think you need to cycle collect VRDeviceService and add traverse/unlink of nsGlobalWindow::mVRDeviceService or you'll get garbage cycles through JS -> nsGlobalWindow -> VRDeviceService -> VRDevice -> JS.
The test case would be setting an expando on a VR device that is in the window to something that points back to the window, like a node or whatever.
Comment on attachment 8453308 [details] [diff] [review]
1 - Add VRDevice interface and navigator.getVRDevices, and Rift support (v2)

>+ReleaseHMDInfoRef(void *, nsIAtom*, void *aPropertyValue, void *)
Nit, * goes with the type. So void*
Same also elsewhere

>+    if (left.IsZero())
>+      left = mHMD->GetRecommendedEyeFOV(vr::HMDInfo::Eye_Left);
>+    if (right.IsZero())
>+      right = mHMD->GetRecommendedEyeFOV(vr::HMDInfo::Eye_Right);
Nit, always {} with 'if'


>+  virtual void GetEyeTranslation(VREye aEye, VRPoint3D& aTranslationOut) {
Nit, { goes to its own line

>+class VRDeviceService : public nsIObserver
>+{
...
>+  // Create a service, generally one per window
>+  static already_AddRefed<VRDeviceService> Create();
>+
>+  // The Navigator interface callback
>+  void GetVRDevices(Promise* aPromise);
>+
>+  void BeginShutdown();
>+
>+  bool Initialize();
>+
>+protected:
>+  VRDeviceService();
>+  virtual ~VRDeviceService();
>+
>+  bool mInitialized;
>+  nsTArray<nsRefPtr<VRDevice> > mKnownDevices;
I don't understand this setup.
We have a global VRDeviceService object, and then mKnownDevices, which are exposed to some page.
What if some other page tries to use this API? It gets the same mKnownDevices but those have already
wrapper for the first page, so the second page will just get some security exception or such?
I think you may need to have internal VRDevice thing, and then per-DOM-Window VRDevice which is the
one inheriting nsWrapperCache.
Attachment #8453308 - Flags: feedback?(bugs)
Comment on attachment 8453289 [details] [diff] [review]
2 - Layout support for VR; nsDisplayVR item when VR property is set on element

Review of attachment 8453289 [details] [diff] [review]:
-----------------------------------------------------------------

This is basically what I had in mind, yes.

::: layout/generic/nsFrame.cpp
@@ +96,5 @@
>  using namespace mozilla::layout;
>  
> +namespace mozilla {
> +namespace gfx {
> +namespace vr {

oh please don't introduce another namespace

@@ +2317,5 @@
>      // but the spec says it acts like the rest of these
>      || disp->mChildPerspective.GetUnit() == eStyleUnit_Coord
>      || disp->mMixBlendMode != NS_STYLE_BLEND_NORMAL
> +    || nsSVGIntegrationUtils::UsingEffectsForFrame(child)
> +    || IsContentWithVRFlag(child);

This property lookup isn't as cheap as I'd like but I guess we can add a flag on the element or frame as an optimization.
Attachment #8453289 - Flags: feedback?(roc) → feedback+
Bas: this is what it looks like when things go fullscreen, fwiw: http://i.imgur.com/3QS7tXY.jpg
Comment on attachment 8453308 [details] [diff] [review]
1 - Add VRDevice interface and navigator.getVRDevices, and Rift support (v2)

>+++ b/dom/vr/VRDevice.cpp
>+  RefPtr<vr::HMDInfo> hmdRef = mHMD;

Hmm.  Not nsRefPtr?

>+++ b/dom/vr/VRDeviceService.cpp
>+CopyFieldOfView(const gfx::vr::FieldOfView& aSrc, VRFieldOfView& aDest)

Could we just use dom::VRFieldOfView instead of gfx::vr::FieldOfView?  Seems annoying to duplicate the structs.

>+class HMDInfoVRDevice : public HMDVRDevice
>+  virtual void SetFieldOfView(const VRFieldOfView& aLeftFOV,
>+                              const VRFieldOfView& aRightFOV)

MOZ_OVERRIDE?

Exposing the hardware info like we do here is a bit worrying for fingerprinting and stuff.  :(

>+VRDeviceService::VRDeviceService()
>+  observerService->AddObserver(this,
>+                               NS_XPCOM_WILL_SHUTDOWN_OBSERVER_ID,
>+                               false);

So this will get called once per window that ends up using the VR stuff.  But this call will make the observer service hold a ref to this object until it's removed... which is at XPCOM shutdown.  So basically, this leaks the object for the lifetime of the app, except frees it right before shutdown so the leak detector doesn't notice.

>+++ b/dom/webidl/VRDevice.webidl
>+dictionary VRPoint3D {
>+dictionary VRPoint4D {

Is there a reason to not use DOMPoint here?

>+  VRPoint3D linearVelocity;
>+  VRPoint3D linearAcceleration;

I agree that using DOMPoint for velocity/acceleration is a bit odd, but not more so than using VRPoint3D.  ;)

>+interface VRDevice {
>+  [Pure, Cached] readonly attribute DOMString hardwareUnitId;
>+  [Pure, Cached] readonly attribute DOMString deviceId;
>+  [Pure, Cached] readonly attribute DOMString deviceName;

Do these need to be cached?  I expect it's not needed for correctness, since we can get the string anyway and JS strings can't be told apart, so we shouldn't bother; these calls are not likely to be super-hot, right?

I didn't really look at the gfx/ bits.
Attachment #8453308 - Flags: feedback?(bzbarsky) → feedback+
Ok! Finally an updated patch to part 1.  Any suggestions on who can/should review this?  bz, I put you down by default, but please feel free to suggest an alternate reviewer/reviewers and I'll pass it along.  This fixes a bunch of the above comments:

MOZ_OVERRIDE should be in all the right places; also no more RefPtr<>; moz prefix is gone everywhere; vr namespace is gone.

I think I have the cycle collection right.  VRDeviceService is also no longer a shutdown observer.  "Service" is a bit of a misnomer; there's one per window, not global, and it's tied to the lifetime of the window.  It's really there to just have a place to initialize and hold on to the VR device objects that are visible to that window.  With some extra work it could even go away, with the window holding a nsTArray of VRDevice, much like VRDeviceService itself does now.  If that's better, I can do that.

bz, re fingerprinting: I -think- the only fingerprint we'd expose is the presence of VR devices and their number.  The actual IDs and the like we could change on every browser startup, or even per-page.  I thought about keeping some of it consistent so that pages could recognize "hey I know this device, I have a saved configuration for it", but I don't think that's necessary.  Right now the IDs are already per-window, they're just not different because I just use the address of the relevant pointer as the ID as a temporary hack.

... but doesn't fix all the issues; specifically:

1 - style nits, will fix these next round; didn't want to do additional rebases while testing

2 - types. VRPoint3D, VRPoint4D:

I just noticed that DOMPoint was 4-component.  Okay, so that's possible.  But it's also an interface instead of a dictionary, which I don't really understand.  Is that so it can have constructors?  Does that affect performance in any meaningful way?  It also seems to depend on layout.css.DOMPoint.enabled, I guess I can force that on if vr is enabled?

3 - dom::VRFieldOfView vs gfx::VRFieldOfView

The only reason I have these as separate types is so that gfx::VRFieldOfView can use a more natural struct style in gfx, that matches the rest.  Otherwise it would be the only one using mUpDegrees vs. upDegrees in that code.  That's really the only reason I can thnk of though, so I'm happy to switch it to using dom::VRFieldOfView at the risk of a mild papercut there.

4 - (not in code right now, would be a separate patch) does a DOMQuaternion class make sense to add, along with associated additional methods on DOMMatrix?  It would make code on the JS side simpler.  Likewise adding quaternion3d() as a CSS transform, to avoid having to go from quat -> matrix.

5 - stop using the address of the pointer as a device ID, and just generate a proper UUID
Attachment #8453308 - Attachment is obsolete: true
Attachment #8499111 - Flags: review?(bzbarsky)
You also need a
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mVRDeviceService)
in NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsGlobalWindow)

Also, you should be able to replace these three things:
  NS_IMPL_CYCLE_COLLECTION_CLASS(VRDeviceService)
  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(VRDeviceService)
  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(VRDeviceService)
with just:
  NS_IMPL_CYCLE_COLLECTION(VRDeviceService, mKnownDevices)

There's a bunch of template stuff that should be able to figure out how to deal with nsTArray<nsRefPtr<>>.

The CC stuff looks good to me otherwise.
(In reply to Andrew McCreight [:mccr8] from comment #11)
> [..]
> 
> There's a bunch of template stuff that should be able to figure out how to
> deal with nsTArray<nsRefPtr<>>.
> 
> The CC stuff looks good to me otherwise.

Thanks for catching that!  Just fixed in my tree; template stuff handled nsTArray just fine.
I'm going to try to get to this tomorrow; failing that I might look for someone else to foist off on.
Comment on attachment 8499111 [details] [diff] [review]
1 - Add VRDevice interface and navigator.getVRDevices, and Rift support (v3)

>+Navigator::GetVRDevices(ErrorResult& aRv)
>+  nsRefPtr<Promise> p = Promise::Create(go, aRv);

Need to return if !p here, right?

>+++ b/dom/base/Navigator.h
>+class VRDeviceListCallback;

Not used.

>+++ b/dom/base/nsGlobalWindow.cpp
>+#include "mozilla/dom/VRDeviceService.h"

This isn't actually a service (in the sense of singletons, etc), right?

Can we come up with a better name for it?

>+mozilla::dom::VRDeviceService*

This whole file is using mozilla::dom.  No need to explicitly qualify it here.

>+++ b/dom/bindings/Bindings.conf

I'd somewhat prefer we used HeaderFile annotations in the IDL instead.  We still support them in the .conf only for worker stuff, really.

Also, these header file names should start with mozilla/dom, right?  That's where they're being exported to.

>+++ b/dom/vr/VRDevice.cpp
>+#include "VRDevice.h"

mozilla/dom/VRDevice.h

>+HMDVRDevice::XxxToggleElementVR(Element& aElement)

The property will live across adopts to a new document, right?  Is this desirable in this case?

>+++ b/dom/vr/VRDevice.h
>+  enum VRDeviceType {
>+    HMD,
>+    PositionSensor
>+  };

Does this mean there are never actual leaf VRDevice instances around?

In that case, you can annotate it as "concrete":False in Bindings.conf and remove its WrapObject implementation so people can't instantiate it.

>+++ b/dom/vr/VRDeviceService.cpp
>+#include "VRDeviceService.h"
>+#include "VRDevice.h"

mozilla/dom on both of those.

>+using namespace mozilla;

Seems pointless, since everything in this file is inside namespace mozilla.

>+    : HMDVRDevice(nullptr, aHMD)

Are there plans to pass an actually useful owner there?

>+class HMDInfoVRDevice : public HMDVRDevice

Can this be in the anonymous namespace?  If not, why not?

>+  virtual void SetFieldOfView(const dom::VRFieldOfView& aLeftFOV,

Worth documenting the defaulting behavior here (falling back to the recommended FOV) in the IDL.

>+  virtual void GetEyeTranslation(VREye aEye, VRPoint3D& aTranslationOut) MOZ_OVERRIDE {

Curly on next line please, and same for the following methods.

>+class HMDPositionVRDevice : public PositionSensorVRDevice

Again, can this be in the anonymous namespace?

>+    : PositionSensorVRDevice(nullptr)

Again, plans to do something better?

The obvious thing would be to have Create() take the pointer to the window, and pass it to Initialize(), right?

I didn't carefully review the interactions with mHMD and mFOVPort.  Let me know if those need review as well.

>+  virtual void GetState(double timeOffset, VRPositionState& aOut) MOZ_OVERRIDE {

Curly on next line.

So this looks like it will leave the position or orientation bits zeroed out if the state.flags doesn't have that info?

Would it not be better to leave them not defined entirely?  Otherwise, how do you tell apart "no position state" and "just sitting at the origin"?

>+  for (uint32_t i = 0; i < tmp->mKnownDevices.Length(); i++) {
>+    ImplCycleCollectionUnlink(tmp->mKnownDevices[i]);
>+  }

This seems like a long-winded way of writing:

  NS_IMPL_CYCLE_COLLECTION_UNLINK(mKnownDevices);

>+  for (uint32_t i = 0; i < tmp->mKnownDevices.Length(); i++) {
>+    ImplCycleCollectionTraverse(cb, tmp->mKnownDevices[i], "Known VR Device");
>+  }

  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mKnownDevices);

Or even just nix all this unlink/traverse/class stuff and do:

  NS_IMPL_CYCLE_COLLECTION(VRDeviceService, mKnownDevices)

>+++ b/dom/vr/VRDeviceService.h
>+#include "VRDevice.h"

mozilla/dom

>+#include "nsGlobalWindow.h"

Unused.

>+class nsIDOMDocument;

Unused, and same for the #include of the same in the .cpp.

>+  nsTArray<nsRefPtr<VRDevice> > mKnownDevices;

Please drop the extra ' ' between the '>' and '>'.

>+++ b/dom/webidl/VRDevice.webidl

>+dictionary VRPoint4D {

Any reason this can't inherit from VRPoint3D and then just add "w"?

>+// We have DOMRect, but it's an interface?

Yes, because it exposes the same information in several different ways, and in some cases is a live object.

There's a good amount of disagreement whether situations like yours should involve neste dictionaries of the sort you have or actual interface objects with prototypes and whatnot.  This is OK for experimenting with, but before we ship this for real we'd want to have that conversation.

I assume that we do in fact guarantee what all of these doubles are finite and not NaN?  I guess for return values it doesn't matter too much in practice.  Though would NaN make more sense than 0 for the "missing position" or "missing orientation" cases of VRPositionState?

>+  [Pure, Cached] readonly attribute DOMString hardwareUnitId;

Why does this need to be cached?  I don't think it does.

Same for your other string attributes.

I assume that we just make up this identifier, so we don't have to expose the exact type of hardware involved, necessarily?

>+  [Pure, Cached] readonly attribute DOMString deviceId;

The comment on this makes this sound great for tracking.  Again, before we enable this for real we'd want to figure out how this work (e.g. maybe it should get reset every time the user clears cookies or something?).  I'd like us to make sure we have a bug tracking this and blocking enabling by default.

>+  VRPositionState getState(optional double timeOffset = 0.0);

Could use some documentation.

r=me with nits picked.  I still didn't read the gfx/ bits because I assume someone else is... if this assumption is false, please tell me!

On to comment 10:

> But it's also an interface instead of a dictionary, which I don't really
> understand.  Is that so it can have constructors? 

And so pages have a meaningful way to ask "is this a Point" (using instanceof) and so forth.

I'm pretty partial, myself, to the view Allen expresses in <http://lists.w3.org/Archives/Public/public-script-coord/2014JanMar/0201.html>.

> Does that affect performance in any meaningful way?

It can.  .x on DOMPoint in hot code (which is where you'd really care) is about 20 CPU instructions, whereas the same thing for a plain object is probably closer to 2-3 CPU instructions.

If this actually becomes an issue, we can make things faster here (e.g. by making the members of DOMPointReadOnly StoreInSlot and adding support for that on non-leaf classes.  In practice, 20 CPU instructions is not exactly super-slow.

> It also seems to depend on layout.css.DOMPoint.enabled, I guess I can force
> that on if vr is enabled?

If we wanted to use DOMPoint here, I think that would be fine, yes.

> does a DOMQuaternion class make sense to add, along with associated
> additional methods on DOMMatrix?

Imo, yes.
Attachment #8499111 - Flags: review?(bzbarsky) → review+
Awesome, thanks for the great review!  I've got most of the comments addressed, and I'm currently working on converting things to use DOMPoint/DOMRect and turning VRPositionState and VRFieldOfView into interfaces.  VRFieldOfView is straightforward, since it's similar to DOMPoint etc.  It's also not going to be on any hot path.  But the state block is more complicated; this is what I have now:

[Pref="dom.vr.enabled"]
interface VRPositionState {
  readonly attribute double timeStamp;

  readonly attribute boolean hasPosition;
  readonly attribute DOMPoint? position;
  readonly attribute DOMPoint? linearVelocity;
  readonly attribute DOMPoint? linearAcceleration;

  readonly attribute boolean hasOrientation;
  // XXX should be DOMQuaternion as soon as we add that
  readonly attribute DOMPoint? orientation;
  readonly attribute DOMPoint? angularVelocity;
  readonly attribute DOMPoint? angularAcceleration;
};

While this would be accurate, it would mean needing to construct up to 7 objects (the state object itself + 6 DOMPoints) every time getState is called.  This isn't a disaster, especially because in the very near term I expect to have a "currentFrameState" attribute that has a cached one of these that represents the state for the current frame's time, but it does seem pretty heavy for something that's generally pure data.  Does the above look right?
>  readonly attribute double timeStamp;

By the way, what are the units and zero time on this?  Seems like this should either be a DOMTimeStamp (integer ms since unix epoch) or DOMHighResTimeStamp (double ms since either document load start or unix epoch, depending on how the interface is defined).

> It would mean needing to construct up to 7 objects (the state object itself + 6
> DOMPoints) every time getState is called.

Well, to be fair the dictionary solution constructed those 7 JSObjects for you in the binding layer, right?

You're right that you now have to construct the C++ objects, but you can do that lazily if you want (whereas the dictionary approach always did all the construction eagerly).

Past that, the above looks right assuming null for the nullable things means "not available".
(In reply to Boris Zbarsky [:bz] from comment #16)
> >  readonly attribute double timeStamp;
> 
> By the way, what are the units and zero time on this?  Seems like this
> should either be a DOMTimeStamp (integer ms since unix epoch) or
> DOMHighResTimeStamp (double ms since either document load start or unix
> epoch, depending on how the interface is defined).

It should be a DOMHighResTimeStamp (to match rAF frame timestamp, performance.now, etc.).  I'll do that even if it's not going to be 100% accurate initially as I need to figure out how to sync up the Oculus time base to what we use.

> > It would mean needing to construct up to 7 objects (the state object itself + 6
> > DOMPoints) every time getState is called.
> 
> Well, to be fair the dictionary solution constructed those 7 JSObjects for
> you in the binding layer, right?

Good point.

> You're right that you now have to construct the C++ objects, but you can do
> that lazily if you want (whereas the dictionary approach always did all the
> construction eagerly).
> 
> Past that, the above looks right assuming null for the nullable things means
> "not available".

Yep (along with hasFoo = false).  My current code was eagerly constructing everything, but it definitely makes sense to lazily construct the velocity/acceleration values.
I think I've got all the review comments addressed in this one; there were enough changes that another pass would be useful.

- DOMPoint/DOMRect are now used.  I didn't add a DOMQuaternion yet, that can happen as a followup (the orientation value is reported as a DOMPoint right now).

- VRDeviceService is gone.  It served no real purpose anyway other than just a holder, and nsGlobalWindow can be that holder just as well.

- VRFieldOfView/VRPositionState are now interfaces.  VRPositionState has hasPosition/hasOrientation booleans.

- I just noticed that I didn't change timeStamp to a DOMHighResTimestamp.  Will do that in a quick followup.
Attachment #8504869 - Flags: review?(bzbarsky)
This patch just add gfxVR.cpp/.h and the oculus API dynamic shims.  They're used by part 1 of the patch, but nothing is hooked in to actual gfx rendering here.
Attachment #8504871 - Flags: review?(jmuizelaar)
Comment on attachment 8504871 [details] [diff] [review]
1.5 - gfxVR stuff

Review of attachment 8504871 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxVR.cpp
@@ +430,5 @@
> +
> +    result.position[0] = pose.ThePose.Position.x;
> +    result.position[1] = pose.ThePose.Position.y;
> +    result.position[2] = pose.ThePose.Position.z;
> +    

There's some extraneous white space in this patch.
Attachment #8504871 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8504869 [details] [diff] [review]
1 - Add VRDevice interface and navigator.getVRDevices, and Rift support (v4)

Sorry for the lag here; at a workweek this week.  :(

>+++ b/dom/base/nsGlobalWindow.cpp
>+nsGlobalWindow::GetVRDevices(nsTArray<nsRefPtr<mozilla::dom::VRDevice>>& aDevices)

Still don't need the "mozilla::dom::" prefix in this .cpp file.  Just nsRefPtr<VRDevice> should work.

>+    bool ok = mozilla::dom::VRDevice::CreateAllKnownVRDevices(ToSupports(this), mVRDevices);

Likewise.

>+++ b/dom/base/nsGlobalWindow.h
>+  bool                                       mVRDevicesInitialized;

Might be nice to group it with other booleans if we have them so we won't waste the space after it due to alignment.  Then again, it's nice to keep the related members together, so I don't feel super-strongly about this.

>+++ b/dom/vr/VRDevice.cpp
>+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(VRFieldOfView, mParent)

This is fine as is, but if we want to we could probably remove the cycle collection, remove the wrappercache, mark this interface and VRPositionState as not wrappercached in bindings.conf, and annotate the methods that return them with [NewObject].  That's assuming all methods returning them always create new objects, which I _think_ is true.  For now.  If that changes, we'd need the wrappercache bits, which is why this might not be worth doing.  Or doing in a followup once we're more sure about the final state of the APIs.

In any case, annotating that bits that are [NewObject] in the IDL is worth it anyway.

>+  SetIsDOMBinding();

No longer exists.  There are only 4 classes in the tree that are wrappercache 
but not DOM bindings, so we switched this to be an opt-out, not an opt-in.  Just remove this line from your constructors.

>+VRPositionState::GetLinearVelocity()

Is this a sensical thing to do if |aState.flags & gfx::VRHMDInfo::State_Position| is 0?  If not, maybe we should return null here in that situation?

>+VRPositionState::GetLinearAcceleration()

Similar.  And similar for the angular cases.

r=me
Attachment #8504869 - Flags: review?(bzbarsky) → review+
Comment on attachment 8513643 [details] [diff] [review]
2 - Layout support for VR & nsDisplayVR item

Review of attachment 8513643 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +1635,5 @@
>  
> +inline static bool IsContentWithVRFlag(const nsIFrame *aFrame)
> +{
> +  nsIContent *content = aFrame->GetContent();
> +  return content && content->GetProperty(nsGkAtoms::vr_state) != nullptr;

It's a bit unfortunate to put this hashtable lookup on a very hot path :-(.

I think the best thing to do is check this in nsFrame::Init and set a frame state bit when this is present. See nsFrameStateBits, there are several free at the moment. Then, when you set or clear this property, call RestyleManager::PostRestyleEvent to restyle it with eRestyle_Self and nsChangeHint_ReconstructFrame.
Attachment #8513643 - Flags: review?(roc) → review+
roc, like this?  I'm not doing PostRestyleEvent because the property is only set/deleted right before we transition in or out of fullscreen -- I'm assuming that forces all sorts of things to get recreated (and it seems to work fine).  Is that a reasonable assumption for now?
Attachment #8514327 - Flags: review?(roc)
Comment on attachment 8514327 [details] [diff] [review]
2.1 - Use frame state bit for vr

Review of attachment 8514327 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +1987,5 @@
>    nsDisplayListBuilder::AutoBuildingDisplayList
>      buildingDisplayList(aBuilder, this, dirtyRect, true);
>  
> +  mozilla::gfx::VRHMDInfo* vrHMDInfo = nullptr;
> +  if (mContent && GetStateBits() & NS_FRAME_HAS_VR_CONTENT) {

() around & subexpression. Don't need to check mContent here, since if it was null the state bit wouldn't be set.
Attachment #8514327 - Flags: review?(roc) → review+
You need to log in before you can comment on or make changes to this bug.