Closed Bug 684620 Opened 13 years ago Closed 12 years ago

Make DOM full-screen work in multi-process Mozilla/B2G

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla17
Project Flags:
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 5 obsolete files)

Patch: Gaia changes, ugly, unpolished.
3.90 KB, patch
Details | Diff | Splinter Review
Patch v2: Forward fullscreen requests through BrowserElement{Child,Parent} (unbitrotten)
38.08 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
Patch part 2 v1:
17.43 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review 
For future proofing, the DOM full-screen API needs to work in multi-process Firefox/fennec. I have a patch in bug 54582 attachment 557407 [details] [diff] [review] that adds support for entering and leaving DOM full-screen mode from *content*, but I can't initiate leaving full-screen mode from chrome since there (currently) appears to be no way to get hold of a corresponding PBrowserParent from any of the C++ entry points in the chrome process. Desktop-Firefox needs this so that when the user pressed F11 (the key to exit browser full-screen mode) we exit browser full-screen mode, and DOM full-screen mode.

Fennec doesn't need that (since there's no full-screen exit mode key command), but still needs something like attachment 557407 [details] [diff] [review] landed so that it can enter DOM full-screen mode from content.
Blocks: 684625
This could probably be WONTFIX now that we're not continuing to pursue e10s.
We may be reducing its priority but we totally are still going to pursue e10s.
We very much want this for b2g.
Blocks: webapi
I'm sorry, I was confused by the things I've heard about the state of e10s. Carry on!
No longer blocks: 688082
Attached patch Patch: WIP (obsolete) — — Splinter Review 
WIP patch rebased on trunk. Forwards requests from content process across to the chrome process to make the window full-screen. Doesn't forward requests to exit the other way as I say in comment 0.
Since e10s is on hold for the time being Fennec/Firefox and we'll only want this in B2G at the present time, I'll hold of doing anything more here until B2G has a product we can make this work in. Posted my WIP patch so we can start working from there when we pick this up again.
Summary: Make DOM full-screen work in multi-process Firefox/Fennec → Make DOM full-screen work in multi-process Mozilla/B2G
Whiteboard: [b2g:blocking+]
blocking-basecamp: --- → +
blocking-kilimanjaro: --- → +
Whiteboard: [b2g:blocking+]
Depends on: 760102
Assignee: nobody → cpearce
Status: NEW → ASSIGNED
Attachment #573728 - Attachment is obsolete: true
A simple patch to forward requests from the content process to the parent process. We simply request fullscreen in the browser parent's frame. No prompting or approval UI is implemented. This worked for me in desktop B2G, I haven't tested on a physical device yet.

This is not a final implementation obviously, just a work in progress.
> No prompting or approval UI is implemented.

You're blazing a trail here, as we have no such UI anywhere in B2G at the moment.

Vivien and I discussed at the work week how we're going to do prompts, but I'm not sure I remember correctly what we concluded.  I'm hoping Vivien recalls more clearly.
There at least is the webapp install prompts that I can (hopefully) copy from...
Well, the prompts are going to be drawn in Gaia, not in the BrowserElement{Parent,Child} code.  You're just going to bubble up an event that asks the embedder (the page which contains the <iframe mozbrowser>) to draw the prompt.

The question which I unfortunately don't remember the answer to is this: In the case of the browser app, where the app itself is inside <iframe mozbrowser> and the browser tab is also inside <iframe mozbrowser>, who draws the prompt -- the embedder of the outer app iframe (the Gaia system app), or the embedder of the inner tab iframe (the browser app)?

The trick is that the embedder can fake the result of the prompt.  That is, instead of showing a prompt, it can just say "the user clicked 'allow'".  So if we let the browser app show the prompt, then the browser app gets permission to silently enable any feature that a website can use (1).

If instead we let the Gaia system app handle this, then we trust the prompts, but then we have a hard time drawing "nice" permission dialogs.  Because the system app doesn't understand the browser's UI or concept of tabs, it can't for example do a doorhanger-style notification.  And it can't do tab-modal notifications.

I /think/ the right solution is to let the browser app handle this.  The browser app is trusted (i.e., code reviewed) anyway, and can already take screenshots of whatever page you're browsing, so (1) isn't so bad.  It's also simpler this way.  But like I said, I don't remember what Vivien and I concluded last week.

All this logic relies on the permission manager, which is not complete yet.  So for your purposes here, I'd be OK with checking whether we're an app (BrowserElementChild already has code for this in its init function) and allowing full-screen if so, otherwise disallowing it.  (The plan is to allow all apps implicit full-screen permission.)

You or I or someone else can figure out the permissions prompts for webpages later.
The browser app should show the prompt for the mozbrowsers it embeds.  If the user approves the request, then the effect is as if browser app itself is asking for fullscreen privileges, and then the system app has the opportunity to confirm/deny based on its policies.  So the browser app is never allowed to grant more privileges than it itself has.
Agreed, the browser app should never grant more privileges than it itself has, so the system app needs to be able to veto permissions the browser app grants to its tabs.  We can bubble this up, for sure.
I agree that we should forward permission requests to the browser app when the request comes from a tab. The browser app could handles itself permissions it has granted when requested from a tab. However, if the app hasn't the requested permission, we could automatically deny it for the moment.
(In reply to Justin Lebar [:jlebar] from comment #10)
> 
> If instead we let the Gaia system app handle this, then we trust the
> prompts, but then we have a hard time drawing "nice" permission dialogs. 
> Because the system app doesn't understand the browser's UI or concept of
> tabs, it can't for example do a doorhanger-style notification.  And it can't
> do tab-modal notifications.
> 
> I /think/ the right solution is to let the browser app handle this.  The
> browser app is trusted (i.e., code reviewed) anyway, and can already take
> screenshots of whatever page you're browsing, so (1) isn't so bad.  It's
> also simpler this way.  But like I said, I don't remember what Vivien and I
> concluded last week.
>

I think we all agree that the browser application itself should be able to create prompts for content permissions prompts. But IMHO for first version, if it is simpler to only have system prompts for such feature that's good enough since many dialogs will already appears on top of everything anyway.

Also implemented this code in the System app is already needed so it doesn't cost much.

(For what it worth the Geolocation prompt dialog was working a few months ago when the request was made from a web page and the system was handling them (there is no UI atm, it just says: yes))
I think we should get the window manager to show the prompts, so that the prompts are the same for webcontent in browser as they are for webcontent loaded in an iframe in another web app.

Note that the fullscreen API asks for approval *after* entering fullscreen, not before, and we want the dimension of the window to be stable once we're in fullscreen (even if we're obscuring the fullscreen document with a prompt), so that authors can immediately start laying out as if they're fullscreen, and so they can make assumptions about the final canvas/viewport size.

So we don't want to have part of the browser UI visible while web content thinks it's fullscreen, while we're awaiting user approval.

Also, it's important to keep in mind that the fullscreen API doesn't know or care about tabs. It only cares, and only needs to care, about documents. Tabs are iframes, and iframes contain documents.


