Closed Bug 695935 Opened 8 years ago Closed 8 years ago

Make document.mozRequestFullScreen() asynchronous


(Core :: DOM: Core & HTML, defect)

Not set





(Reporter: cpearce, Assigned: cpearce)



(Keywords: dev-doc-complete)


(1 file, 1 obsolete file)

To match other anticipated implementations and the anticipated spec, we should make our requestFullScreen implementation asynchronous. We'll need a fullscreendenied event for that.
Assignee: nobody → chris
Version: 9 Branch → 10 Branch
Target Milestone: --- → mozilla10
Version: 10 Branch → Trunk
Blocks: 545812
No longer depends on: 545812
For the record, Chrome 17.0.926.0 Canary's full-screen API is synchronous, but Safari 5.1.1's is asynchronous. That is, video.webkitDisplayingFullscreen returns true immediately after calling video.webkitEnterFullScreen, whereas in Safari video.webkitDisplayingFullscreen does not return true until some time (>100ms) after calling video.webkitEnterFullScreen.
Attached patch Patch v1 (obsolete) — Splinter Review
* Process the full-screen request state changes asynchronously. This is to allow us to write a spec which allows implementations to show an enter full-screen animation before full-screen state is reported, and/or a permission grant UI is required to enter full-screen.
* Dispatch mozfullscreenerror events to document, not element, matching the W3 draft.
* Note this is based on top of the patch in bug 691947, which is landed on inbound.
Attachment #571578 - Flags: review?(bzbarsky)
Hmm, a new case I need to handle which is introduced by this patch is if a change tab (or ALT+TAB too I guess) comes in after full-screen is requested, but before the request is actioned. We should cancel full-screen in this case, as otherwise (in the change-tab case) the window goes full-screen, but the document with the full-screen styles applied is in a background tab.

I'll handle this in another patch in this bug. I can probably do this by checking whether the documents which receive mozfullscreenchange are descendent from the currently focused tab in browser.js.
Comment on attachment 571578 [details] [diff] [review]
Patch v1

>+++ b/content/base/src/nsDocument.cpp
> nsDocument::GetMozFullScreenEnabled(bool *aFullScreen)
>+  if ((nsContentUtils::IsCallerChrome() &&
>+      nsContentUtils::IsFullScreenApiEnabled()) ||

That second line shouldbe indented by one more space

>+nsDocument::IsFullScreenEnabled() {

Curly brace on next line please.

>+++ b/content/base/src/nsDocument.h
>+  // Returns true is a request for DOM full-screen is currently enabled in

s/is a request/if a request/

>+  // this document. This checks whether there are no windowed plugins in this
>+  // doc tree, whether the document is visible, and whether the api is not
>+  // disabled by pref.

Replace all those "whether" with "that", please.  I think that would be clearer.

>+++ b/content/html/content/src/nsGenericHTMLElement.cpp
>+class nsCallRequestFullScreen : public nsRunnable
>+    mElement->OwnerDoc()->RequestFullScreen(mElement);

Hmm.  So if the element is adopted into a different document while the event is pending, this will request fullscreen on the new document.  That seems pretty weird to me; I would prefer that we save the ownerDocument when posting the event and just bail out if it's changed since then.  But either way is ok, I guess as long as it's specced.

Per irc discussion, we need to store the IsCallerChrome state from when nsGenericHTMLElement::MozRequestFullScreen is called in the nsCallRequestFullScreen and propagate it to RequestFullScreen.  This last should replace IsFullScreenEnabled() with a test more like the one in  GetMozFullScreenEnabled (possibly factored out as a helper function).

r=me with those changes, though I'd sort of like to take a look at the code addressing the IsCallerChrome bit once it's done.
Attachment #571578 - Flags: review?(bzbarsky) → review+
Attached patch Patch v2Splinter Review
* Don't grant full-screen requests when full-screen element has moved out of document.
* Save IsCallerChrome result for async full-screen request.
Attachment #571578 - Attachment is obsolete: true
Attachment #571898 - Flags: review?(bzbarsky)
Comment on attachment 571898 [details] [diff] [review]
Patch v2

Attachment #571898 - Flags: review?(bzbarsky) → review+
Comment on attachment 571898 [details] [diff] [review]
Patch v2

>--- a/content/base/src/nsDocument.cpp
>+++ b/content/base/src/nsDocument.cpp
>+  NS_IMETHOD Run()
>+    nsDocument* doc = static_cast<nsDocument*>(mDoc.get());


>+nsDocument::AsyncRequestFullScreen(Element* aElement)
>+  nsCOMPtr<nsIRunnable> event(new nsCallRequestFullScreen(aElement));

Please make this

nsCOMPtr<nsIRunnable> event = new nsCallRequestFullScreen(aElement);
Depends on: 699885
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
This is very old, I'm in the progress of updating our docs for the latest unprefixed implementation :-)

So let's call this done.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.