We need a way to convey to authors that the DOM full-screen API is enabled and requests for full-screen are likely to be granted in the current document. This is so authors know whether they can display a "full-screen" button in their UI. Add a readonly boolean attribute to Document, mozFullScreenAllowed. This returns true when the full-screen API is enabled, no windowed plugins are present, and all ancestor documents have the mozallowfullscreen attribute set. Note this attribute doesn't imply requests will be granted - requests not run in a short-running user-generated event handler will still be denied. Additionally, we may want to fire events when Document.mozFullScreenAllowed changes, but we're not sure if we actually need this yet or not.

s/mozFullScreenAllowed/mozFullScreenEnabled/g

Comment on attachment 563942 [details] [diff] [review] Patch for bug 689058 I noticed that I hadn't implemented the two other checks for windowed plugins and ancestor documents with mozallowfullscreen attribute.

Right. What sort of feedback are you looking for other than that? :-)

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4) > Right. What sort of feedback are you looking for other than that? :-) Hehe, just wanted a gutcheck to make sure I wasn't headed down the completely wrong path :-)

I used cpearce's implementation of searching ancestor iframes for mozallowfullscreen. Should I move the bit of duplicated code to nsContentUtils? I tried to implement the windowed plugin check, but my nsContentList is length zero when tested on http://pearce.org.nz/full-screen/ with the windowed plugin button. Can you help me out here with what I might be doing wrong?

I don't think I've ever seen a stack-allocated nsContentList...

Stack allocating reference counted objects is bad times all around.

