Similarly to mozLockOrientation, it would help to release the security about mozRequestFullScreen for applications. It it hard otherwise to implement a window manager that make the application fullscreen if the application has asked it without requiring any user interactions.

Did this come up in the security review? I definitely wouldn't want this by default on a desktop system...

(In reply to Benjamin Smedberg [:bsmedberg] from comment #2) > Did this come up in the security review? I definitely wouldn't want this by > default on a desktop system... I agree for the desktop. Do you have the same opinion for mobile device?

No, all mobile-device apps are fullscreen by default, so I'm not even sure what it would mean to grant them fullscreen. Unless it means that the android header would be suppressed? I'm not sure I have a strong opinion about that.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #4) > No, all mobile-device apps are fullscreen by default, so I'm not even sure > what it would mean to grant them fullscreen. Unless it means that the > android header would be suppressed? I'm not sure I have a strong opinion > about that. That's basically what it means. In the case of B2G/Gaia it means the statusbar will be suppressed too.

Is this implementation B2G-specific, or would this affect multiple operating systems? Wondering if this would affect the desktop web runtime implementation - we are tracking a bug on this in bug 757686, so I'm wondering if we can kill two birds with one stone with this implementation.

(In reply to Jason Smith [:jsmith] from comment #6) > Is this implementation B2G-specific, or would this affect multiple operating > systems? Wondering if this would affect the desktop web runtime > implementation - we are tracking a bug on this in bug 757686, so I'm > wondering if we can kill two birds with one stone with this implementation. This is not B2G specific.

(In reply to Vivien Nicolas (:vingtetun) from comment #7) > (In reply to Jason Smith [:jsmith] from comment #6) > > Is this implementation B2G-specific, or would this affect multiple operating > > systems? Wondering if this would affect the desktop web runtime > > implementation - we are tracking a bug on this in bug 757686, so I'm > > wondering if we can kill two birds with one stone with this implementation. > > This is not B2G specific. This is not b2g specific but some plumbings is needed to have this working for desktop webrt.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #8) > (In reply to Vivien Nicolas (:vingtetun) from comment #7) > > (In reply to Jason Smith [:jsmith] from comment #6) > > > Is this implementation B2G-specific, or would this affect multiple operating > > > systems? Wondering if this would affect the desktop web runtime > > > implementation - we are tracking a bug on this in bug 757686, so I'm > > > wondering if we can kill two birds with one stone with this implementation. > > > > This is not B2G specific. > > This is not b2g specific but some plumbings is needed to have this working > for desktop webrt. I assume you said that because of the <iframe mozapp> expectation in this code?

Not really. The window has to know that it is inside an application. This can be set by different ways but has to be set otherwise window->IsPartOfApp() will retur false.

Comment on attachment 628722 [details] [diff] [review] Patch Review of attachment 628722 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for slow review, I was on vacation. ::: content/base/src/nsGenericElement.cpp @@ +6450,5 @@ > > static const char* > GetFullScreenError(nsIDocument* aDoc) > { > + nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(aDoc->GetInnerWindow()); nsIDocument::GetInnerWindow() return an nsPIDOMWindow, so you shouldn't need to QI here. @@ +6452,5 @@ > GetFullScreenError(nsIDocument* aDoc) > { > + nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(aDoc->GetInnerWindow()); > + if (win) { > + if (static_cast<nsGlobalWindow*>(win.get())->IsPartOfApp()) { I get the feeling that we're going to want to call nsGlobalWindow::IsPartOfApp() all over the place in future, so why don't we expose on the nsPIDOMWindow interface so we can avoid the cast? (I'm not qualified to review such a change, you'd need to do that in another patch and request review from someone else). nsGlobalWindow::IsPartOfApp() forwards to the outer window, so why not just call nsDocument::GetWindow() instead of GetInnerWindow()? I converted my fullscreen test site at http://pearce.org.nz/fullscreen/ to a web app, built with your patch applied, and ran my test app with in the patched runtime and IsPartOfApp() was failing for me. Does it work for you? Or did I do something wrong with my web app? @@ +6461,5 @@ > if (!nsContentUtils::IsRequestFullScreenAllowed()) { > return "FullScreenDeniedNotInputDriven"; > } > > if (nsContentUtils::IsSitePermDeny(aDoc->NodePrincipal(), "fullscreen")) { This will mean that script in an app will be able to enter/exit fullscreen at will, we won't only allow fullscreen requests in response to user input. Traditionally installed desktop applications already have this capability, so I suppose we're not making the web any more dangerous by allowing this... Another problem we have to consider here is pointer lock. Pointer lock requests are automatically approved when nsDocument::mIsApprovedForFullscreen is true, and that's set here: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#8993 So (assuming IsPartOfApp() is going to work...) I think we need to instead initialize mIsApprovedForFullscreen like so: mIsApprovedForFullscreen = GetWindow()->IsPartOfApp() || nsContentUtils::IsSitePermAllow(NodePrincipal(), "fullscreen"); This means when running as part of a web app, our fullscreen code will behaves as if the user has already approved the domain for fullscreen, and so pointer lock is going work while we're in fullscreen mode. Also note we'll have to do something similar to make pointerlock work when *not* in fullscreen mode for web apps when we fix bug 737100.

Thanks for the review, I will update the patch asap. (In reply to Chris Pearce (:cpearce) from comment #11) > > I converted my fullscreen test site at http://pearce.org.nz/fullscreen/ to a > web app, built with your patch applied, and ran my test app with in the > patched runtime and IsPartOfApp() was failing for me. Does it work for you? > Or did I do something wrong with my web app? > Maybe IsPartOfApp() is too specific despite his name. Actually it is turned on only if the window is an <iframe mozapp="manifest_url.webapp">. The container of the iframe also need to have the permission to spawn mozapp iframe (see http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/webapps/apphelper.js#19). And possibly dom.mozBrowserFramesEnabled should be turned on too.

(In reply to Vivien Nicolas (:vingtetun) from comment #12) > Thanks for the review, I will update the patch asap. > > (In reply to Chris Pearce (:cpearce) from comment #11) > > > > I converted my fullscreen test site at http://pearce.org.nz/fullscreen/ to a > > web app, built with your patch applied, and ran my test app with in the > > patched runtime and IsPartOfApp() was failing for me. Does it work for you? > > Or did I do something wrong with my web app? > > > > Maybe IsPartOfApp() is too specific despite his name. Actually it is turned > on only if the window is an <iframe mozapp="manifest_url.webapp">. The > container of the iframe also need to have the permission to spawn mozapp > iframe (see > http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/webapps/ > apphelper.js#19). Note - I don't think that will work on desktop (we don't operate in an iframe for web apps in the desktop web runtime).

(In reply to Jason Smith [:jsmith] from comment #13) > (In reply to Vivien Nicolas (:vingtetun) from comment #12) > > Thanks for the review, I will update the patch asap. > > > > (In reply to Chris Pearce (:cpearce) from comment #11) > > > > > > I converted my fullscreen test site at http://pearce.org.nz/fullscreen/ to a > > > web app, built with your patch applied, and ran my test app with in the > > > patched runtime and IsPartOfApp() was failing for me. Does it work for you? > > > Or did I do something wrong with my web app? > > > > > > > Maybe IsPartOfApp() is too specific despite his name. Actually it is turned > > on only if the window is an <iframe mozapp="manifest_url.webapp">. The > > container of the iframe also need to have the permission to spawn mozapp > > iframe (see > > http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/webapps/ > > apphelper.js#19). > > Note - I don't think that will work on desktop (we don't operate in an > iframe for web apps in the desktop web runtime). Can you call http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl#1154 on the window that contains the app?

(In reply to Vivien Nicolas (:vingtetun) from comment #14) > (In reply to Jason Smith [:jsmith] from comment #13) > > (In reply to Vivien Nicolas (:vingtetun) from comment #12) > > > Thanks for the review, I will update the patch asap. > > > > > > (In reply to Chris Pearce (:cpearce) from comment #11) > > > > > > > > I converted my fullscreen test site at http://pearce.org.nz/fullscreen/ to a > > > > web app, built with your patch applied, and ran my test app with in the > > > > patched runtime and IsPartOfApp() was failing for me. Does it work for you? > > > > Or did I do something wrong with my web app? > > > > > > > > > > Maybe IsPartOfApp() is too specific despite his name. Actually it is turned > > > on only if the window is an <iframe mozapp="manifest_url.webapp">. The > > > container of the iframe also need to have the permission to spawn mozapp > > > iframe (see > > > http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/webapps/ > > > apphelper.js#19). > > > > Note - I don't think that will work on desktop (we don't operate in an > > iframe for web apps in the desktop web runtime). > > Can you call > http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/ > nsIDOMWindowUtils.idl#1154 on the window that contains the app? Myk - Can you answer this question? (Myk btw is the tech lead on the engineering side for desktop, I'm the QA for desktop side).

Using nsIDOMWindowUtils is an option if you are using JS code. If you do that in C++ you can directly access nsGlobalWindow.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #16) > Using nsIDOMWindowUtils is an option if you are using JS code. If you do > that in C++ you can directly access nsGlobalWindow. Calling nsGlobalWindow::SetIsApp(true) from nsGenericElement results in an assertion failure here: http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#10179 Calling nsDOMWindowUtils::SetIsApp(true) didn't seem to have any effect. I guess we'll need more plumbing before this approach is going to work.

(In reply to Chris Pearce (:cpearce) from comment #17) > (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #16) > > Using nsIDOMWindowUtils is an option if you are using JS code. If you do > > that in C++ you can directly access nsGlobalWindow. > > Calling nsGlobalWindow::SetIsApp(true) from nsGenericElement results in an > assertion failure here: > http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow. > cpp#10179 > > Calling nsDOMWindowUtils::SetIsApp(true) didn't seem to have any effect. > > I guess we'll need more plumbing before this approach is going to work. I think you need to call first SetIsApp(true) followed by SetApp(aValidManifestURL) for the application to be trusted.

I think SetIsApp() should probably be killed. SetApp(manifestURL) should be used instead.

Hmm, I spoke too quickly... Actually right now you should do SetIsApp(true) and SetApp(manifestURL) for some reasons (a mozbrowser inside an app needs to be marked as not being an app).

Comment on attachment 634837 [details] [diff] [review] Patch v0.2 Review of attachment 634837 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the nsGlobalWindow/nsPIDOMWindow changes. I will let Chris review the fullscreen related ones. ::: dom/base/nsPIDOMWindow.h @@ +594,5 @@ > > + /** > + * Returns if the window is part of an application. > + * It will check for the window app state and its parents until a window has > + * an app state different from |TriState_Unknown|. nit: add a note explaining that TriState_Unknown is declared in nsGlobalWindow.h or re-phrase the comment to remove that keyword.

Comment on attachment 634837 [details] [diff] [review] Patch v0.2 Review of attachment 634837 [details] [diff] [review]: ----------------------------------------------------------------- r+ with two changes. ::: content/base/src/nsGenericElement.cpp @@ +6635,5 @@ > > static const char* > GetFullScreenError(nsIDocument* aDoc) > { > + nsCOMPtr<nsPIDOMWindow> win = aDoc->GetWindow(); Move this check below the IsRequestFullScreenAllowed() check; I think we should still only approve fullscreen requests made in a user generated event handler in apps. ::: dom/base/nsPIDOMWindow.h @@ +594,5 @@ > > + /** > + * Returns if the window is part of an application. > + * It will check for the window app state and its parents until a window has > + * an app state different from |TriState_Unknown|. Don't we need to update the UUID since the interface/vtable has changed?

(In reply to Chris Pearce (:cpearce) from comment #23) > ::: content/base/src/nsGenericElement.cpp > @@ +6635,5 @@ > > > > static const char* > > GetFullScreenError(nsIDocument* aDoc) > > { > > + nsCOMPtr<nsPIDOMWindow> win = aDoc->GetWindow(); > > Move this check below the IsRequestFullScreenAllowed() check; I think we > should still only approve fullscreen requests made in a user generated event > handler in apps. That make sense but that won't resolve my use case where an app has asked the permission to be fullscreen by default (there is a field for that in https://developer.mozilla.org/en/Apps/Manifest#Fields). But I guess this should be resolve by the future permission model.

(In reply to Vivien Nicolas (:vingtetun) from comment #24) > (In reply to Chris Pearce (:cpearce) from comment #23) > > ::: content/base/src/nsGenericElement.cpp > > @@ +6635,5 @@ > > > > > > static const char* > > > GetFullScreenError(nsIDocument* aDoc) > > > { > > > + nsCOMPtr<nsPIDOMWindow> win = aDoc->GetWindow(); > > > > Move this check below the IsRequestFullScreenAllowed() check; I think we > > should still only approve fullscreen requests made in a user generated event > > handler in apps. > > > That make sense but that won't resolve my use case where an app has asked > the permission to be fullscreen by default (there is a field for that in > https://developer.mozilla.org/en/Apps/Manifest#Fields). > > But I guess this should be resolve by the future permission model. Hmm, IsRequestFullScreenAllowed() doesn't seem to be working for me on B2G. Tell you what, hold off making this change in the meantime (i.e. don't require key input for fullscreen for web apps). I'll email around the stake holders and we'll figure out the exact behaviour for the various cases for fullscreen/apps/b2g.

It seems nsConentUtils::IsRequestFullScreenAllowed() is working on B2G, hazzah! Regardless, we've decided, for the time being at least, to approve fullscreen requests in webapps in the app's origin (i.e. not only approve in user generated event handlers).

I built with Vivien's patch today (has trivial bitrot), and discovered fullscreen still wasn't working in webapps because the pref to enable the fullscreen API isn't set in the webapp runtime. This patch enables it. With this patch, and with Vivien's patch, we can enter and exit fullscreen, but it's not hooked up correctly to reported approval, so pointer lock still won't work. I'll file a bug for fixing that. We can land Vivien's patch along with this one now. https://tbpl.mozilla.org/?tree=Try&rev=28ec1f0a6deb

Alright, I'm confused. The patch in comment 27 sounds like a firefox --> webapp runtime patch, more so for bug 757686, but it's here. Are we just landing both and marking both bugs as fixed (bug 757686 and this one)? Should we move the patch over to bug 757686? Or should I dup bug 757686 against this one?

(In reply to Jason Smith [:jsmith] from comment #28) > Or should I dup bug 757686 against this one? I think we should do this.

Comment on attachment 637379 [details] [diff] [review] Patch: Enable fullscreen API pref in webapp runtime This change looks fine but I don't see how these two patches alone will make the fullscreen API work in the webapp runtime. There's nothing there that will make IsPartOfApp return true, as this is something b2g-specific so far. It's probably just a matter of setting window.QI(winutils).setIsApp(true) in the toplevel of webapp.xul/js, but I don't know if there's more to it. The intended behavior is that any page can request fullscreen in a webapp, even from a different domain than the original, right? Will it still require user interaction to enter fullscreen, or can a webapp launch itself in fullscreen? (If I'm mistaken and this is all that is needed let me know. I see that you sent these patches to try to test)

As mentioned earlier, this isn't B2G specific, plumbing is required for B2G to make IsPartOfApp work there. I have tested these patches with web apps in desktop, and IsPartOfApp is returning true there. Fullscreen works on desktop with these patches (though the menubar is still showing on my Linux box in fullscreen). I haven't been able to get a web app to install on a B2G device yet, so I haven't tested there yet. I'm going to look into this further tomorrow. I think we should land this now, and follow up in another bug for the B2G plumbing.

Comment on attachment 637379 [details] [diff] [review] Patch: Enable fullscreen API pref in webapp runtime Ok, I don't know what makes IsPartOfApp return true in desktop webapps, but if it works, it works! r+ for adding the pref to enable the api. I wasn't worried about splitting up follow-ups for b2g, I was more thinking that this would not be enough to get it working on desktop

https://hg.mozilla.org/integration/mozilla-inbound/rev/87207dba8272 https://hg.mozilla.org/integration/mozilla-inbound/rev/687c15c6d651

(In reply to Felipe Gomes (:felipe) from comment #32) > Comment on attachment 637379 [details] [diff] [review] > Patch: Enable fullscreen API pref in webapp runtime > > Ok, I don't know what makes IsPartOfApp return true in desktop webapps, but > if it works, it works! r+ for adding the pref to enable the api. > > I wasn't worried about splitting up follow-ups for b2g, I was more thinking > that this would not be enough to get it working on desktop Urgh, indeed it isn't that simple. IsPartOfApp() isn't returning true, even if I try calling winUtil.setIsApp(true) in webapp.js. Fullscreen is only appearing to work in this case because of the pref change, and due to the other checks we do have recently started to pass for web apps. This is also why pointer lock is failing (bug 769150); mIsApprovedForFullscreen is being initialized to false because IsPartOfApp() is returning false. We need IsPartOfApp() to work; we need to allow fullscreen requests in the app origin to be granted even when not in a user generated event handler, so we need IsPartOfApp() to work in order for us to distinguish this case. (In reply to Felipe Gomes (:felipe) from comment #30) > The intended behavior is that any page can request fullscreen in a webapp, > even from a different domain than the original, right? Yes. > Will it still require user interaction to enter fullscreen, or can a webapp > launch itself in fullscreen? The plan is, fullscreen requests made in the same origin as the webapp will be auto-approved, and no warning or prompt for them will be shown. Fullscreen requests for origins other than the webapp's origin will prompt for approval after entering fullscreen, or if fullscreen has already been approved, will warn after entering, and requests which don't come from a user generated event handler will be denied.

Chris, can you point me to a page where I can test what is meant to work? I can help figure out what else is needed from the webapprt side. I tested a <video> with the full-screen-api.enabled pref set to true and fullscreen worked, but I guess that's a different test?

(In reply to Felipe Gomes (:felipe) from comment #36) > Chris, can you point me to a page where I can test what is meant to work? I > can help figure out what else is needed from the webapprt side. http://pearce.org.nz/f <- Fullscreen test page that is also installable as a web app. http://pearce.org.nz/s <- Simpler testcase that is just requests fullscreen. > I tested a <video> with the full-screen-api.enabled pref set to true and > fullscreen worked, but I guess that's a different test? In a web app on desktop? Fullscreen in a web app should work with the patch to set the pref. Fullscreen in web content on B2G doesn't work yet (I'm working on this ATM, will post a patch in bug 684620). Remaining work for fullscreen in web apps: 1. Display approval UI when fullscreen is requested outside of the app origin in a document whose principal's URI's domain doesn't have the "fullscreen" permission. 2. Display a warning when fullscreen is requested outside of the app origin and the document's principals's URI's domain has the "fullscreen" permisison. 3. Get IsPartOfApp() to return true when running in a web app (on desktop at least). For B2G web apps I think 1 and 2 may need to share code with web content fullscreen, and for desktop web apps we should reuse the fullscreen approval code from browser-fullScreen.js. The warning/approval UI needs to be shown in response to a "MozEnteredDomFullscreen" event which is dispatched to chrome, targeted at the fullscreen doc.

https://hg.mozilla.org/mozilla-central/rev/87207dba8272 Sorry, this merged yesterday, but I completely forgot to do the bug marking: https://hg.mozilla.org/mozilla-central/rev/687c15c6d651

Verified on desktop. Needs to still be tested on b2g.