The default bug view has changed. See this FAQ.

Handle multiple fullscreen documents concurrently

RESOLVED FIXED in mozilla22

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Depends on: 2 bugs)

Trunk
mozilla22
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 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.
(Assignee)

Comment 1

4 years ago
Created attachment 678929 [details] [diff] [review]
Patch: Support multiple concurrent fullscreen documents

* 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.
Attachment #678929 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

4 years ago
Created attachment 678940 [details] [diff] [review]
Patch: Don't remove fullscreen observer if we didn't add it

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.
Attachment #678940 - Flags: review?(bzbarsky)
Sorry for the lag here.  I'm hoping to get to this tomorrow....
Still hasn't happened, obviously; I haven't had several hours in a row.  :(

Aiming to do it either tomorrow or the day after.
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
Attachment #678929 - Flags: review?(bzbarsky) → review+
Comment on attachment 678940 [details] [diff] [review]
Patch: Don't remove fullscreen observer if we didn't add it

r=me
Attachment #678940 - Flags: review?(bzbarsky) → review+

Comment 7

4 years ago
Any reason not to land this?
(Assignee)

Comment 8

4 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.
(Assignee)

Comment 9

4 years ago
Security review completed today in bug 822654. Got the go ahead to land.
(Assignee)

Comment 10

4 years ago
Created attachment 718218 [details] [diff] [review]
Patch 1 (unbitrotten): Support multiple concurrent fullscreen documents

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
Attachment #678929 - Attachment is obsolete: true
Attachment #718218 - Flags: review+
(Assignee)

Comment 11

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8a73639eb9b
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4516c0a30e8
https://hg.mozilla.org/mozilla-central/rev/a8a73639eb9b
https://hg.mozilla.org/mozilla-central/rev/f4516c0a30e8
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22

Updated

4 years ago
Depends on: 845552

Updated

4 years ago
Depends on: 847251
(Assignee)

Updated

4 years ago
Duplicate of this bug: 876536
You need to log in before you can comment on or make changes to this bug.