Comment on attachment 563951 [details] [diff] [review] Patch for bug 689058 v2 (Few random comments) >+nsDocument::GetMozFullScreenEnabled(PRBool *aFullScreen) >+{ >+ NS_ENSURE_ARG_POINTER(aFullScreen); >+ *aFullScreen = false; >+ if (!nsContentUtils::IsFullScreenApiEnabled()) >+ return NS_OK; if (expr) { stmt; } >+ >+ nsIDocument* rootDocument = GetRootDocument(this); >+ nsContentList objectContentList(rootDocument, kNameSpaceID_XHTML, nsGkAtoms::object, nsGkAtoms::object); Please don't use refcounted objects as stack variables. We've had security bugs when such objects have "leaked" to outside the scope. >+ for (PRUint32 i = 0; i < objectContentList.Length(false); ++i) { >+ nsIContent* objectContent = objectContentList.Item(i, false); >+ NS_ENSURE_TRUE(objectContent, NS_ERROR_UNEXPECTED); >+ nsIFrame* objectFrame = objectContent->GetPrimaryFrame(); >+ NS_ENSURE_TRUE(objectFrame, NS_ERROR_UNEXPECTED); >+ nsIWidget* objectWidget = objectFrame->GetNearestWidget(); >+ NS_ENSURE_TRUE(objectWidget, NS_ERROR_UNEXPECTED); I'm pretty sure we don't want to throw ever here, so replace NS_ERROR_* with NS_OK or something (I'm not sure what this code is trying to do)

Fixed the nits that people have mentioned.

Comment on attachment 563952 [details] [diff] [review] Patch for bug 689058 v3 Review of attachment 563952 [details] [diff] [review]: ----------------------------------------------------------------- We really need to share code between this attribute and mozRequestFullScreen.

Comment on attachment 563952 [details] [diff] [review] Patch for bug 689058 v3 Review of attachment 563952 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsDocument.cpp @@ +8671,5 @@ > return NS_OK; > } > > +NS_IMETHODIMP > +nsDocument::GetMozFullScreenEnabled(PRBool *aFullScreen) To share code, can we have nsGenericHTMLElement::MozRequestFullScreen() call this before it proceeds with the request? Then we only have one copy of the mozallowfullscreen check loop. @@ +8679,5 @@ > + if (!nsContentUtils::IsFullScreenApiEnabled()) { > + return NS_OK; > + } > + > + // Ensure that there are no windowed plugins are present. I wouldn't bother with this check yet. The code to test for windowed plugins hasn't landed yet (Bug 684618), as it depends on bug 90268 which may take a while to debug and land. The patch in bug 684618 adds nsContentUtils::HasPluginWithUncontrolledEventDispatch(nsIDocument*), which we can just use here instead of this loop. We can easily add this check when bug 684618 lands. ::: content/html/content/test/test_fullscreen-api.html @@ +43,2 @@ > SpecialPowers.setBoolPref("full-screen-api.enabled", true); > + is(document.mozFullScreenEnabled, true, "document.mozFullScreenEnabled should be true if full-screen-api.enabled is true"); The mochitest's iframe does not have a mozallowfullscreen attribute, so mozFullScreenEnabled will return false when it runs on tinderbox. Do these tests in file_fullscreen-api.html instead. ::: dom/interfaces/core/nsIDOMDocument.idl @@ +400,5 @@ > */ > readonly attribute boolean mozFullScreen; > > /** > + * Denotes whether the full-screen api is enabled, per the about:config Comment is now out of date.

Updated the code based on the feedback.

Comment on attachment 564011 [details] [diff] [review] Patch for bug 689058 v4 Review of attachment 564011 [details] [diff] [review]: ----------------------------------------------------------------- Only one change needed. :) I'm not a content-peer, so we'll also need an additional review from Olli or perhaps Roc once we get this in shape. ::: content/base/src/nsDocument.cpp @@ +8682,5 @@ > + // This stops the full-screen from being abused similar to the popups of old, > + // and it also makes it harder for bad guys' script to go full-screen and > + // spoof the browser chrome/window and phish logins etc. > + if (!nsContentUtils::IsFullScreenApiEnabled() || > + !nsContentUtils::IsRequestFullScreenAllowed()) { Do the nsContentUtils::IsRequestFullScreenAllowed() check in nsGenericHTMLElement::MozRequestFullScreen(), otherwise we can't use mozFullScreenEnabled outside user-generated event handlers, which is what we want it for. ::: content/html/content/src/nsGenericHTMLElement.cpp @@ +3399,5 @@ > } > > nsresult nsGenericHTMLElement::MozRequestFullScreen() > { > + nsIDocument* doc = GetOwnerDoc(); if (!nsContentUtils::IsRequestFullScreenAllowed()) { return NS_OK; } ::: content/html/content/test/file_fullscreen-api.html @@ +144,5 @@ > + > + SpecialPowers.setBoolPref("full-screen-api.enabled", false); > + is(document.mozFullScreenEnabled, false, "document.mozFullScreenEnabled should be false if full-screen-api.enabled is false"); > + document.body.mozRequestFullScreen(); > + ok(!document.mozFullScreen, "Should still be in normal mode, because pref has is not enabled."); We should really be verifying that requesting full-screen inside an iframe without mozallowfullscreen is denied. Eventually we'll also need to verify that a windowed plugin blocks mozFullScreenEnabled on non-MacOSX platforms. Both of these will be easier once we implement the "mozfullscreendenied" event. We can hold off testing that until "mozfullscreendenied" is implemented.

Updated patch based on feedback from cpearce, as well as switched implementation to use bool instead of PRBool.

Comment on attachment 564232 [details] [diff] [review] Patch for bug 689058 v5 Review of attachment 564232 [details] [diff] [review]: ----------------------------------------------------------------- Get review from Chris first, then ask me. ::: content/html/content/src/nsGenericHTMLElement.cpp @@ -3436,5 @@ > - // This request is not authorized by the parent document. > - return NS_OK; > - } > - node = nsContentUtils::GetCrossDocParentNode(node); > - } while (node); Who's doing this check now? Seems like you moved it to GetMozFullScreenEnabled, but you still need to check this in MozRequestFullScreen.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16) > Comment on attachment 564232 [details] [diff] [review] [diff] [details] [review] > [...] > Who's doing this check now? Seems like you moved it to > GetMozFullScreenEnabled, but you still need to check this in > MozRequestFullScreen. MozRequestFullScreen calls GetMozFullScreenEnabled to ensure the request should be granted before calling nsIDocument::RequestFullScreen() to change to full-screen mode.

Comment on attachment 564232 [details] [diff] [review] Patch for bug 689058 v5 Review of attachment 564232 [details] [diff] [review]: ----------------------------------------------------------------- Oops

Looks like you forgot to rev uuids for nsIDOM{HTML,SVG,XML}Document.

Ms2ger pointed out that I forgot to update UUIDs for interfaces that inherit nsIDOMDocument. This patch simply updates the UUIDs for the other interfaces.

We generally don't mark these as FIXED until they land in mozilla-central. https://hg.mozilla.org/mozilla-central/rev/8ed606bd0e5e

This was already documented but was flagged as being in Firefox 9, not 10. Updated: https://developer.mozilla.org/en/DOM/Using_full-screen_mode https://developer.mozilla.org/en/DOM/document.mozFullScreenEnabled And mentioned on Firefox 10 for developers.