Closed
Bug 1036606
Opened 10 years ago
Closed 10 years ago
Add options dict and vrDevice to mozRequestFullScreen
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: vlad, Assigned: vlad)
References
Details
(Keywords: dev-doc-needed)
Attachments
(1 file, 2 obsolete files)
15.54 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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 1•10 years ago
|
||
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+
Assignee | ||
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
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-
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8514413 -
Attachment is obsolete: true
Comment 5•10 years ago
|
||
Comment on attachment 8523103 [details] [diff] [review] Add options dict arg to mozRequestFullScreen (v3) r=me
Attachment #8523103 -
Flags: review?(bzbarsky) → review+
Updated•10 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/135200e5405d
https://hg.mozilla.org/mozilla-central/rev/135200e5405d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 8•9 years ago
|
||
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)
Comment 9•7 years ago
|
||
clearing the needinfo, I don't think we need that anymore...
Flags: needinfo?(vladimir)
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•