(In reply to Justin Lebar [:jlebar] from comment #10)
> If instead we let the Gaia system app handle this, then we trust the
> prompts, but then we have a hard time drawing "nice" permission dialogs. 
> Because the system app doesn't understand the browser's UI or concept of
> tabs, it can't for example do a doorhanger-style notification.  

The fullscreen API asks for approval *after* entering fullscreen, so by the time that the content has entered fullscreen and the approval prompt is shown, there will be no browser UI that a "doorhanger" could "hang" off of.


> And it can't do tab-modal notifications.

I don't understand the significance of this. Tabs are just documents, and fullscreen requests (from non app-origin content) should only be granted from the focused document. If focus is lost, the focused tab changes, or navigation occurs, the fullscreen request should be canceled automatically.

Of course the window manager must must exit apps from fullscreen when we switch from a fullscreen app to another app, and return to fullscreen when we restore the app (I think we can use the app manifest.fullscreen code path for this). We should only restore the app to fullscreen if the origin of the fullscreen content is in the app's origin though.


> (The plan is to allow all apps implicit full-screen permission.)

Right, but only in the app's origin. We'll use the same webcontent approval policy for fullscreen requests from content not in the app's origin (i.e. cross-origin webcontent in an iframe in the app).
The proposal that the browser app should show the permission request would apply to /all/ permission prompts.

It sounds like one could sanely implement full-screen prompts in the window manager, particularly because we ask /after/ entering fullscreen.  But that doesn't speak to the question of whether we want to implement /all/ prompts in the window manager.  And certainly if every other prompt is handled by the browser, full-screen should be too.

> I think we should get the window manager to show the prompts, so that the prompts are the same for 
> webcontent in browser as they are for webcontent loaded in an iframe in another web app.

This is a definite downside to doing the prompts in the browser.  But note that the same argument applies to alert(), which is handled by the browser and not the system app -- we want all alert()'s to look the same, but we decided that it was more important that the browser app have control.

>> (The plan is to allow all apps implicit full-screen permission.)
>
> Right, but only in the app's origin. We'll use the same webcontent approval policy for fullscreen 
> requests from content not in the app's origin (i.e. cross-origin webcontent in an iframe in the 
> app).

For the purposes of this patch, you could disallow full-screen requests when the app is off-origin so as to get around the need for a prompt.  But certainly if you /want/ to write the prompt code, I will not object!
Depends on: 772743
The model I ended up using is to change the existing fullscreen implementation to detect when we hit a process boundary while traversing the document hierarchy and send messages across the boundary to continue the operation on the other side.

I added a few the methods in nsIFrameLoader which BrowserElementParent uses to forward messages to the parent process nsIDocument; I'm not sure that's the best place, but it sure seemed convenient.

I have to pass the origin of the document which requested fullscreen around and report it up to B2G's shell.js so that it can fire an event to inform the window manager of the origin change. Gaia needs to handle this event and show an approval prompt when it occurs. Gaia's window manager should remember what origins have been approved, and not show the warning for those.

I'll also attach my Gaia patch so you can see how Gaia's supposed to react. It's unfinished and ugly, but works and can be built on.

We don't send the origin change message if the requesting origin is in the app's domain, and documents in a web app that are same-origin to the web app are not subject to same level of  security checks.

Web content fullscreen works fine with this patch. I haven't had much chance to test this with web apps; currently when a web app enters fullscreen Gaia (as far as I can tell it's Gaia) throws up a blank white screen. I'm not sure why yet, I'll look into it further, but I'm pretty sure it's Gaia...
Attachment #637843 - Attachment is obsolete: true
Attachment #641848 - Flags: review?(bugs)
Attachment #641848 - Flags: feedback?(bzbarsky)
Comment on attachment 641848 [details] [diff] [review]
Patch v1: Fullscreen support for B2G web apps + web content

Requesting review from jlebar for the BrowserElement{Parent,Child} stuff...
Attachment #641848 - Flags: review?(justin.lebar+bug)
Comment on attachment 641848 [details] [diff] [review]
Patch v1: Fullscreen support for B2G web apps + web content

I outlined some of the reasons that I thought the mozbrowser embedder should handle full-screen requests in comment 16.  Did you consider this?  I feel like, compared to the approach suggested there, this patch is much more complicated.

I don't like the fact that we're overloading mozCancelFullScreen() to mean "cancel this document's full-screen request" or "cancel one of our child document's full screen requests".  I think "cancel this full-screen request" should be a function on the iframe mozbrowser which (transitively) requested full-screen.

An additional problem with this approach is that it doesn't work with nested content processes (bug 761935); we can only traverse up one level of process separation, as far as I can tell.  Although we don't currently have nested content processes, every patch we land which doesn't work with them adds to the amount of work we'll need to do in order to enable them, further delaying that bug's completion.  So I would much prefer if this had a shot of working with nested content processes out of the box, or at least was not designed in a way which precluded nested content processes from working.

To be concrete, I was hoping we'd do something like the following:

 * BrowserElementChild asks its parent (A) "I want to go full-screen".
 * A asks its parent (B) "I want to go full-screen", and so on, until we no longer have parents.
 * One of the parents is responsible for showing a prompt.  We could make that the first parent which gets the notification, or the last parent which gets the notification.  I'd prefer the first one, personally.
Comment on attachment 641848 [details] [diff] [review]
Patch v1: Fullscreen support for B2G web apps + web content

GetCurrentDoc or GetOwnerDoc, depending on which one you want, not GetDocument().

I assume the frameloader bits are only there because you can't add chrome-only bits to nsIDOMDocument?  Worth documenting and filing a followup bug on that blocked on the new DOM bindings metabug, since those do allow you to add chrome-only stuff.
Attachment #641848 - Flags: feedback?(bzbarsky) → feedback+

