Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Description • 12 years ago

The current fullscreen code assumes that there can only be one fullscreen document. This may not always be the case once bug 724554 is fixed.

If we don't fix this before we land bug 724554, we can get into the situation where two windows enter fullscreen, but only the window that most recently entered fullscreen can exit fullscreen.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 1 • 12 years ago

* Instead of storing a weak reference to the fullscreen document which the root of the doctree branch which is fullscreen, store a list of such references. This means we can easily find all fullscreen documents, so we can exit them all from fullscreen easily.
* Don't store references to the documents which are bottom-most in the doctree branch (the "fullscreen leaf documents"), determine them by traversing the doctree instead when we need them.
* Change nsIDocument::ExitFullscreen() to be able to exit all fullscreen doctree branches from fullscreen, or just a single doctree branch from fullscreen.
* Have each fullscreen doc store a weak reference to its fullscreen root. This is so that if a document is detached from its doctree we know what its fullscreen ancestor document is, so we can exit it from fullscreen as well.
* Includes mochitest.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 2 • 12 years ago

We listen for the observer service notification "fullscreen-approved" in order to tell when chrome has received user approval that a document is allowed to go fullscreen. We currently try to remove this observer even if it's not been added, which causes NS_ENSURE_SUCCESS failures and spams my console with warnings. I'd like to not try to remove the observer service listener only when it's been added, to avoid the warning spam in my terminal.

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 12 years ago

Sorry for the lag here.  I'm hoping to get to this tomorrow....

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 12 years ago

Still hasn't happened, obviously; I haven't had several hours in a row.  :(

Aiming to do it either tomorrow or the day after.

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 12 years ago

Comment on attachment 678929 [details] [diff] [review]
Patch: Support multiple concurrent fullscreen documents

>+++ b/content/base/public/nsIDocument.h
>+   * fullscreen state at one time. We're in inconsistent state if the a

Preexisting, but s/the a/a/

>@@ -785,30 +785,50 @@ public:
>+   * Returns the document which is at the root of this document's branch
>+   * in the document tree. Returns nullptr if the document isn't fullscreen.
>+   */
>+  virtual nsIDocument* GetFullscreenRoot() = 0;

This might want a few words about how this returns the in-process fullscreen root.

>+   * only its ancestors and descendents exit fullscreen, i.e. if there are

descendants.

>+++ b/content/base/src/nsDocument.cpp

>@@ -7122,28 +7115,35 @@ nsDocument::OnPageHide(bool aPersisted,
>+    nsIDocument::ExitFullscreen(this, /* async */ false);

Please add a blank line between this call and the following comment?

And thank you for adding the comments!

>+class FullscreenRoots {
>+  static void ForEach(void(*aFunction)(nsIDocument* aDoc));

This should probably document that removal of a document that has not yet been ttraversed during iteration will mean it doesn't get traversed, while addition of a document during iteration will not cause it to be traversed.  Or something like that.  Alternately, we could use nsTObserverArray, which would in fact traverse additions.  Either way is fine, as far as I'm concerned.

>+FullscreenRoots::ForEach(void(*aFunction)(nsIDocument* aDoc))
>+    if (root && FullscreenRoots::Contains(root)) {

Should document that the Contains() call is so that we don't traverse removed things.

>+ExitFullscreenInDocTree(nsIDocument* aMaybeNotARootDoc)
>+  nsCOMPtr<nsIDocument> root =
>+    aMaybeNotARootDoc ? aMaybeNotARootDoc->GetFullscreenRoot() : nullptr;
>+  NS_ASSERTION(root, "Should have root when in fullscreen!");

Seems like you could just MOZ_ASSERT(aMaybeNotARootDoc) up front, and not do the null-check there...

>+  if (!root)

Please leave the curly braces on this.

>+GetFullscreenLeaf(nsIDocument* aDoc, void* aData)

Shouldn't we return false in the "else if (aDoc->IsFullScreenDoc())" case too?  There's no point enumerating siblings of aDoc is aDoc is fullscreen, right?

>+GetFullscreenLeaf(nsIDocument* aDoc)
>+    // Non-leaf fullscreen document. Fullscreen leaf must be a descendent of
>+    // this document. Look for it in descedents.

descendant and descendants

I'm trying to understand why we're doing all the checks for IsFullscreenLeaf() and such in this function instead of writing it like this:

  nsIDocument* leaf = nullptr;
  GetFullScreenLeaf(aDoc, &leaf);
  if (leaf) {
    return leaf;
  }
  // Otherwise we could be either in a non-fullscreen doc tree, or we're
  // below the fullscreen doc. Start the search from the root.
  nsIDocument* root = nsContentUtils::GetRootDocument(aDoc);
  // Check that the root is actually fullscreen so we don't waste time walking
  around its descendants.
  if (!root->IsFullScreenDoc()) {
    return nullptr;
  }
  GetFullScreenLeaf(root, &leaf);
  return leaf;

Are we trying to optimize away the calls to the two-argument form GetFullScreenLeaf in common cases?  It doesn't obviously seem worth it if we can make the code here simpler.

r=me with those nits

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 12 years ago

Comment on attachment 678940 [details] [diff] [review]
Patch: Don't remove fullscreen observer if we didn't add it

r=me

	JP Rosevear [:jpr]
	Comment 7 • 12 years ago

Any reason not to land this?

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 8 • 12 years ago

(In reply to JP Rosevear [:jpr] from comment #7)
> Any reason not to land this?

We're still waiting for a security review being conducted in bug 724554. Yvan Boily is working on it.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 9 • 11 years ago

Security review completed today in bug 822654. Got the go ahead to land.

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 10 • 11 years ago

Patch 1 rebased on top of current trunk, with review nits picked. bz: I replaced my GetFullscreenLeaf() code with yours (and tested that it works (it does!)).

https://tbpl.mozilla.org/?tree=Try&rev=2e053d3cee90

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 11 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8a73639eb9b
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4516c0a30e8

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/a8a73639eb9b
https://hg.mozilla.org/mozilla-central/rev/f4516c0a30e8