Closed Bug 1036606 Opened 6 years ago Closed 6 years ago

Add options dict and vrDevice to mozRequestFullScreen

Categories

(Core :: DOM: Core & HTML, 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

(1 file, 2 obsolete files)

mozRequestFullScreen currently takes no arguments.  This adds an optional dictionary argument for options; it defines a "vrDevice" option that can accept a HMDVRDevice to indicate a) what display the window should be made full screen on; b) that VR rendering/postprocessing is to be done.
Attachment #8453295 - Flags: feedback?(bzbarsky)
Comment on attachment 8453295 [details] [diff] [review]
Add options dict arg to mozRequestFullScreen

>+++ b/content/base/public/Element.h
>+  void MozRequestFullScreen(const mozilla::dom::RequestFullscreenOptions& aOptions);

Should be able to drop the "mozilla::dom" bit there, I expect.

>+  Element::MozRequestFullScreen(mozilla::dom::RequestFullscreenOptions());    \

And here.

>+++ b/content/base/src/Element.cpp
>+  if (aOptions.mVrDisplay.WasPassed() &&
>+      aOptions.mVrDisplay.Value())

So there's an API design question here.  Do we want script to allow explicitly passing { vrDisplay: null } for this options struct and treating that as if there were no vrDisplay passed at all?  That's what your code aims for right now, but it's not clear to me that we want that.  It might make more sense to throw on explicit null passed.

In any case, if you want the behavior you have now, you should probably make the IDL say "HMDVRDevice? vrDisplay = null;" and then here you can just do:

  if (aOptions.mVrDisplay) {
    opts.mVRHMDDevice = aOptions.mVrDisplay->GetHMD();
  }

or some such, without the WasPassed() and Value() bits.

If, on the other hand, you want to disallow explicit null, then have the IDL say "HMDVRDevice vrDisplay" and have this code be:

  if (aOptions.mVrDisplay.WasPassed()) {
    opts.mVRHMDDevice = aOptions.mVrDisplay.Value()->GetHMD();
  }

In either case, the curly before the if body goes at end of line, not beginning of line.

>+++ b/content/base/src/nsDocument.cpp
>+  nsCallRequestFullScreen(Element* aElement, dom::FullScreenOptions& aOptions)

Why do you need the "dom::" bit?

>+nsDocument::AsyncRequestFullScreen(Element* aElement,
>+                                   mozilla::dom::FullScreenOptions& aOptions)

And the mozilla::dom here?

>@@ -11022,7 +11033,9 @@ nsresult nsDocument::RemoteFrameFullscreenChanged(nsIDOMElement* aFrameElement,
>+  dom::FullScreenOptions opts;

And the dom:: here.

> nsDocument::RequestFullScreen(Element* aElement,
>+                              dom::FullScreenOptions& aOptions,

And here.

>+++ b/dom/base/nsPIDOMWindow.h

Should probably change the IID here.

>+++ b/dom/webidl/Element.webidl
>+dictionary RequestFullscreenOptions {

Document that this is non-standard?

I assume this stuff isn't supposed to live behind a pref or anything?
Attachment #8453295 - Flags: feedback?(bzbarsky) → feedback+
Updated.  I can't remove the mozilla::dom:: in the macro definition because it's used outside of the dom namespace in at least one place.  This isn't behind a pref, because obtaining the VRHMDDevice object is -- this doesn't really do much without it, so I figure it's safe, right?
Attachment #8453295 - Attachment is obsolete: true
Attachment #8514413 - Flags: review?(bzbarsky)
Comment on attachment 8514413 [details] [diff] [review]
Add options dict arg to mozRequestFullScreen (v2)

This doesn't address my comments about the Element.cpp code above.  Nor some of the nsDocument.cpp ones.  r- for that bit.

> this doesn't really do much without it, so I figure it's safe, right?

Well, maybe.  It makes mozRequestFullScreen(5) throw where it didn't use to before....

Probably safe enough, but still a bit worrisome.
Attachment #8514413 - Flags: review?(bzbarsky) → review-
Now with review comments properly addressed.  The remaining mozilla::dom:: namespace references are necessary (they're in headers outside of namespace blocks, or in the case of Element.h, part of a macro that gets used outside of the dom namespace and without a using decl).

I also went with having "null" to vrDisplay mean "no vr display/do normal fullscreen". It seemed a bit of a coin toss, and doing it this way means that code can potentially use a variable to indicate whether it's using VR or not, so can write |mozRequestFullScreen({ vrDisplay: vrHMD });| instead of writing

if (vrHMD) {
  el.mozRequestFullScreen({ vrDisplay: vrHMD });
} else {
  el.mozRequestFullScreen();
}

But the latter might be clearer as to what's going on.  I just picked one.
Attachment #8523103 - Flags: review?(bzbarsky)
Comment on attachment 8523103 [details] [diff] [review]
Add options dict arg to mozRequestFullScreen (v3)

r=me
Attachment #8523103 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/135200e5405d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1106351
Depends on: 1117579
This change broke a few websites (bug 1106351, bug 1117579).
So, we should backout this change in 36 and find a more compatible solution for 37.
Valdimir, could you prepare a backout of this patch? Thanks.
Flags: needinfo?(vladimir)
clearing the needinfo, I don't think we need that anymore...
Flags: needinfo?(vladimir)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.