Last Comment Bug 695935 - Make document.mozRequestFullScreen() asynchronous
: Make document.mozRequestFullScreen() asynchronous
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Chris Pearce (:cpearce)
:
Mentors:
Depends on: 688648 699885
Blocks: 545812
  Show dependency treegraph
 
Reported: 2011-10-19 19:01 PDT by Chris Pearce (:cpearce)
Modified: 2016-06-07 06:11 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (20.61 KB, patch)
2011-11-03 02:34 PDT, Chris Pearce (:cpearce)
bzbarsky: review+
Details | Diff | Review
Patch v2 (23.26 KB, patch)
2011-11-04 00:27 PDT, Chris Pearce (:cpearce)
bzbarsky: review+
Details | Diff | Review

Description Chris Pearce (:cpearce) 2011-10-19 19:01:33 PDT
To match other anticipated implementations and the anticipated spec, we should make our requestFullScreen implementation asynchronous. We'll need a fullscreendenied event for that.
Comment 1 Chris Pearce (:cpearce) 2011-11-02 20:03:52 PDT
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.
Comment 2 Chris Pearce (:cpearce) 2011-11-03 02:34:27 PDT
Created attachment 571578 [details] [diff] [review]
Patch v1

* 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.
Comment 3 Chris Pearce (:cpearce) 2011-11-03 14:53:33 PDT
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 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-11-03 20:16:04 PDT
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.
Comment 5 Chris Pearce (:cpearce) 2011-11-04 00:27:18 PDT
Created attachment 571898 [details] [diff] [review]
Patch v2

* Don't grant full-screen requests when full-screen element has moved out of document.
* Save IsCallerChrome result for async full-screen request.
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-11-04 11:12:01 PDT
Comment on attachment 571898 [details] [diff] [review]
Patch v2

r=me
Comment 7 :Ms2ger 2011-11-04 11:22:29 PDT
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);
Comment 9 Marco Bonardo [::mak] 2011-11-05 03:03:32 PDT
https://hg.mozilla.org/mozilla-central/rev/ed6dfa32837e
Comment 10 Jean-Yves Perrier [:teoli] 2016-06-07 06:11:51 PDT
This is very old, I'm in the progress of updating our docs for the latest unprefixed implementation :-)

So let's call this done.

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