Closed Bug 961362 Opened 6 years ago Closed 6 years ago

[e10s] HTML5 video's fullscreen feature zooms video to fill tab, not screen

Categories

(Core :: General, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla34
Tracking Status
e10s + ---
firefox29 --- affected

People

(Reporter: cpeterson, Assigned: mconley)

References

Details

Attachments

(2 files, 13 obsolete files)

21.27 KB, patch
mconley
: review+
Details | Diff | Splinter Review
744.77 KB, application/x-gzip
Details
STR:
1. With e10s enabled, opt into YouTube HTML5 video: http://youtube.com/html5
2. Play any HTML5 video
3. Click the fullscreen video button

RESULT:
The video fills the entire tab, not the whole screen.
tracking-e10s: --- → +
This bug breaks any fullscreen request, whether its embeded video or in my particular case, pdfjs requests for fullscreen mode. The call from the child to go fullscreen always goes "full tab" vs. full screen. I think this bug should get upgraded to m2.
Summary: [e10s] YouTube HTML5 video's fullscreen button zooms video to fill tab, not screen → [e10s] HTML5 video's fullscreen feature zooms video to fill tab, not screen
Blocks: old-e10s-m2
Depends on: 1002354
Assignee: nobody → mconley
Priority: -- → P2
Status: NEW → ASSIGNED
So I think I've figured this one out.

nsGlobalWindow::SetFullScreenInternal, which is what gets eventually called in order to trigger fullscreen at the top level (after the fullscreened element has been modified to consume the entire browser content area), is designed to only work at the chrome level.

We fail some checks in this function that check to make sure we're looking at a chrome docshell tree item, and then bail out.

I think the solution here is to find a point where we can forward a request to enter fullscreen mode to the parent process. Same with exit.
Bug 684620 implemented DOM fullscreen for b2g, so I'm looking there for inspiration.
See Also: → 684620
Comment on attachment 8460387 [details] [diff] [review]
[WIP strawman]: Support DOM fullscreen API in e10s tabs. r=?

So here's my crappy WIP strawman showing where I'm going with this. A few things:

1) While I'm certainly dispatching the MozEnteredDomFullscreen event on the mFrameLoader's document, it doesn't look like that's being captured by the onMozEnteredDomFullscreen handler defined in browser.js, so that's why the browser chrome isn't disappearing.

2) I'm seeing some intermittent crashes when shutting down after fullscreening with this patch. Haven't dug into why just yet - I'll post a trace once my build finishes over here.

But I'm not 100% sure this is the right approach. Bug 684620 added some observer notifications and uses messageManager message passing and interaction with DOMWindowUtils to glue the content process to the parent process. I'm going to check to see if I can re-use some of that stuff next.
Hey Chris - I saw that you worked on bug 684620. I'm trying to see if I can adapt the solution to desktop.

Does the notion of "fullscreen origins" map nicely to desktop? I'm not sure it does - it's not 100% clear to me their purpose having skimmed bug 684620. It seems I can indeed count on that "fullscreen-origin-change" observer notification to fire if fullscreen is triggered from the DOM, and I can certainly forward that to the parent process. Is that advisable? Is fullscreen-origin-change something that I can rely on in desktop-land?

Also, it seems as if b2g doesn't know (or care) whether or not nsGlobalWindow's mFullScreen is, since setting of it occurs after this condition in nsGlobalWindow::SetFullScreenInternal:

  if (mDocShell->ItemType() != nsIDocShellTreeItem::typeChrome)
    return NS_ERROR_FAILURE;

