Last Comment Bug 689058 - Implement Document.mozFullScreenEnabled
: Implement Document.mozFullScreenEnabled
Status: RESOLVED FIXED
[fixed-in-fx-team]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Jared Wein [:jaws] (please needinfo? me)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 470628 545812 694690
  Show dependency treegraph
 
Reported: 2011-09-25 14:30 PDT by Chris Pearce (:cpearce)
Modified: 2011-12-08 10:50 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for bug 689058 (3.94 KB, patch)
2011-09-30 23:42 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch for bug 689058 v2 (5.42 KB, patch)
2011-10-01 04:13 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch for bug 689058 v3 (5.48 KB, patch)
2011-10-01 04:29 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch for bug 689058 v4 (8.22 KB, patch)
2011-10-01 18:09 PDT, Jared Wein [:jaws] (please needinfo? me)
cpearce: review-
Details | Diff | Splinter Review
Patch for bug 689058 v5 (7.95 KB, patch)
2011-10-03 10:33 PDT, Jared Wein [:jaws] (please needinfo? me)
cpearce: review+
roc: superreview+
Details | Diff | Splinter Review
Patch for bug 689058 p2 (updating UUIDs) (2.55 KB, patch)
2011-10-04 10:48 PDT, Jared Wein [:jaws] (please needinfo? me)
roc: review+
Details | Diff | Splinter Review

Description Chris Pearce (:cpearce) 2011-09-25 14:30:39 PDT
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.
Comment 1 Chris Pearce (:cpearce) 2011-09-25 14:52:29 PDT
s/mozFullScreenAllowed/mozFullScreenEnabled/g
Comment 2 Jared Wein [:jaws] (please needinfo? me) 2011-09-30 23:42:37 PDT
Created attachment 563942 [details] [diff] [review]
Patch for bug 689058
Comment 3 Jared Wein [:jaws] (please needinfo? me) 2011-09-30 23:52:56 PDT
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.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-01 02:09:48 PDT
Right. What sort of feedback are you looking for other than that? :-)
Comment 5 Jared Wein [:jaws] (please needinfo? me) 2011-10-01 03:44:15 PDT
(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 :-)
Comment 6 Jared Wein [:jaws] (please needinfo? me) 2011-10-01 04:13:34 PDT
Created attachment 563951 [details] [diff] [review]
Patch for bug 689058 v2

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?
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2011-10-01 04:16:08 PDT
I don't think I've ever seen a stack-allocated nsContentList...
Comment 8 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-01 04:18:27 PDT
Stack allocating reference counted objects is bad times all around.
Comment 9 Olli Pettay [:smaug] 2011-10-01 04:25:09 PDT
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)
Comment 10 Jared Wein [:jaws] (please needinfo? me) 2011-10-01 04:29:25 PDT
Created attachment 563952 [details] [diff] [review]
Patch for bug 689058 v3

Fixed the nits that people have mentioned.
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-01 05:23:14 PDT
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 12 Chris Pearce (:cpearce) 2011-10-01 12:03:04 PDT
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.
Comment 13 Jared Wein [:jaws] (please needinfo? me) 2011-10-01 18:09:31 PDT
Created attachment 564011 [details] [diff] [review]
Patch for bug 689058 v4

Updated the code based on the feedback.
Comment 14 Chris Pearce (:cpearce) 2011-10-02 14:16:22 PDT
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.
Comment 15 Jared Wein [:jaws] (please needinfo? me) 2011-10-03 10:33:11 PDT
Created attachment 564232 [details] [diff] [review]
Patch for bug 689058 v5

Updated patch based on feedback from cpearce, as well as switched implementation to use bool instead of PRBool.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-03 13:49:12 PDT
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.
Comment 17 Chris Pearce (:cpearce) 2011-10-03 14:02:42 PDT
(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 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-03 14:12:21 PDT
Comment on attachment 564232 [details] [diff] [review]
Patch for bug 689058 v5

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

Oops
Comment 19 Jared Wein [:jaws] (please needinfo? me) 2011-10-03 17:01:14 PDT
https://hg.mozilla.org/integration/fx-team/rev/da524e9568b6
Comment 20 :Ms2ger (⌚ UTC+1/+2) 2011-10-04 03:19:56 PDT
Looks like you forgot to rev uuids for nsIDOM{HTML,SVG,XML}Document.
Comment 21 Rob Campbell [:rc] (:robcee) 2011-10-04 05:32:02 PDT
https://hg.mozilla.org/mozilla-central/rev/da524e9568b6
Comment 22 Jared Wein [:jaws] (please needinfo? me) 2011-10-04 10:48:47 PDT
Created attachment 564606 [details] [diff] [review]
Patch for bug 689058 p2 (updating UUIDs)

Ms2ger pointed out that I forgot to update UUIDs for interfaces that inherit nsIDOMDocument.

This patch simply updates the UUIDs for the other interfaces.
Comment 23 Jared Wein [:jaws] (please needinfo? me) 2011-10-04 12:40:42 PDT
https://hg.mozilla.org/integration/fx-team/rev/8ed606bd0e5e
Comment 24 Rob Campbell [:rc] (:robcee) 2011-10-06 04:22:27 PDT
We generally don't mark these as FIXED until they land in mozilla-central.

https://hg.mozilla.org/mozilla-central/rev/8ed606bd0e5e
Comment 25 Eric Shepherd [:sheppy] 2011-12-08 10:50:10 PST
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.

Note You need to log in before you can comment on or make changes to this bug.