(In reply to Justin Lebar [:jlebar] from comment #20)
> Comment on attachment 641848 [details] [diff] [review]
> Patch v1: Fullscreen support for B2G web apps + web content
> 
> I outlined some of the reasons that I thought the mozbrowser embedder should
> handle full-screen requests in comment 16.  Did you consider this?  I feel
> like, compared to the approach suggested there, this patch is much more
> complicated.

Yes I considered this.

Regardless of whether we handle prompts in the browser app, we need to handle prompts in the window manager so that cross origin content in iframes in (non browser) web apps can go fullscreen. Right?

Or do you want to explicitly disallow fullscreen in non-app-origin frames, now and forever?

Are all APIs that require prompts going to have a second implementation of their prompting code for non-app-origin requests in non-browser apps?

Or are all APIs that require prompts going to be disallowed in non-app-origin iframes in non-browser apps?

Is it going to be possible for someone to write another browser app which has the same features as our browser app?

It seems silly to me for us to be duplicating all our prompting code.


> I don't like the fact that we're overloading mozCancelFullScreen() to mean
> "cancel this document's full-screen request" or "cancel one of our child
> document's full screen requests".  I think "cancel this full-screen request"
> should be a function on the iframe mozbrowser which (transitively) requested
> full-screen.

I don't think you fully understand the fullscreen API.

Each document maintains a "stack" of fullscreen elements, when requesting they push elements onto the stack (unless the top is already the fullscreen element), and when canceling fullscreen they pop off the stack, making the previous fullscreen element fullscreen again. When an element requests fullscreen, all frames containing that element also effectively have their frame element request fullscreen. If a stack pop empties a document's stack, we pop in the parent document as well. When cancelFullscreen is called we clear the stacks in all descendent documents.

What my patch does is just make the fullscreen API behave as if each process' doc trees are part of the same doc tree. The API continues to work as advertised across process boundaries. Basically the API behaves the same as on desktop as in B2G with this patch, irrespective of process boundaries.


> An additional problem with this approach is that it doesn't work with nested
> content processes (bug 761935); we can only traverse up one level of process
> separation, as far as I can tell.

I tried to take this into account, and to the best of my understanding it should work with nested content processes. Why do you think it won't work?


> To be concrete, I was hoping we'd do something like the following:
> 
>  * BrowserElementChild asks its parent (A) "I want to go full-screen".
>  * A asks its parent (B) "I want to go full-screen", and so on, until we no
> longer have parents.
>  * One of the parents is responsible for showing a prompt.  We could make
> that the first parent which gets the notification, or the last parent which
> gets the notification.  I'd prefer the first one, personally.

If we took an approach like this we'd have divergent fullscreen UX on desktop and B2G.

What your proposing can probably be made to work. I'm not convinced it would be less code; but it's the UX that we want that should determine what we do, not the code size.

I'm also wary of giving away information regarding what origin is fullscreen in the subframe to arbitrary web apps.
(In reply to Boris Zbarsky (:bz) from comment #21)
> Comment on attachment 641848 [details] [diff] [review]
> Patch v1: Fullscreen support for B2G web apps + web content
> 
> GetCurrentDoc or GetOwnerDoc, depending on which one you want, not
> GetDocument().
> 
> I assume the frameloader bits are only there because you can't add
> chrome-only bits to nsIDOMDocument?  Worth documenting and filing a followup
> bug on that blocked on the new DOM bindings metabug, since those do allow
> you to add chrome-only stuff.

Basically yes. I could add methods to nsIDOMDocument which abort if the caller doesn't have chrome privledges (like nsPIDOMWindow::SetFullScreen()), but that seems ugly.
> Regardless of whether we handle prompts in the browser app, we need to handle prompts in the window 
> manager so that cross origin content in iframes in (non browser) web apps can go fullscreen. Right?

Right.

> Are all APIs that require prompts going to have a second implementation of their prompting code for 
> non-app-origin requests in non-browser apps?

To be clear, every mozbrowser embedder already must provide its own implementation for window.open, window.close, alert/prompt/confirm, and context menus.  Permission prompts would be in good company.

Note that, as your Gaia implementation reveals, we're not talking about a particularly amount of code being duplicated here.

The argument, in the case of the full-screen API, is that we prompt after we're already full-screen, so the browser doesn't need to show a prompt.  That's not a general argument, so we'll have to have this fight again when we talk about geolocation or whatever, but I'm OK having the system app be the sole prompter for full-screen, now that I understand better what's going on here.

> I'm also wary of giving away information regarding what origin is fullscreen in the subframe to arbitrary 
> web apps.

The browser API gives the browser embedder a huge amount of privilege.  The embedder can, for example, take screenshots of the browser frame content.  In comparison, revealing the origin inside a subframe that requested full-screen is a miniscule amount of privilege.

Anyway arbitrary (i.e., untrusted) web apps are not able to create <iframe mozbrowser>'s.

> I tried to take this into account, and to the best of my understanding it should work with nested 
> content processes. Why do you think it won't work?

I was missing the context you provided in comment 22: "What my patch does is just make the fullscreen API behave as if each process' doc trees are part of the same doc tree."  I understand much better what you're doing now, and I see why it should work.  Thanks for explaining.
> I could add methods to nsIDOMDocument which abort if the caller doesn't have chrome
> privledges 

Right.  With the WebIDL bindings you'll be able to add methods that only appear in chrome.  ;)
As a general comment: The nsDocument code is heavily invested in whether or not
we're in a content process.

Nearly all of the existing mozbrowser code is multiprocess-agnostic; I think
this would be a nice property to maintain, if we can.

It seems that the only reason we care about whether or not we're multi-process
is so that, when we iterate over an in-process mozbrowser, we don't call
[whatever function] twice, once by our iteration, and once via BrowserElement.
But if instead we stopped iterating when we hit a mozbrowser boundary, then we
wouldn't have this problem, right?

What do you think?

>diff --git a/content/base/public/nsIDocument.h b/content/base/public/nsIDocument.h
>--- a/content/base/public/nsIDocument.h
>+++ b/content/base/public/nsIDocument.h
>@@ -739,16 +739,32 @@ public:
>    * Asynchronously requests that the document make aElement the full-screen
>    * element, and move into full-screen mode. The current full-screen element
>    * (if any) is pushed onto the full-screen element stack, and it can be
>    * returned to full-screen status by calling RestorePreviousFullScreenState().
>    */
>   virtual void AsyncRequestFullScreen(Element* aElement) = 0;
> 
>   /**
>+   * Called when a frame in a remote child document has entered fullscreen or
>+   * changed fullscreen to another origin. aFrameElement is the iframe element
>+   * which contains the child-process fullscreen document, and aNewOrigin is
>+   * the origin of the new fullscreen document.
>+   */

Nit: "changed fullscreen to another origin" doesn't make much sense.  How about
"Called when a frame in a child process has entered fullscreen or when a
fullscreen frame in a child process changes to another origin."

>+  virtual nsresult RemoteFrameFullscreenChanged(nsIDOMElement* aFrameElement,
>+                                                const nsAString& aNewOrigin) = 0;
>+
>+  /**
>+   * Called when a frame in a remote child document has rolled back fullscreen
>+   * so that all its fullscreen element stacks are empty; we must continue the
>+   * rollback in this parent process' doc tree (branch).
>+   */

I'm not sure what you mean by "doc tree (branch)", maybe get rid of "(branch)"?
If it's an important detail, perhaps you could reword htis.

>+   virtual nsresult RemoteFrameFullscreenReverted() = 0;
>+  /**
>diff --git a/content/base/public/nsIFrameLoader.idl b/content/base/public/nsIFrameLoader.idl
>--- a/content/base/public/nsIFrameLoader.idl
>+++ b/content/base/public/nsIFrameLoader.idl
>@@ -183,16 +183,43 @@ interface nsIFrameLoader : nsISupports
>    * @see nsIDOMWindowUtils sendKeyEvent.
>    */
>   void sendCrossProcessKeyEvent(in AString aType,
>                                 in long aKeyCode,
>                                 in long aCharCode,
>                                 in long aModifiers,
>                                 [optional] in boolean aPreventDefault);
> 
>+  /**
>+   * Called when the remote child frame has changed its fullscreen state,
>+   * when entering fullscreen, and when the origin which is fullscreen changes.
>+   * aFrameElement is the iframe element which contains the child-process
>+   * fullscreen document, and aNewOrigin is the origin of the new fullscreen
>+   * document.   
>+   */
>+  void remoteFrameFullscreenChanged(in nsIDOMElement aFrameElement,
>+                                    in AString aNewOrigin);
>+
>+  /**
>+   * Called when the remote frame has popped all fullscreen elements off its
>+   * stack, so that the operation can complete on the parent side.
>+   */                                      
>+  void remoteFrameFullscreenReverted();
>+  
>+  /**
>+   * Called when the child frame has fully exit fullscreen, so that the parent
>+   * process can also fully exit.
>+   */
>+  void exitFullscreen();

These would make more sense to me on nsIDOMWindowUtils, which is more of a grab-bag than nsIFrameLoader.

>+
>+  /**
>+   * True when the child mozbrowser is cross process.
>+   */
>+  readonly attribute boolean hasRemoteFrame;

This isn't just for mozbrowser.

>diff --git a/content/base/src/nsDocument.cpp b/content/base/src/nsDocument.cpp
>--- a/content/base/src/nsDocument.cpp
>+++ b/content/base/src/nsDocument.cpp
>+static bool
>+ResetFullScreen(nsIDocument* aDocument, void* aData)
>+{
>   if (aDocument->IsFullScreenDoc()) {
>     static_cast<nsDocument*>(aDocument)->CleanupFullscreenState();
>     NS_ASSERTION(!aDocument->IsFullScreenDoc(), "Should reset full-screen");
>     nsTArray<nsIDocument*>* changed = reinterpret_cast<nsTArray<nsIDocument*>*>(aData);
>     changed->AppendElement(aDocument);
>+    
>+    if (HasCrossProcessParent(aDocument)) {
>+      // We're at the top of the content-process side doc tree. Ask the parent
>+      // process to exit fullscreen.
>+      nsAutoString origin;
>+      nsContentUtils::GetUTFOrigin(aDocument->NodePrincipal(), origin);
>+      nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>+      os->NotifyObservers(aDocument, "ask-parent-to-exit-fullscreen", nsnull);
>+    } else {
>+      // Dispatch a notification so that if this document has any
>+      // cross-process subdocuments, they'll be notified to exit fullscreen.
>+      // The BrowserElementParent listens for this event and performs the
>+      // cross process notification if it has a remote child process.
>+      nsAutoString origin;
>+      nsContentUtils::GetUTFOrigin(aDocument->NodePrincipal(), origin);
>+      nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>+      os->NotifyObservers(aDocument, "ask-children-to-exit-fullscreen", nsnull);
>+    }    

Nit: Trailing whitespace at the top and bottom of this hunk.

Why is this an |else|?

If we just stopped enumerating subdocuments when we encountered a mozbrowser
boundary, could we then be process-agnostic?

>@@ -8866,18 +8942,46 @@ IsInActiveTab(nsIDocument* aDoc)
>   fm->GetActiveWindow(getter_AddRefs(activeWindow));
>   if (!activeWindow) {
>     return false;
>   }
> 
>   return activeWindow == rootWin;
> }
> 
>-void
>-nsDocument::RequestFullScreen(Element* aElement, bool aWasCallerChrome)
>+nsresult nsDocument::RemoteFrameFullscreenChanged(nsIDOMElement* aFrameElement,
>+                                                  const nsAString& aOrigin)
>+{
>+  // Ensure the frame element is the fullscreen element in this document.
>+  nsCOMPtr<nsIContent> content(do_QueryInterface(aFrameElement));
>+  RequestFullScreen(content->AsElement(), false, aOrigin);
>+
>+  // Origin changed in child process, notify the B2G shell, so that it can
>+  // send a mozChromeEvent which notifies the window manager of the fullscreen
>+  // origin change.

Nit: We've tried to keep our comments more general than this (i.e., not focused
on B2G or its architecture).

>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
>--- a/content/base/src/nsFrameLoader.cpp
>+++ b/content/base/src/nsFrameLoader.cpp
>+nsresult
>+nsFrameLoader::ExitFullscreen()
>+{
>+  nsIDocument::ExitFullScreen(false);

Please comment the meaning of this boolean parameter (/* async = */).

>@@ -10685,16 +10713,24 @@ nsGlobalWindow::SetApp(const nsAString& 
> 
>   nsCOMPtr<mozIDOMApplication> app;
>   appsService->GetAppByManifestURL(aManifestURL, getter_AddRefs(app));
>   if (!app) {
>     NS_WARNING("No application found with the specified manifest URL");
>     return NS_ERROR_FAILURE;
>   }
> 
>+  nsCOMPtr<nsIURI> uri;
>+  nsresult res = NS_NewURI(getter_AddRefs(uri), aManifestURL);
>+  NS_ENSURE_SUCCESS(res, res);
>+
>+  nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
>+  res = ssm->GetCodebasePrincipal(uri, getter_AddRefs(mAppPrincipal));
>+  NS_ENSURE_SUCCESS(res, res);

I have a feeling that we'll want to store the full manifest URI rather than
just the principal, but all of this code is in a high degree of flux, and we
can figure it out later.

>@@ -139,16 +140,28 @@ BrowserElementChild.prototype = {
>                                this._keyEventHandler.bind(this),
>                                /* useCapture = */ true);
>     els.addSystemEventListener(global, 'DOMWindowClose',
>                                this._closeHandler.bind(this),
>                                /* useCapture = */ false);
>     els.addSystemEventListener(global, 'contextmenu',
>                                this._contextmenuHandler.bind(this),
>                                /* useCapture = */ false);
>+
>+    Services.obs.addObserver(function(aSubject, aTopic, aData) {
>+      if (aSubject == content.document) {
>+        sendAsyncMsg("fullscreen-child-origin-change", aData);
>+      }
>+     }, "fullscreen-child-origin-change", false);

Please make this one match the other two below (or vice versa).

>+    
>+    Services.obs.addObserver(this._askParentToExitFullscreen.bind(this),
>+                             'ask-parent-to-exit-fullscreen', /* ownsWeak = */ false);
>+
>+    Services.obs.addObserver(this._askParentToRollbackFullscreen.bind(this),
>+                             'ask-parent-to-rollback-fullscreen', /* ownsWeak = */ false);

Since you never release this observer, doesn't using a strong ref here leak us?

>diff --git a/dom/browser-element/BrowserElementParent.js b/dom/browser-element/BrowserElementParent.js
>--- a/dom/browser-element/BrowserElementParent.js
>+++ b/dom/browser-element/BrowserElementParent.js

>+  let os = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
>+  os.addObserver(this._askChildrenToExitFullscreen.bind(this),
>+                 'ask-children-to-exit-fullscreen',
>+                 /* ownsWeak = */ false);

Shouldn't this be weak?

>@@ -340,11 +348,40 @@ BrowserElementParent.prototype = {
>     let evt = this._window.document.createEvent("KeyboardEvent");
>     evt.initKeyEvent(data.json.type, true, true, this._window,
>                      false, false, false, false, // modifiers
>                      data.json.keyCode,
>                      data.json.charCode);
> 
>     this._frameElement.dispatchEvent(evt);
>   },
>+
>+  _exitFullscreen: function() {
>+    let frameLoader = this._frameElement.QueryInterface(Ci.nsIFrameLoaderOwner)
>+                          .frameLoader;
>+    frameLoader.exitFullscreen();
>+  },
>+
>+  _remoteFullscreenChange: function(data) {
>+    let origin = data.json;
>+    let frameLoader = this._frameElement.QueryInterface(Ci.nsIFrameLoaderOwner)
>+                          .frameLoader;
>+    frameLoader.remoteFrameFullscreenChange(this._frameElement, origin);
>+  },
>+
>+  _remoteFrameFullscreenReverted: function(data) {
>+    let frameLoader = this._frameElement.QueryInterface(Ci.nsIFrameLoaderOwner)
>+                          .frameLoader;
>+    frameLoader.remoteFrameFullscreenReverted();
>+  },
>+
>+  _askChildrenToExitFullscreen: function(subject, topic, data) {
>+    let doc = this._frameElement.ownerDocument;
>+    if (subject == doc && 
>+        this._frameElement.QueryInterface(Ci.nsIFrameLoaderOwner)
>+            .frameLoader.hasRemoteFrame) {
>+      this._sendAsyncMsg('exit-fullscreen');

We actually know when the BrowserElementParent is created whether it's a remote
frame or not; we either get a remote-browser-frame-shown or an
in-process-browser-frame-shown event, so you don't need hasRemoteFrame
(although if you want to keep it, I agree it's useful).

But if we made nsDocument process-agnostic, then we could be process-agnostic
here, too.

I'd like to have another look, but I promise to do my best to be fast.  (I'm a
DOM peer now, so Olli doesn't need to review, unless of course you want him to.)
Attachment #641848 - Flags: review?(justin.lebar+bug)
Attachment #641848 - Flags: review?(bugs)
Attachment #641848 - Flags: review-
Thanks for your comments.

(In reply to Justin Lebar [:jlebar] from comment #26)
> It seems that the only reason we care about whether or not we're
> multi-process
> is so that, when we iterate over an in-process mozbrowser, we don't call
> [whatever function] twice, once by our iteration, and once via
> BrowserElement.

Basically yes, that's correct.

> But if instead we stopped iterating when we hit a mozbrowser boundary, then
> we wouldn't have this problem, right?

Couldn't code run between sending the notification to the mozbrowser and it calling back from the other side? Then that code could be exposed to inconsistent state, i.e. the process would think it's still partially fullscreen?

Plus in future when all mozbrowser's are remote we will stop at the mozbrowser boundaries anyway; we can probably just send the "ask-parents..." notification whenever our parent is null once all mozbrowsers are remote. i.e. we can remove the explicit check then.



> >+  virtual nsresult RemoteFrameFullscreenChanged(nsIDOMElement* aFrameElement,
> >+                                                const nsAString& aNewOrigin) = 0;
> >+
> >+  /**
> >+   * Called when a frame in a remote child document has rolled back fullscreen
> >+   * so that all its fullscreen element stacks are empty; we must continue the
> >+   * rollback in this parent process' doc tree (branch).
> >+   */
> 
> I'm not sure what you mean by "doc tree (branch)", maybe get rid of
> "(branch)"?

Only one branch of the document tree can have its documents in fullscreen state at one time. We're in inconsistent state if the a fullscreen document has a parent and that parent isn't fullscreen. We preserve this property across process boundaries. I'll add to the comment to this affect.


> >+
> >+  /**
> >+   * True when the child mozbrowser is cross process.
> >+   */
> >+  readonly attribute boolean hasRemoteFrame;
> 
> This isn't just for mozbrowser.

So s/mozbrowser// I take it?

Out of curiosity, what else can have cross process children?
> Couldn't code run between sending the notification to the mozbrowser and it calling back from the 
> other side? Then that code could be exposed to inconsistent state, i.e. the process would think it's 
> still partially fullscreen?

Hmm.

That /might/ not be a problem, because by design, content inside mozbrowser can't see its parent.

But assuming there's code which will fall over if script runs when a process is partially full-screen, could you use sync messages in the browser-element code when sending notifications to an in-process frame?  You might also want to use sync messages when sending to an OOP frame, to avoid the same kind of inconsistent state across processes, depending on exactly what kind of inconsistent state you want to prevent.

> Plus in future when all mozbrowser's are remote we will stop at the mozbrowser boundaries anyway; we 
> can probably just send the "ask-parents..." notification whenever our parent is null once all 
> mozbrowsers are remote. i.e. we can remove the explicit check then.

This is so far in the future, I'd rather not bank on the presumption of eventuality if we have other options.
So I've had a look at stopping the in-process iteration in nsDocument's fullscreen state whenever we hit a mozbrowser boundary, and I can't find a clean way to do it. The problem is that each process stores the bottom-most document in the tree which requested fullscreen (sFullScreenDoc) so that we know where to start the iteration from. If we started calling back into the same process we'd need to basically maintain a separate stack of the fullscreen docs in order to ensure our iterations started in the right place and didn't stomp on each others' state. This complicates the code greatly, and I'd prefer not to do that.

I have all the other comments addressed, just waiting on a Try run before requesting review...
Do you have tests for this?
Not for B2G, there are plenty of tests for desktop fullscreen, I want to make sure nothing I changed affects that, or anything else for that matter.
> Not for B2G, there are plenty of tests for desktop fullscreen

Do you intend to write b2g/mozbrowser tests?  We have tests for every other mozbrowser feature, run in- and out-of-process; it would be a shame to be missing tests for full-screen.
Yes, I'll do that in a follow up bug, since it'll take a while to setup.
With review comments addressed.
Attachment #641848 - Attachment is obsolete: true
Attachment #641849 - Attachment is obsolete: true
Attachment #643868 - Flags: review?(justin.lebar+bug)
Comment on attachment 643868 [details] [diff] [review]
Patch v2: Forward fullscreen requests through BrowserElement{Child,Parent}

r=me with a few fixes below.

These comments are on the interdiff v1 --> v2:

interdiff -U8  <(curl -L https://bugzilla.mozilla.org/attachment.cgi?id=641848) <(curl -L https://bugzilla.mozilla.org/attachment.cgi?id=643868)

> nsresult nsDocument::RemoteFrameFullscreenChanged(nsIDOMElement* aFrameElement,
>                                                   const nsAString& aOrigin)
> {
>   // Ensure the frame element is the fullscreen element in this document.
>   nsCOMPtr<nsIContent> content(do_QueryInterface(aFrameElement));
>   RequestFullScreen(content->AsElement(), false, aOrigin);
> 
>-  // Origin changed in child process, notify the B2G shell, so that it can
>-  // send a mozChromeEvent which notifies the window manager of the fullscreen
>-  // origin change.
>+  // Origin changed in child process, send notifiction, so that chrome can
>+  // update the UI to reflect the fullscreen origin change if necessary.

Nit: Nix the second comma (or otherwise change the sentence structure).

>diff -U8 b/dom/browser-element/BrowserElementChild.js b/dom/browser-element/BrowserElementChild.js
>--- b/dom/browser-element/BrowserElementChild.js
>+++ b/dom/browser-element/BrowserElementChild.js
>@@ -51,16 +51,21 @@
> function BrowserElementChild() {
>   // Maps outer window id --> weak ref to window.  Used by modal dialog code.
>   this._windowIDDict = {};
> 
>   this._init();
> };
> 
> BrowserElementChild.prototype = {
>+
>+  classID: Components.ID("{81a79c94-c5cf-4175-b5b0-d1b94a979761}"),

Nit: I'm pretty sure you don't need a classID just because you have a QI
implementation.

>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
>+                                         Ci.nsISupportsWeakReference]),
>+
>   _init: function() {
>     debug("Starting up.");
>     sendAsyncMsg("hello");
> 
>     BrowserElementPromptService.mapWindowToBrowserElementChild(content, this);
> 
>     docShell.isBrowserFrame = true;
>     docShell.QueryInterface(Ci.nsIWebProgress)

>diff -U8 b/dom/browser-element/BrowserElementParent.js b/dom/browser-element/BrowserElementParent.js
>--- b/dom/browser-element/BrowserElementParent.js
>+++ b/dom/browser-element/BrowserElementParent.js
>@@ -54,17 +54,17 @@
>     if (!this._browserFramesPrefEnabled()) {
>       var prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
>       prefs.addObserver(BROWSER_FRAMES_ENABLED_PREF, this, /* ownsWeak = */ true);
>       return;
>     }
> 
>     debug("_init");
>     this._initialized = true;
>-
>+  

Nit: Trailing whitespace.

>@@ -177,19 +176,29 @@

> BrowserElementParent.prototype = {
>+
>+  classID: Components.ID("{632049a8-1632-471c-8c75-6fa3334227bc}"),

Don't think you need this.

>@@ -353,35 +362,27 @@
> 
>     this._frameElement.dispatchEvent(evt);
>   },
> 
>   _exitFullscreen: function() {
>-    let frameLoader = this._frameElement.QueryInterface(Ci.nsIFrameLoaderOwner)
>-                          .frameLoader;
>-    frameLoader.exitFullscreen();
>+    this._windowUtils.exitFullscreen();
>   },

This is a lot cleaner; thanks!

>+  observe: function(subject, topic, data) {
>+    if (topic == 'ask-children-to-exit-fullscreen' &&
>+        this._frameElement.ownerDocument == subject &&
>+        this._hasRemoteFrame)
>       this._sendAsyncMsg('exit-fullscreen');
>-    }
>   },

Now that bug 773980 landed (on m-i earlier today), please check this._isAlive() before touching this._frameElement.

>--- a/dom/base/nsDOMWindowUtils.cpp
>+++ b/dom/base/nsDOMWindowUtils.cpp
>@@ -2595 +2595,35 @@
> }
>+
>+nsresult
>+nsDOMWindowUtils::RemoteFrameFullscreenChanged(nsIDOMElement* aFrameElement,
>+                                            const nsAString& aNewOrigin)
>+{
>+  nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
>+  NS_ENSURE_STATE(window);
>+
>+  nsCOMPtr<nsIDocument> doc(do_QueryInterface(window->GetExtantDocument()));
>+  NS_ENSURE_STATE(doc);
>+  
>+  doc->RemoteFrameFullscreenChanged(aFrameElement, aNewOrigin);
>+  return NS_OK;
>+}

Content can access the nsDOMWindowUtils interface (it's nuts, I know...), so you need a universal xpconnect privilege check on these three methods.

>+nsresult
>+nsDOMWindowUtils::RemoteFrameFullscreenReverted()
>+{
>+  nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
>+  NS_ENSURE_STATE(window);
>+
>+  nsCOMPtr<nsIDocument> doc(do_QueryInterface(window->GetExtantDocument()));
>+  NS_ENSURE_STATE(doc);
>+  
>+  doc->RemoteFrameFullscreenReverted();
>+  return NS_OK;
>+}
>+
>+nsresult
>+nsDOMWindowUtils::ExitFullscreen()
>+{
>+  nsIDocument::ExitFullScreen(/* async = */ false);
>+  return NS_OK;
>+}
Attachment #643868 - Flags: review?(justin.lebar+bug) → review+
Chris, you may want to do a pull request on github for the Gaia specific part.
Yeah, it needs cleaning up and to be made to confirm to the prevailing style there first. I posted the patch here more as a demonstration of what was required.
Depends on: 775965
Depends on: 775967
Just an update: B2G has been pretty busted over the last week or so, so I haven't been able to land my patch. In the meantime I discovered that in-process fullscreen doesn't work properly with this patch anyway, so I'll put together another patch to sit on top of the existing one to fix that.
Attached patch Patch part 2 v1: — — Splinter Review 
With my patch above we still weren't showing the fullscreen approval UI at all the right times when requesting fullscreen in the parent process. I didn't spot this before because bug 775965 made fullscreen throw up a blank screen for me in some apps, and I'd thought at the time this was a bug in Gaia.

Problem was we relied on HasCrossProcessParent() to send the fullscreen-child-origin-change notification that chrome relied upon to show the approval UI. This meant if the fullscreen request wasn't initiated in a subprocess, we'd not be able to send fullscreen-child-origin-change, and we weren't showing the approval UI properly when running in-process.

So this patch changes fullscreen-child-origin-change to fullscreen-origin-change, and we listen for that in BrowserElementChild and in shell.js. BrowserElementChild forwards it onto the parent, and that re-fires it at the root document in the parent process, which may be the chrome doc, or if its another BrowserElementChild we'll forward it again until shell.js picks it up in the chrome document.

Once this is r+'d I'll fold it into my earlier patch for simpler landing. I'm requesting review with it as a separate patch to make reviewing easier.

Once this is r+'d I can land.
Attachment #646065 - Flags: review?(justin.lebar+bug)
(In reply to Vivien Nicolas (:vingtetun) from comment #37)
> Chris, you may want to do a pull request on github for the Gaia specific
> part.

Pull request:
https://github.com/mozilla-b2g/gaia/pull/2890
># HG changeset patch
># Parent a1bac635c1cd2bb559a00515fa451bc57eac1285
>Bug 684620 - Fix in-process fullscreen in B2G. r=?
>
>diff --git a/content/base/src/nsDocument.cpp b/content/base/src/nsDocument.cpp
>--- a/content/base/src/nsDocument.cpp
>+++ b/content/base/src/nsDocument.cpp
>+  // Clear full-screen stacks in all descendant documents, bottom up.
>   // Reset the fullscreen state in all in-process documents.

Isn't the second sentence redundant with the first one?  (If not, I don't get it.)

>+  nsIDocument* doc = fullScreenDoc;
>   while (doc != this) {
>     NS_ASSERTION(doc->IsFullScreenDoc(), "Should be full-screen doc");
>     static_cast<nsDocument*>(doc)->CleanupFullscreenState();
>     UnlockPointer();
>     DispatchFullScreenChange(doc);
>     doc = doc->GetParentDocument();
>   }
> 
>@@ -8713,31 +8713,33 @@ nsDocument::RestorePreviousFullScreenSta
>             (!nsContentUtils::IsSitePermAllow(doc->NodePrincipal(), "fullscreen") &&
>              !static_cast<nsDocument*>(doc)->mIsApprovedForFullscreen)) {
>           nsRefPtr<nsAsyncDOMEvent> e =
>             new nsAsyncDOMEvent(doc,
>                                 NS_LITERAL_STRING("MozEnteredDomFullscreen"),
>                                 true,
>                                 true);
>           e->PostDOMEvent();
>-
>-          nsIDocument* root = nsContentUtils::GetRootDocument(doc);
>-          if (HasCrossProcessParent(root)) {
>-            // We're a child process, and the fullscreen origin changed. Send
>-            // a message to the root's cross process parent to notify chrome that
>-            // the fullscreen origin has changed.
>-            nsAutoString origin;
>-            nsContentUtils::GetUTFOrigin(doc->NodePrincipal(), origin);
>-            nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>-            os->NotifyObservers(static_cast<nsIDocument*>(root),
>-                                "fullscreen-child-origin-change",
>-                                PromiseFlatString(origin).get());
>-          }
>         }
>       }
>+
>+      // The origin which is fullscreen changed. Send a notification to the
>+      // parent process (if we have one) so that a warning can be shown if
>+      // necessary.

This isn't necessarily crossing a process boundary, right?  Since this code is not process-agnostic, we should get the comments correct in this respect.

>+      if (doc != fullScreenDoc &&
>+          !nsContentUtils::HaveEqualPrincipals(doc, fullScreenDoc)) {
>+        nsAutoString origin;
>+        nsContentUtils::GetUTFOrigin(doc->NodePrincipal(), origin);
>+        nsIDocument* root = nsContentUtils::GetRootDocument(doc);
>+        nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>+        os->NotifyObservers(root,
>+                            "fullscreen-origin-change",
>+                            PromiseFlatString(origin).get());
>+      }
>+
>       sFullScreenDoc = do_GetWeakReference(doc);
>       break;
>     }
>   }
> 
>   if (doc == nsnull) {
>     // We moved all documents out of full-screen mode, reset global full-screen
>     // state and move the top-level window out of full-screen mode.
>@@ -8968,21 +8967,29 @@ IsInActiveTab(nsIDocument* aDoc)
> nsresult nsDocument::RemoteFrameFullscreenChanged(nsIDOMElement* aFrameElement,
>                                                   const nsAString& aOrigin)
> {
>   // Ensure the frame element is the fullscreen element in this document.
>+  // If the frame element is already the fullscreen element in this document,
>+  // this has no effect.
>   nsCOMPtr<nsIContent> content(do_QueryInterface(aFrameElement));
>-  RequestFullScreen(content->AsElement(), false, aOrigin);
>+  RequestFullScreen(content->AsElement(),
>+                    /* aWasCallerChrome */ false,
>+                    /* aNotifyOnOriginChange */ false);
> 
>   // Origin changed in child process, send notifiction, so that chrome can
>   // update the UI to reflect the fullscreen origin change if necessary.
>+  // The BrowserElementChild listens on this, and forwards it over to
>+  // the parent process, where it is redispatched. Chrome (in the parent
>+  // process, which could be *this* process) listens for this notification
>+  // so that it can show a warning or approval UI.

Again, this is pretty confusing because (if I understand correctly) this
comment draws process boundaries where they might not exist.  In the
parenthetical above, The parent process would be /this/ process iff there isn't
a process boundary between the parent and child, right?

>@@ -9161,16 +9159,34 @@ nsDocument::RequestFullScreen(Element* a
>   NS_ASSERTION(IsFullScreenDoc(), "Should be full-screen doc");
>   nsCOMPtr<nsIDOMElement> fse;
>   GetMozFullScreenElement(getter_AddRefs(fse));
>   nsCOMPtr<nsIContent> c(do_QueryInterface(fse));
>   NS_ASSERTION(c->AsElement() == aElement,
>     "GetMozFullScreenElement should match GetFullScreenElement()");
> #endif
> 
>+  // Origin changed in child process, send notifiction so that the chrome

notification

>+  // document knows the origin of the document which requested fullscreen.
>+  // This is used for the fullscreen approval UI. If we're in a child
>+  // process, the root BrowserElementChild also listens for this
>+  // notification, and forwards it across to its BrowserElementParent, which
>+  // re-broadcasts the message for the chrome document in its process.

Isn't the BrowserElementChild listening for the notification regardless of whether we're in a child process?

Is this notification supposed to be handled idempotently?  (I don't see how that could be, since we don't control the observers.)  If not, how are we ensuring that only one fullscreen-origin-change event happens per actual origin change event?

>+  if (aNotifyOnOriginChange &&
>+      !nsContentUtils::HaveEqualPrincipals(previousFullscreenDoc, this)) {
>+    nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>+    nsIDocument* root = nsContentUtils::GetRootDocument(this);
>+    nsAutoString origin;
>+    nsContentUtils::GetUTFOrigin(NodePrincipal(), origin);
>+    os->NotifyObservers(static_cast<nsIDocument*>(root),
>+                        "fullscreen-origin-change",
>+                        PromiseFlatString(origin).get());
>+  }
>+
>diff --git a/dom/browser-element/BrowserElementParent.js b/dom/browser-element/BrowserElementParent.js
>--- a/dom/browser-element/BrowserElementParent.js
>+++ b/dom/browser-element/BrowserElementParent.js
>@@ -158,17 +158,17 @@ function BrowserElementParent(frameLoade
>   addMessageListener("error", this._fireEventFromMsg);
>   addMessageListener("scroll", this._fireEventFromMsg);
>   addMessageListener("get-mozapp-manifest-url", this._sendMozAppManifestURL);
>   addMessageListener("keyevent", this._fireKeyEvent);
>   addMessageListener("showmodalprompt", this._handleShowModalPrompt);
>   addMessageListener('got-screenshot', this._gotDOMRequestResult);
>   addMessageListener('got-can-go-back', this._gotDOMRequestResult);
>   addMessageListener('got-can-go-forward', this._gotDOMRequestResult);
>-  addMessageListener('fullscreen-child-origin-change', this._remoteFullscreenChange);
>+  addMessageListener('fullscreen-origin-change', this._remoteFullscreenChange);

This shouldn't be called remoteFullscreenChange anymore.

That I commented almost exclusively on the comments here should indicate to you that I don't really understand this change.  I'm going to do another pass over this to try to understand what's actually going on here, but I wanted to give you feedback sooner rather than later.
(In reply to Justin Lebar [:jlebar] from comment #43)
> ># HG changeset patch
> ># Parent a1bac635c1cd2bb559a00515fa451bc57eac1285
> >Bug 684620 - Fix in-process fullscreen in B2G. r=?
> >
> >diff --git a/content/base/src/nsDocument.cpp b/content/base/src/nsDocument.cpp
> >--- a/content/base/src/nsDocument.cpp
> >+++ b/content/base/src/nsDocument.cpp
> >+  // Clear full-screen stacks in all descendant documents, bottom up.
> >   // Reset the fullscreen state in all in-process documents.
> 
> Isn't the second sentence redundant with the first one?  (If not, I don't
> get it.)

Ah yes, I will remove the first sentence. Thanks!


> >@@ -8713,31 +8713,33 @@ nsDocument::RestorePreviousFullScreenSta
> >             (!nsContentUtils::IsSitePermAllow(doc->NodePrincipal(), "fullscreen") &&
> >              !static_cast<nsDocument*>(doc)->mIsApprovedForFullscreen)) {
> >           nsRefPtr<nsAsyncDOMEvent> e =
> >             new nsAsyncDOMEvent(doc,
> >                                 NS_LITERAL_STRING("MozEnteredDomFullscreen"),
> >                                 true,
> >                                 true);
> >           e->PostDOMEvent();
> >-
> >-          nsIDocument* root = nsContentUtils::GetRootDocument(doc);
> >-          if (HasCrossProcessParent(root)) {
> >-            // We're a child process, and the fullscreen origin changed. Send
> >-            // a message to the root's cross process parent to notify chrome that
> >-            // the fullscreen origin has changed.
> >-            nsAutoString origin;
> >-            nsContentUtils::GetUTFOrigin(doc->NodePrincipal(), origin);
> >-            nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
> >-            os->NotifyObservers(static_cast<nsIDocument*>(root),
> >-                                "fullscreen-child-origin-change",
> >-                                PromiseFlatString(origin).get());
> >-          }
> >         }
> >       }
> >+
> >+      // The origin which is fullscreen changed. Send a notification to the
> >+      // parent process (if we have one) so that a warning can be shown if
> >+      // necessary.
> 
> This isn't necessarily crossing a process boundary, right?  Since this code
> is not process-agnostic, we should get the comments correct in this respect.

Correct, this isn't necessarily crossing a process boundary. We could be running this in the root process.

I'd actually adjusted this comment, but forgot to qref before requesting review. It should read:

+      // The origin which is fullscreen changed. Send a notification to
+      // chrome (possibly via the parent process) so that a warning or
+      // approval UI can be shown if necessary.

Basically, we listen for the "fullscreen-origin-change" notification in both all BrowserElementChild's and in shell.js which is in the "root" process.

Also note that the "fullscreen-origin-change" notification has its subject as the document which should respond to the notification, and the notification is targeted to the document at the process boundaries (the root document in its process). BrowserElementChild ignores notifications which don't have a subject that is their document.

In a child process, the root document has a BrowserElementChild, which will forward the "fullscreen-origin-change" notification over to the parent process, where the BrowserElementParent re-broadcasts it (by calling nsDocument::RemoteFrameFullscreenChanged() which does the broadcast).

In the "root" process the root document isn't a BrowserElementChild, and it runs shell.js which listens for this notification and will notify Gaia upon receiving it.

I could make my comments clearer by saying "root" instead of "parent" in a few key places. ;)


> > nsresult nsDocument::RemoteFrameFullscreenChanged(nsIDOMElement* aFrameElement,
> >                                                   const nsAString& aOrigin)
> > {
> >   // Ensure the frame element is the fullscreen element in this document.
> >+  // If the frame element is already the fullscreen element in this document,
> >+  // this has no effect.
> >   nsCOMPtr<nsIContent> content(do_QueryInterface(aFrameElement));
> >-  RequestFullScreen(content->AsElement(), false, aOrigin);
> >+  RequestFullScreen(content->AsElement(),
> >+                    /* aWasCallerChrome */ false,
> >+                    /* aNotifyOnOriginChange */ false);
> > 
> >   // Origin changed in child process, send notifiction, so that chrome can
> >   // update the UI to reflect the fullscreen origin change if necessary.
> >+  // The BrowserElementChild listens on this, and forwards it over to
> >+  // the parent process, where it is redispatched. Chrome (in the parent
> >+  // process, which could be *this* process) listens for this notification
> >+  // so that it can show a warning or approval UI.
> 
> Again, this is pretty confusing because (if I understand correctly) this
> comment draws process boundaries where they might not exist.  In the
> parenthetical above, The parent process would be /this/ process iff there
> isn't
> a process boundary between the parent and child, right?

Note we only call RemoteFrameFullscreenChanged in a process which is a parent. So this is *a* parent process, but it may not be the *root* process which contains shell.js.

If that comment read: "Chrome (in the root process, which could be *this* process) listens...", then would that make more sense to you?

> 
> >@@ -9161,16 +9159,34 @@ nsDocument::RequestFullScreen(Element* a
> >   NS_ASSERTION(IsFullScreenDoc(), "Should be full-screen doc");
> >   nsCOMPtr<nsIDOMElement> fse;
> >   GetMozFullScreenElement(getter_AddRefs(fse));
> >   nsCOMPtr<nsIContent> c(do_QueryInterface(fse));
> >   NS_ASSERTION(c->AsElement() == aElement,
> >     "GetMozFullScreenElement should match GetFullScreenElement()");
> > #endif
> > 
> >+  // Origin changed in child process, send notifiction so that the chrome
> 
> notification
> 
> >+  // document knows the origin of the document which requested fullscreen.
> >+  // This is used for the fullscreen approval UI. If we're in a child
> >+  // process, the root BrowserElementChild also listens for this
> >+  // notification, and forwards it across to its BrowserElementParent, which
> >+  // re-broadcasts the message for the chrome document in its process.
> 
> Isn't the BrowserElementChild listening for the notification regardless of
> whether we're in a child process?

Ah yes, I should be notifying here with the root document as the subject of the notification so that in-process BrowserElements between the parent and the root don't need to forward the notification up the chain of in-process mozbrowsers.


> Is this notification supposed to be handled idempotently?  (I don't see how
> that could be, since we don't control the observers.)

I'm not sure what you mean by this. The observer service shouldn't be broadcasting to content/Gaia? What observers don't we control?

>  If not, how are we
> ensuring that only one fullscreen-origin-change event happens per actual
> origin change event?

The subject of the notification is the document that the notification applies to. Listeners check the notification subject and only act on the notification if they're listening on behalf of the subject document.

We could use a chrome-only DOM event instead of the observer service notification if you prefer. Then we wouldn't need to check that subject==thisDocument in the observers/listeners. I would have to define a custom event class though, so that the event can have a field which is the origin that requested fullscreen.


> >diff --git a/dom/browser-element/BrowserElementParent.js b/dom/browser-element/BrowserElementParent.js
> >--- a/dom/browser-element/BrowserElementParent.js
> >+++ b/dom/browser-element/BrowserElementParent.js
> >@@ -158,17 +158,17 @@ function BrowserElementParent(frameLoade
> >   addMessageListener("error", this._fireEventFromMsg);
> >   addMessageListener("scroll", this._fireEventFromMsg);
> >   addMessageListener("get-mozapp-manifest-url", this._sendMozAppManifestURL);
> >   addMessageListener("keyevent", this._fireKeyEvent);
> >   addMessageListener("showmodalprompt", this._handleShowModalPrompt);
> >   addMessageListener('got-screenshot', this._gotDOMRequestResult);
> >   addMessageListener('got-can-go-back', this._gotDOMRequestResult);
> >   addMessageListener('got-can-go-forward', this._gotDOMRequestResult);
> >-  addMessageListener('fullscreen-child-origin-change', this._remoteFullscreenChange);
> >+  addMessageListener('fullscreen-origin-change', this._remoteFullscreenChange);
> 
> This shouldn't be called remoteFullscreenChange anymore.

How about _remoteFullscreenOriginChange? Note this message (it's not a notification!) is only received when the fullscreen origin in a remote frame changes.


> That I commented almost exclusively on the comments here should indicate to
> you that I don't really understand this change.  I'm going to do another
> pass over this to try to understand what's actually going on here, but I
> wanted to give you feedback sooner rather than later.

Hopefully that helps to clear some things up. Let me know if I can help you understand things more.
>> Is this notification supposed to be handled idempotently?  (I don't see how
>> that could be, since we don't control the observers.)
>
> I'm not sure what you mean by this. The observer service shouldn't be broadcasting to content/Gaia? 
> What observers don't we control?

I guess I was referring to shell.js.  But anyway, if we only fire the observer once per document, then we're all good.

> How about _remoteFullscreenOriginChange? Note this message (it's not a notification!) is only 
> received when the fullscreen origin in a remote frame changes.

Only if you make the following change, right?

> I should be notifying here with the root document as the subject of the notification so that 
> in-process BrowserElements between the parent and the root don't need to forward the notification 
> up the chain of in-process mozbrowsers.


> BrowserElementChild ignores notifications which don't have a subject that is their document.

I see that now.  Okay.

> We could use a chrome-only DOM event instead of the observer service notification if you prefer. 
> Then we wouldn't need to check that subject==thisDocument in the observers/listeners. I would have 
> to define a custom event class though, so that the event can have a field which is the origin that 
> requested fullscreen.

Well, you could use CustomEvent and stick the origin in the event's detail, so it wouldn't be /quite/ so bad.  But I agree it's probably not any better.

> Hopefully that helps to clear some things up. Let me know if I can help you understand things more.

Yes, this helps a lot (particularly the comment changes, actually, because it turns out I wasn't understanding the intent correctly), thanks.  I'll look at it again in the morning, because I have a rule against r+'ing patches after midnight.  :)
Comment on attachment 646065 [details] [diff] [review]
Patch part 2 v1:

r=me with some nits addressed.  Sorry this took a while; thanks for being patient with me.

>@@ -8713,31 +8713,33 @@ nsDocument::RestorePreviousFullScreenSta
>             (!nsContentUtils::IsSitePermAllow(doc->NodePrincipal(), "fullscreen") &&
>              !static_cast<nsDocument*>(doc)->mIsApprovedForFullscreen)) {
>           nsRefPtr<nsAsyncDOMEvent> e =
>             new nsAsyncDOMEvent(doc,
>                                 NS_LITERAL_STRING("MozEnteredDomFullscreen"),
>                                 true,
>                                 true);
>           e->PostDOMEvent();
>-
>-          nsIDocument* root = nsContentUtils::GetRootDocument(doc);
>-          if (HasCrossProcessParent(root)) {
>-            // We're a child process, and the fullscreen origin changed. Send
>-            // a message to the root's cross process parent to notify chrome that
>-            // the fullscreen origin has changed.
>-            nsAutoString origin;
>-            nsContentUtils::GetUTFOrigin(doc->NodePrincipal(), origin);
>-            nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>-            os->NotifyObservers(static_cast<nsIDocument*>(root),
>-                                "fullscreen-child-origin-change",
>-                                PromiseFlatString(origin).get());
>-          }
>         }
>       }
>+
>+      // The origin which is fullscreen changed. Send a notification to the
>+      // parent process (if we have one) so that a warning can be shown if
>+      // necessary.
>+      if (doc != fullScreenDoc &&
>+          !nsContentUtils::HaveEqualPrincipals(doc, fullScreenDoc)) {

Nit: Please move this comment into the if statement (or make the comment itself conditional).

>+        nsAutoString origin;
>+        nsContentUtils::GetUTFOrigin(doc->NodePrincipal(), origin);
>+        nsIDocument* root = nsContentUtils::GetRootDocument(doc);
>+        nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>+        os->NotifyObservers(root,
>+                            "fullscreen-origin-change",
>+                            PromiseFlatString(origin).get());

Nit: I don't think you need to promise flat string on an nsAutoString.  (Only the null-terminated string classes even have a get() method, so it's easy to test if I'm wrong.)

>@@ -9161,16 +9159,34 @@ nsDocument::RequestFullScreen(Element* a
>   NS_ASSERTION(IsFullScreenDoc(), "Should be full-screen doc");
>   nsCOMPtr<nsIDOMElement> fse;
>   GetMozFullScreenElement(getter_AddRefs(fse));
>   nsCOMPtr<nsIContent> c(do_QueryInterface(fse));
>   NS_ASSERTION(c->AsElement() == aElement,
>     "GetMozFullScreenElement should match GetFullScreenElement()");
> #endif
> 
>+  // Origin changed in child process, send notifiction so that the chrome
>+  // document knows the origin of the document which requested fullscreen.
>+  // This is used for the fullscreen approval UI. If we're in a child
>+  // process, the root BrowserElementChild also listens for this
>+  // notification, and forwards it across to its BrowserElementParent, which
>+  // re-broadcasts the message for the chrome document in its process.
>+  if (aNotifyOnOriginChange &&
>+      !nsContentUtils::HaveEqualPrincipals(previousFullscreenDoc, this)) {

Nit: Earlier in the patch, AIUI, you have the additional check that
previousFullScreenDoc != this.  That's redundant with the principal check, so
it's correct either way, but we should be consistent.
 
>+    nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>+    nsIDocument* root = nsContentUtils::GetRootDocument(this);
>+    nsAutoString origin;
>+    nsContentUtils::GetUTFOrigin(NodePrincipal(), origin);
>+    os->NotifyObservers(static_cast<nsIDocument*>(root),
>+                        "fullscreen-origin-change",
>+                        PromiseFlatString(origin).get());

Nit: Don't think you need PromiseFlatString.
Attachment #646065 - Flags: review?(justin.lebar+bug) → review+
Landed with nits picked, and folded into one patch:

https://hg.mozilla.org/integration/mozilla-inbound/rev/df1d8e3c45cb
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/df1d8e3c45cb
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 795184
See Also: → 961362
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: