To match other anticipated implementations and the anticipated spec, we should make our requestFullScreen implementation asynchronous. We'll need a fullscreendenied event for that.

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.

* 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.

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.

* Don't grant full-screen requests when full-screen element has moved out of document. * Save IsCallerChrome result for async full-screen request.

Comment on attachment 571898 [details] [diff] [review] Patch v2 r=me

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);

This is very old, I'm in the progress of updating our docs for the latest unprefixed implementation :-) So let's call this done.