Which, I *believe* (I haven't tested this theory) will fail in the content process on b2g. It certainly does in the content process on desktop. That makes exiting fullscreen a bit tricky, since the content process's nsGlobalWindow doesn't remember if it's in fullscreen mode or not.

Anyhow, that's just a bit of a brain-dump.

So, to boil it down - is the work in bug 684620 infrastructure that I can rely on for getting the DOM Fullscreen API to work on desktop with remote tabs, or do I need to invent my own way (a la my WIP patch)?
Flags: needinfo?(cpearce)
(In reply to Mike Conley (:mconley) from comment #7)
> Hey Chris - I saw that you worked on bug 684620. I'm trying to see if I can
> adapt the solution to desktop.
> 
> Does the notion of "fullscreen origins" map nicely to desktop? I'm not sure
> it does - it's not 100% clear to me their purpose having skimmed bug 684620.
> It seems I can indeed count on that "fullscreen-origin-change" observer
> notification to fire if fullscreen is triggered from the DOM, and I can
> certainly forward that to the parent process. Is that advisable? Is
> fullscreen-origin-change something that I can rely on in desktop-land?

Looking at the fullscreen code in nsDocument.cpp, I don't see any reason why fullscreen-origin-change would not be dispatched in desktop Firefox. You should probably test that however.

We'd need to patch the fullscreen code in Firefox to handle fullscreen-origin-change as well. It's in browser-fullScreen.js. For single process Firefox at least.



> 
> Also, it seems as if b2g doesn't know (or care) whether or not
> nsGlobalWindow's mFullScreen is, 

I think B2G's top level window is fullscreen anyway.

> since setting of it occurs after this
> condition in nsGlobalWindow::SetFullScreenInternal:
> 
>   if (mDocShell->ItemType() != nsIDocShellTreeItem::typeChrome)
>     return NS_ERROR_FAILURE;
> 
> Which, I *believe* (I haven't tested this theory) will fail in the content
> process on b2g. It certainly does in the content process on desktop. That
> makes exiting fullscreen a bit tricky, since the content process's
> nsGlobalWindow doesn't remember if it's in fullscreen mode or not.

I'm not sure about this.

> 
> Anyhow, that's just a bit of a brain-dump.
> 
> So, to boil it down - is the work in bug 684620 infrastructure that I can
> rely on for getting the DOM Fullscreen API to work on desktop with remote
> tabs, or do I need to invent my own way (a la my WIP patch)?

I think you should be able to rely on it. We definitely want to be able to share the code paths there.
Flags: needinfo?(cpearce)
Attachment #8460387 - Attachment is obsolete: true
Attachment #8462948 - Attachment description: DOM Fullscreen API support for e10s. r=? → [WIP] DOM Fullscreen API support for e10s. r=?
Also flip to using AsyncEventDispatcher::OnlyChrome for MozEnteredDomFullscreen.
Attachment #8462948 - Attachment is obsolete: true
Comment on attachment 8464270 [details] [diff] [review]
Switch to using enum for dispatch target refinement in AsyncEventDispatcher

This changes AsyncEventDispatcher from using a bool to an enum for refinements in its dispatching behaviour.

Specifically, it adds 3 types of dispatching:

Normal (default) - dispatches to the target, and can be handled by both content and chrome
OnlyChrome - dispatches to target, but will only be handled by chrome handlers[1].
ChromeEventHandler - dispatched directly to the chrome event handler on the nsWindowRoot.


Thoughts on this, smaug?


[1]: At least, I think that's what's going on.
Attachment #8464270 - Flags: review?(bugs)
Comment on attachment 8464269 [details] [diff] [review]
DOM Fullscreen API support for e10s. r=?

So this approach seems to do the job.

The only thing busted with this patch is that we display the origin in large text in the approval dialog, and we don't offer to the user the ability to remember approval or deny decisions per origin.

I plan on addressing that in a follow-up.

What do you think, Chris?
Attachment #8464269 - Flags: feedback?(cpearce)
Attachment #8464270 - Flags: review?(bugs) → review+
Comment on attachment 8464269 [details] [diff] [review]
DOM Fullscreen API support for e10s. r=?

Review of attachment 8464269 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.
Attachment #8464269 - Flags: feedback?(cpearce) → feedback+
Attachment #8464269 - Attachment is obsolete: true
Comment on attachment 8466288 [details] [diff] [review]
Part 2: DOM Fullscreen API support for e10s. r=?

Requesting review from cpearce for the nsDocument changes, dao for the browser-fullScreen.js changes (sorry for inundating you with so many review requests lately - lemme know if I should redirect), and evilpie for the browser-child.js changes.

This shouldn't change the non-e10s behaviour of the DOM Fullscreen API at all, and adds initial support. You'll notice, however, that the origin header and "remember my decision" checkbox in the DOM fullscreen permission dialog is missing for e10s. I plan on fixing that in a follow-up (there seems to be some tricky interplay between DOM Fullscreen and Pointer Lock that I need to figure out before I can do that).

But I think this is a decent intermediate step which gives DOM Fullscreen support for e10s, using the same infrastructure that b2g set up.

Here's a green try push:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f6ee3cb79df0
Attachment #8466288 - Flags: review?(evilpies)
Attachment #8466288 - Flags: review?(dao)
Attachment #8466288 - Flags: review?(cpearce)
Comment on attachment 8466288 [details] [diff] [review]
Part 2: DOM Fullscreen API support for e10s. r=?

>+  uninit: function() {
>+    window.removeEventListener("fullscreen", this, true);
>+    window.removeEventListener("MozEnteredDomFullscreen", this, true);

This shouldn't be needed.

>+    FullScreen.init();
> 
>     if (window.fullScreen)
>-      onFullScreen();
>+      FullScreen.toggle();
>     if (document.mozFullScreen)
>-      onMozEnteredDomFullscreen();
>+      FullScreen.enterDomFullscreen();

How about moving this to init?
Comment on attachment 8466288 [details] [diff] [review]
Part 2: DOM Fullscreen API support for e10s. r=?

Good points Dao - I'll fix that in the next iteration once the others have had a glance at it.

Redirecting from evilpie to billm because evilpie doesn't have the cycles to review right now.
Attachment #8466288 - Flags: review?(evilpies) → review?(wmccloskey)
Comment on attachment 8466288 [details] [diff] [review]
Part 2: DOM Fullscreen API support for e10s. r=?

Review of attachment 8466288 [details] [diff] [review]:
-----------------------------------------------------------------

I must admit I don't understand the actual fullscreen stuff too well, so I'm just reviewing what I know.

::: browser/base/content/browser-fullScreen.js
@@ +14,5 @@
>    },
> +
> +  init: function() {
> +    Services.obs.addObserver(this, "ask-children-to-exit-fullscreen",
> +                             /* ownsWeak */ true);

I'm pretty opposed to weak observers when they can be avoided. Since we have the uninit function, I think it would be better to make this strong and remove the observer in uninit.

::: toolkit/content/browser-child.js
@@ +380,5 @@
> +    Services.obs.addObserver(this, "fullscreen-origin-change",
> +                             /* ownsWeak = */ true);
> +    Services.obs.addObserver(this, "ask-parent-to-exit-fullscreen",
> +                             /* ownsWeak = */ true);
> +    Services.obs.addObserver(this, "ask-parent-to-rollback-fullscreen",

It looks like you're registering the observers twice: once strongly and once weakly. Since you have the unload listener, it seems fine to just register them strongly.

@@ +383,5 @@
> +                             /* ownsWeak = */ true);
> +    Services.obs.addObserver(this, "ask-parent-to-rollback-fullscreen",
> +                             /* ownsWeak = */ true);
> +    addMessageListener("DOMFullscreen:ChildrenMustExit", () => {
> +      let utils = content.document.defaultView

I think content.document.defaultView === content.

@@ +396,5 @@
> +    });
> +  },
> +
> +  observe: function(aSubject, aTopic, aData) {
> +    if (aSubject != content.document) {

If an iframe asks for fullscreen mode, what should we do? It looks like maybe this ignores the request? Is that what we do in non-e10s? Or do these requests always get sent to toplevel windows?

@@ +409,5 @@
> +      case "ask-parent-to-exit-fullscreen": {
> +        sendAsyncMessage("DOMFullscreen:RequestExit");
> +        break;
> +      }
> +      case "ask-parent-to-rollback-fullscreen": {

Just to help me, what does rollback here mean?

::: toolkit/content/widgets/remote-browser.xml
@@ +184,5 @@
> +      <property name="_windowUtils">
> +        <getter>
> +          <![CDATA[
> +            if (!this.__windowUtils) {
> +              this.__windowUtils = window.QueryInterface(Ci.nsIInterfaceRequestor)

I'm not sure it's worth caching this.
Attachment #8466288 - Flags: review?(wmccloskey)
Attachment #8466288 - Flags: review?(dao)
Comment on attachment 8466288 [details] [diff] [review]
Part 2: DOM Fullscreen API support for e10s. r=?

Review of attachment 8466288 [details] [diff] [review]:
-----------------------------------------------------------------

This looks OK to me, but I'm not qualified to r+ changes in toolkit or nsDocument.cpp. smaug or bz can probably r+ the nsDocument change.

::: toolkit/content/browser-child.js
@@ +409,5 @@
> +      case "ask-parent-to-exit-fullscreen": {
> +        sendAsyncMessage("DOMFullscreen:RequestExit");
> +        break;
> +      }
> +      case "ask-parent-to-rollback-fullscreen": {

@billm: rollback fullscreen means make the previous thing that was fullscreen the fullscreen thing again. Fullscreen can be nested. For example a power point page may be fullscreen, then it makes a <video> inside its page fullscreen, then it rolls back to making the power point page fullscreen again (without exiting fullscreen first).
Attachment #8466288 - Flags: review?(cpearce) → feedback+
Hey cpearce, a quick follow-up question.

I'm trying to get the approval UI working, and that involves re-jigging browser-fullScreen.js to not listen for the MozDomEnteredFullscreen event, but listen for the fullscreen-origin-change notification instead on the parent side, and then react to it.

Part of the problem is that MozDomEnteredFullscreen used be an event targeted on the document that was requesting fullscreen, and that was handy, since that allowed the front-end to test whether or not the document requesting fullscreen still belonged to an active docshell (example: http://hg.mozilla.org/mozilla-central/file/71497ed2e0db/browser/base/content/browser-fullScreen.js#l117).

With fullscreen-origin-change, we lose that ability, because the notification is not connected to a particular document or frameloader - the subject is always the same root XUL window.

Do you have any ideas about how I might reconcile this? I considered passing the frameloader that is requesting fullscreen as the subject for fullscreen-origin-change... that way, I might be able to determine if the associated browser (remote or not) has an active docshell. But is there a better way?
Flags: needinfo?(cpearce)
(In reply to Mike Conley (:mconley) from comment #25)
> Do you have any ideas about how I might reconcile this? I considered passing
> the frameloader that is requesting fullscreen as the subject for
> fullscreen-origin-change... that way, I might be able to determine if the
> associated browser (remote or not) has an active docshell. But is there a
> better way?

I can't think of a better way. I'm not an expert in e10s documents, so I'm not the best person to ask...
Flags: needinfo?(cpearce)
Attachment #8466288 - Attachment is obsolete: true
Depends on: 1053413
Attachment #8471118 - Attachment is obsolete: true
Attachment #8464270 - Attachment is obsolete: true
Attachment #8472564 - Attachment is obsolete: true
Attachment #8472571 - Attachment is obsolete: true
Comment on attachment 8472577 [details] [diff] [review]
DOM Fullscreen API support for e10s. r=?

So this seems to work, but some caveats:

1) I'm not too pleased about how much dancing about I have to do to deal with the fullscreen-origin-change observer notification. I think it makes way more sense to use an nsIMessageManager message so that we don't have to guess about whether or not a notification is something that a particular browser window cares about. I've filed bug 1053413 about that. If that's something we want to block on for this, let me know. If not, this might be an acceptable workaround until it lands.

2) I had to set an _initted flag in the browser FullScreen object, otherwise I hit leaks on Linux debug builds (probably other platforms as well, though I never ran debug tests on them).

3) I had to introduce an AsyncObserverNotification class, otherwise we couldn't properly bail out of fullscreen if the platform transition is async (like OS X's). This mimics what MozEnteredDomFullscreen used to do.

4) This makes browser-fullScreen's FullScreen no longer rely on MozEnteredDomFullscreen to trigger the transition. We still fire it, and we still have tests that detect it, because I wasn't sure if we wanted to whole-sale remove it without some deprecation in case there are add-ons that consume it.

So, requesting feedback from smaug on the nsDocument.cpp changes, and billm and/or felipe for the other stuff.

If this is too ugly, maybe we should consider blocking on using messages instead of notifications, because I think that'd clean this up a bit, but might take some extra time to implement.
Attachment #8472577 - Flags: feedback?(wmccloskey)
Attachment #8472577 - Flags: feedback?(felipc)
Attachment #8472577 - Flags: feedback?(bugs)
NeilAway also suggested that instead of trying to adapt desktop to the notification world of B2G, we attempt to bring B2G over to using the MozEnteredDomFullscreen events that desktop currently uses.
That means what? I don't know how fullscreen works on b2g.
AsyncObserverNotification looks like a useful thing, which could be moved to nsContentUtils, and
then nsContentUtils could have a static method
nsContentUtils::NotifyAsynchronously(nsISupports* aSubject,
                                     const nsCString& aTopic,
                                     const nsAString& aSomeData)

(I don't know why you store topic as nsString, and not nsCString)
Comment on attachment 8472577 [details] [diff] [review]
DOM Fullscreen API support for e10s. r=?

(I don't have much to say to browser/* or toolkit/*)
Attachment #8472577 - Flags: feedback?(bugs) → feedback+
Comment on attachment 8472577 [details] [diff] [review]
DOM Fullscreen API support for e10s. r=?

Y'know what - scratch this plan. I think I've figured out a better solution.
Attachment #8472577 - Flags: feedback?(wmccloskey)
Attachment #8472577 - Flags: feedback?(felipc)
Attachment #8472577 - Attachment is obsolete: true
My new plan seems to work.

Basically, instead of keying off entering fullscreen by observing fullscreen-origin-change notifications, I added an event listener in content.js (which runs in content for both e10s and non-e10s browsers), and dispatches a message to the chrome, forwarding the event along. tabbrowser.xml hears the message, determines if we got the message from a remote browser, and then kicks off the fullscreen transition if we are before calling FullScreen.enterDomFullscreen (which displays the approval dialog).

We don't have to kick off the fullscreen transition if we're not a remote browser - the fullscreen code in nsDocument will travel up the document hierarchy all the way to the root browser document and kick it off itself.

I still need to observe notifications for children requesting to exit or rollback fullscreen, since there are no events for those. I do the ol' docShell tree owner comparison for those still.

The stuff that's common between non-e10s and e10s, I put into content.js. The stuff that's e10s-specific, I tossed into browser-child.js. Hopefully this organization makes sense.
Comment on attachment 8473137 [details] [diff] [review]
DOM Fullscreen API support for e10s. r=?

Still not sure who's best to review this stuff. I'm gonna see if billm is comfortable with the content.js, browser-child.js and remote-browser.xml changes... and then smaug for the nsDocument.cpp changes (they're small!), and then maybe Dao for the browser-fullScreen.js and other tabbrowser.xml stuff.
Attachment #8473137 - Flags: review?(wmccloskey)
Attachment #8473137 - Flags: review?(dao)
Attachment #8473137 - Flags: review?(bugs)
Comment on attachment 8473137 [details] [diff] [review]
DOM Fullscreen API support for e10s. r=?

>   nsCOMPtr<nsIDocShell> docShell = win->GetDocShell();
>   if (!docShell) {
>     return false;
>   }
>-  return docShell->GetIsBrowserOrApp();
>+  if (docShell->GetIsBrowserOrApp()) {
>+    return true;
>+  }
>+  TabChild* tabChild(TabChild::GetFrom(docShell));
>+  if (tabChild && tabChild->IsRootContentDocument()) {
>+    return true;
>+  }
I have no idea what this tries to do.
It returns true for all the documents in child process, including those
in iframes.
The method description says "Returns true if the document is a direct child of a cross process parent"
Attachment #8473137 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #44)
> Comment on attachment 8473137 [details] [diff] [review]
> DOM Fullscreen API support for e10s. r=?
> 
> >   nsCOMPtr<nsIDocShell> docShell = win->GetDocShell();
> >   if (!docShell) {
> >     return false;
> >   }
> >-  return docShell->GetIsBrowserOrApp();
> >+  if (docShell->GetIsBrowserOrApp()) {
> >+    return true;
> >+  }
> >+  TabChild* tabChild(TabChild::GetFrom(docShell));
> >+  if (tabChild && tabChild->IsRootContentDocument()) {
> >+    return true;
> >+  }
> I have no idea what this tries to do.
> It returns true for all the documents in child process, including those
> in iframes.

Are you sure? Doesn't:

  if (aDocument->GetParentDocument() != nullptr) {
    return false;
  }

Mean that we're ensuring we're the top-level document?

> The method description says "Returns true if the document is a direct child
> of a cross process parent"

Right - I'm trying to modify this method so that it returns true if we're the top-level document in a content process. The old code did a final check to make sure the docshell returned true for GetIsBrowserOrApp, which doesn't work for e10s desktop, since we're neither, so that's what my changes were for...

What would you recommend I do to this method to make it behave more precisely in the way I've described?
Flags: needinfo?(bugs)
> > >+  TabChild* tabChild(TabChild::GetFrom(docShell));
> > >+  if (tabChild && tabChild->IsRootContentDocument()) {
> > >+    return true;
> > >+  }
> > I have no idea what this tries to do.
> > It returns true for all the documents in child process, including those
> > in iframes.
> 
> Are you sure? Doesn't:
> 
>   if (aDocument->GetParentDocument() != nullptr) {
>     return false;
>   }
That does, but what are you trying to do with tabChild->IsRootContentDocument()?
Any docshell in a docshell tree under a TabChild will return the same TabChild when
GetFrom is used.


> Right - I'm trying to modify this method so that it returns true if we're
> the top-level document in a content process.
aDocument->GetParentDocument() == nullptr isn't enough for you?


But ok, I see, you return false already earlier, and then later check IsRootContentDocument, although
you actually know already that we're top level content document in e10s.
But b2g wants special handling...
This is rather misleading.. Certainly needs a comment at least why you call IsRootContentDocument
Flags: needinfo?(bugs)
Attachment #8473137 - Attachment is obsolete: true
Attachment #8473137 - Flags: review?(wmccloskey)
Attachment #8473137 - Flags: review?(dao)
Comment on attachment 8473789 [details] [diff] [review]
DOM Fullscreen API support for e10s. r=?

Ok, addressed smaug feedback.
Attachment #8473789 - Flags: review?(wmccloskey)
Attachment #8473789 - Flags: review?(dao)
Attachment #8473789 - Flags: review?(bugs)
Comment on attachment 8473789 [details] [diff] [review]
DOM Fullscreen API support for e10s. r=?

As far as I see, you could just drop
+  if (docShell->GetIsBrowserOrApp()) {
+    return true;
+  }


(This code is odd)
Attachment #8473789 - Flags: review?(bugs) → review+
Comment on attachment 8474596 [details] [diff] [review]
DOM Fullscreen API support for e10s. r+'d by smaug

Hey billm - how do you feel about how I've arranged things here wrt remote-browser.xml, browser-child.js and content.js? Does it make sense, or am I making more work for toolkit consumers by not putting more stuff in browser-content.js?
Attachment #8474596 - Attachment description: DOM Fullscreen API support for e10s. r=? → DOM Fullscreen API support for e10s. r+'d by smaug
Attachment #8474596 - Flags: review?(wmccloskey)
Attachment #8473789 - Attachment is obsolete: true
Attachment #8473789 - Flags: review?(wmccloskey)
Attachment #8473789 - Flags: review?(dao)
Comment on attachment 8474596 [details] [diff] [review]
DOM Fullscreen API support for e10s. r+'d by smaug

Review of attachment 8474596 [details] [diff] [review]:
-----------------------------------------------------------------

Looks nice.

::: browser/base/content/browser-fullScreen.js
@@ +227,3 @@
>        }
> +
> +      case "ask-children-to-exit-fullscreen": {

I think it would be cleaner to put the observer in remote-browser.xml. The code seems to fit better there. Generally there's a connection:
  browser.js <-> content.js
  remote-browser.xml <-> browser-child.js
  browser.xml <-> browser-content.js
And, in general, it's nice if the only code sending messages to a content script on the right is the code on the left. Otherwise the message becomes part of the API, which doesn't seem great.

@@ +438,4 @@
>      let hostLabel = document.getElementById("full-screen-domain-text");
>      let rememberCheckbox = document.getElementById("full-screen-remember-decision");
>      let isApproved = false;
> +    if (aOrigin) {

It seems like you're changing the meaning of this test. Previously we tested whether the URI has a null host (like a file: URL I'm guessing). Now we're just testing if the URI is empty.

@@ +454,5 @@
>        rememberCheckbox.removeAttribute("hidden");
>  
>        // Note we only allow documents whose principal's URI has a host to
>        // store permission grants.
> +      let uri = BrowserUtils.makeURI(aOrigin);

Why not move this up and leave the uri/host stuff as is?

::: browser/base/content/content.js
@@ +586,5 @@
> +  handleEvent: function(aEvent) {
> +    if (aEvent.type == "MozEnteredDomFullscreen") {
> +      this._fullscreenDoc = aEvent.target;
> +      sendAsyncMessage("MozEnteredDomFullscreen", {
> +        origin: this._fullscreenDoc.documentURI.toLowerCase(),

Why toLowercase()? Also, I usually think of an origin as some combination of the host and port, whereas this is the full URI. Maybe change the name to documentURI?

::: browser/base/content/tabbrowser.xml
@@ +3108,5 @@
>                this.selectedTab = tab;
>                window.focus();
>                break;
>              }
> +            case "MozEnteredDomFullscreen": {

Why put this here? It seems like it would fit better in browser-fullScreen.js.

::: toolkit/content/browser-child.js
@@ +399,5 @@
> +    // windows' content. We should ignore those. We differentiate by comparing
> +    // the docshell tree owners of the element and this browser's content. This
> +    // will not be necessary once we fix bug 1053413 and stop using observer
> +    // notifications for this stuff.
> +    let elementDocShellTreeOwner = aSubject.ownerGlobal

I think we usually use defaultView instead of ownerGlobal for this sort of thing. As far as I know they mean the same thing. And usually we do this sort of comparison with |document.defaultView.top !== content|.
Attachment #8474596 - Flags: review?(wmccloskey)
Attachment #8474596 - Attachment is obsolete: true
Comment on attachment 8475328 [details] [diff] [review]
DOM Fullscreen API support for e10s. r=?

Thanks for the review billm - I agree on all counts, and I've addressed your points in this latest patch.
Attachment #8475328 - Flags: review?(wmccloskey)
Comment on attachment 8475328 [details] [diff] [review]
DOM Fullscreen API support for e10s. r=?

Review of attachment 8475328 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: browser/base/content/browser-fullScreen.js
@@ +408,5 @@
> +    if (isApproved) {
> +      gBrowser.selectedBrowser
> +              .messageManager
> +              .sendAsyncMessage("DOMFullscreen:Approved");
> +    } else

Please brace both branches.

::: toolkit/content/widgets/remote-browser.xml
@@ +9,5 @@
>            xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>  
>    <binding id="remote-browser" extends="chrome://global/content/bindings/browser.xml#browser">
>  
> +    <implementation type="application/javascript" implements="nsIObserver, nsIDOMEventListener, nsIMessageListener, nsIObserver">

nsIObserver was already in this list (although we weren't using it).

@@ +318,5 @@
> +        <body><![CDATA[
> +          if (aTopic == "ask-children-to-exit-fullscreen") {
> +            // All children are supposed to exit fullscreen mode. Broadcast
> +            // the exit message to all of the remote children in this browser
> +            // window. Non-remote children get taken care of automatically

The comment about "Broadcast..." is a little out of date now I guess.
Attachment #8475328 - Flags: review?(wmccloskey) → review+
Thanks billm - good eye.

From IRC, billm asked about the removal of the following chunk of code:

    if (document.mozFullScreen)
      onMozEnteredDomFullscreen();

This got removed from browser.js's _delayedStartup function, and wasn't put into FullScreen.init() like the window.fullscreen check.

I removed that because I don't think that code's been working for quite some time - calling onMozEnteredDomFullscreen with no arguments results in us passing undefined to FullScreen.enterDomFullscreen, which will choke as soon as it attempts to check that the event target requesting fullscreen belongs to an active docshell (since the event will be undefined).

cpearce - it looks like that old call into onMozEnteredDomFullscreen was first introduced by you in bug 684625. Do you remember why it was added? Is it even possible for a window to have mozFullScreen set to true at that point in the window startup sequence?
Flags: needinfo?(cpearce)
Attachment #8475328 - Attachment is obsolete: true
Comment on attachment 8475439 [details] [diff] [review]
DOM Fullscreen API support for e10s. r=smaug,billm.

Thanks billm - pushing to try...

remote: You can view the progress of your build at the following URL:
remote:   https://tbpl.mozilla.org/?tree=Try&rev=29da007f327e
remote: Alternatively, view them on Treeherder (experimental):
remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=29da007f327e
Attachment #8475439 - Attachment description: DOM Fullscreen API support for e10s → DOM Fullscreen API support for e10s. r=smaug,billm.
Attachment #8475439 - Flags: review+
Duplicate of this bug: 1055502
Er, better try build, without me accidentally pushing a half-baked patch.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=91f00f0a73c1
(In reply to Mike Conley (:mconley) from comment #56)
> cpearce - it looks like that old call into onMozEnteredDomFullscreen was
> first introduced by you in bug 684625. Do you remember why it was added? Is
> it even possible for a window to have mozFullScreen set to true at that
> point in the window startup sequence?

I think this was there so that if the user exited the browser while in DOM fullscreen mode, when they restarted the browser and restored their session they'd get a notification again. This doesn't work anymore I notice; if the user exits their browser while in DOM fullscreen mode, when they restore the session they end up in browser fullscreen mode, but not DOM fullscreen mode, and they need to press F11 to exit fullscreen (which is bad, as it's not obvious). Someone should fix that. I don't have the time right now...
Flags: needinfo?(cpearce)
Try looks good.

remote:   https://hg.mozilla.org/integration/fx-team/rev/4359b6c8800d
Whiteboard: [fixed-in-fx-team]
Filed bug 1056148 for fixing entering DOM Fullscreen from a restored session that cpearce mentioned in comment 61.
https://hg.mozilla.org/mozilla-central/rev/4359b6c8800d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla34
While we're add it. Why not take of the fact that fullscreening HTML5 content will not restore the sidebar when it get's out of fullscreen.

ie. 
1) Have the sidebar open (bookmarks or history)
2) Fullscreen a HTML5 video.
3) Get out of fullscreen.
4) The sidebar does NOT get restored.
I was able to reproduce this bug on Firefox 33.0a1 (2014-07-01) using Windows 7 x64.

Verified fixed on Windows 7 x64, Ubuntu 14.04 x84 and Mac OSX 10.9.5 using Beta 34.0b1 (20141014134955).

This fix can be marked as verified.
Thanks vasilicamihasca, I also verified this on Windows 8.1 32bit using latest Nightly. Based on the above, marking this as Verified Fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.