This is at least needed for permission management.

Fabrice, could you review the apps/service part?

Comment on attachment 623314 [details] [diff] [review] Part 3 - Get the application object from an AppsService based on the manifest URL and save it on the window object Justin, could you review the content parts?

Comment on attachment 623314 [details] [diff] [review] Part 3 - Get the application object from an AppsService based on the manifest URL and save it on the window object Review of attachment 623314 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed ::: dom/apps/src/AppsService.js @@ +6,5 @@ > + > +function debug(s) { > + //dump("-*- AppsService: " + s + "\n"); > +} > + do something like: let DEBUG = false; let debug = DEBUG ? function(s) { dump(...); } : function(s) { } or remove it totally... @@ +36,5 @@ > + classInfo : XPCOMUtils.generateCI({classID: APPS_SERVICE_CID, > + contractID: APPS_SERVICE_CONTRACTID, > + classDescription: "AppsService", > + interfaces: [Ci.nsIAppsService], > + flags: Ci.nsIClassInfo.DOM_OBJECT}) If you don't plan to use in a content DOM api, you don't need classInfo

Comment on attachment 623312 [details] [diff] [review] Part 1 - Get the @mozapp value from the parent process instead of its presence. I was pretty confused by this patch initially, because it's not clear that you're passing around a manifest URL. The names should be changed to reflect this. > // Set the app state of the window by requesting it to the parent. > + let mozApp = sendSyncMsg('get-mozapp')[0]; Well, you didn't fix this like I asked in bug 754140 comment 1 (you request "request /from/", not "request /to/"), but now it's doing something different anyway. :) How about "Get the app manifest from the parent, if our frame has one."? The message should be called get-mozapp-manifest-url if that's what you're doing. Please change the variables too. > content.QueryInterface(Ci.nsIInterfaceRequestor) > .getInterface(Components.interfaces.nsIDOMWindowUtils) >- .setIsApp(sendSyncMsg('get-mozapp')[0]); >+ .setIsApp(mozApp != null && mozApp != ""); Maybe |setIsApp(!!mozApp)|? (Except it should be |!!appManifestURL| or something.) >- addMessageListener("get-mozapp", this._sendAppState); >+ addMessageListener("get-mozapp", this._sendMozAppValue); this._sendMozAppManifestURL? I'd like to have a look at the new patch, if you don't mind.

Comment on attachment 623313 [details] [diff] [review] Part 2 - Add a SetApp() method that takes the manifest URL value >diff --git a/dom/base/BrowserElementChild.js b/dom/base/BrowserElementChild.js >--- a/dom/base/BrowserElementChild.js >+++ b/dom/base/BrowserElementChild.js >@@ -50,20 +50,25 @@ BrowserElementChild.prototype = { > // also mozapp). That is, mozapp is transitive down to its children, but > // mozbrowser serves as a barrier. > // > // This is because mozapp iframes have some privileges which we don't want > // to extend to untrusted mozbrowser content. > // > // Set the app state of the window by requesting it to the parent. > let mozApp = sendSyncMsg('get-mozapp')[0]; >+ let windowUtils = content.QueryInterface(Ci.nsIInterfaceRequestor) >+ .getInterface(Components.interfaces.nsIDOMWindowUtils); > >- content.QueryInterface(Ci.nsIInterfaceRequestor) >- .getInterface(Components.interfaces.nsIDOMWindowUtils) >- .setIsApp(mozApp != null && mozApp != ""); >+ if (mozApp != null && mozApp != "") { |if (!!appManifestURL)|? >+ windowUtils.setIsApp(true); >+ windowUtils.setApp(mozApp); setAppManifestURL(). > NS_IMETHODIMP > nsDOMWindowUtils::SetIsApp(bool aValue) > { >+ if (!IsUniversalXPConnectCapable()) { >+ return NS_ERROR_DOM_SECURITY_ERR; >+ } >+ OOC how does one call this function without being universal xpconnect capable? >+NS_IMETHODIMP >+nsDOMWindowUtils::SetApp(const nsAString& aManifestURL) SetAppManifestURL >+nsresult >+nsGlobalWindow::SetApp(const nsAString& aManifestURL) SetAppManifestURL >+ printf("\n URL: %s\n\n", NS_ConvertUTF16toUTF8(aManifestURL).get()); This is removed in a later patch, I see. (I don't see how splitting this up into separate patches is easier, btw; it just means that whenever I see something that looks like a mistake, I have to go look through all later patches to see if you fixed it...) >+ if (mIsApp != TriState_True) { >+ return NS_ERROR_FAILURE; >+ } Should this be an assertion? I see you tweaked it in a later patch, but still didn't make it a hard MOZ_ASSERT. >+ /** >+ * Associate the window with an application by passing the URL of the >+ * application's manifest. >+ * This method will throw an exception if the manifest URL isn't a valid URL >+ * or isn't the manifest URL of a launched application. >+ */ >+ void setApp(in DOMString manifestURL); Nit: Empty line between paragraphs (or otherwise don't wrap the end of the line after the first sentence). What's a "launched application"? From your later patch, it looks like you mean "installed application", but I'm not sure.

I indeed meant installed application. In general, I prefer to keep SetApp() instead of changing to SetManifestURL() because the content code will actually set an application object to the window based on the manifest URL (see part 3).

> In general, I prefer to keep SetApp() instead of changing to SetManifestURL() because the content > code will actually set an application object to the window based on the manifest URL (see part 3). Okay, I see. But all the code that gets the manifest URL should still be called that.

Comment on attachment 623314 [details] [diff] [review] Part 3 - Get the application object from an AppsService based on the manifest URL and save it on the window object >diff --git a/dom/apps/src/AppsService.js b/dom/apps/src/AppsService.js >new file mode 100644 >--- /dev/null >+++ b/dom/apps/src/AppsService.js >@@ -0,0 +1,43 @@ >+/* This Source Code Form is subject to the terms of the Mozilla Public >+ * License, v. 2.0. If a copy of the MPL was not distributed with this file, >+ * You can obtain one at http://mozilla.org/MPL/2.0/. */ >+ >+"use strict" >+ >+function debug(s) { >+ //dump("-*- AppsService: " + s + "\n"); >+} >+ >+const Ci = Components.interfaces; >+const Cu = Components.utils; >+ >+Cu.import("resource://gre/modules/XPCOMUtils.jsm"); >+Cu.import("resource://gre/modules/Webapps.jsm"); >+ >+const APPS_SERVICE_CONTRACTID = "@mozilla.org/AppsService;1"; >+const APPS_SERVICE_CID = Components.ID("{05072afa-92fe-45bf-ae22-39b69c117058}"); Please write a comment briefly explaining what the this code is for. I'm not even sure what the main purpose is here -- is it to expose parts of DOMApplicationRegistry as an XPCOM service? >+let myGlobal = this; You never use this. >+function AppsService() >+{ >+ debug("AppsService Constructor"); >+} >+ >+AppsService.prototype = { >+ getAppByManifestURL: function getAppByManifestURL(aManifestURL) { >+ debug("GetAppByManifestURL( " + aManifestURL + " )"); >+ return DOMApplicationRegistry.getAppByManifestURL(aManifestURL) >+ }, >+ >+ classID : APPS_SERVICE_CID, >+ QueryInterface : XPCOMUtils.generateQI([Ci.nsIAppsService]), >+ >+ classInfo : XPCOMUtils.generateCI({classID: APPS_SERVICE_CID, >+ contractID: APPS_SERVICE_CONTRACTID, >+ classDescription: "AppsService", >+ interfaces: [Ci.nsIAppsService], >+ flags: Ci.nsIClassInfo.DOM_OBJECT}) >+} >+ >+const NSGetFactory = XPCOMUtils.generateNSGetFactory([AppsService]) >diff --git a/dom/apps/tests/test_apps_service.xul b/dom/apps/tests/test_apps_service.xul >new file mode 100644 >--- /dev/null >+++ b/dom/apps/tests/test_apps_service.xul >+ var appsService = Components.classes['@mozilla.org/AppsService;1'] >+ .getService(Components.interfaces.nsIAppsService); >+ SimpleTest.ok(appsService, "Should be able to get the Apps Service"); >+ >+ SimpleTest.ok('getAppByManifestURL' in appsService, >+ "getAppByManifestURL() should be a method in nsIAppsService"); >+ >+ SimpleTest.is(appsService.getAppByManifestURL(''), null, >+ "getAppByManifestURL() should return null for an empty string manifest url"); This is kind of a lame test. Oh well. :) >diff --git a/dom/base/Webapps.jsm b/dom/base/Webapps.jsm >--- a/dom/base/Webapps.jsm >+++ b/dom/base/Webapps.jsm >+ getAppByManifestURL: function(aManifestURL) { >+ for (let id in this.webapps) { >+ let app = this.webapps[id]; >+ if (app.manifestURL == aManifestURL) { >+ return this._cloneAppObject(app); >+ } >+ } >+ >+ return null; >+ }, Add a comment that this is O(n) and could be O(1)? We'll almost surely want to change this. >diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp >--- a/dom/base/nsGlobalWindow.cpp >+++ b/dom/base/nsGlobalWindow.cpp I recall we had something of a disagreement over MOZ_ASSERT versus NS_ASSERTION last time. I still think we should use MOZ_ASSERT for invariants the programmer is responsible for; NS_ASSERTION is invariably ignored. Fatal assertions are a good thing! >@@ -10016,47 +10017,66 @@ nsGlobalWindow::SizeOfIncludingThis(nsWi > mNavigator->SizeOfIncludingThis(aWindowSizes->mMallocSizeOf) : 0; > } > > void > nsGlobalWindow::SetIsApp(bool aValue) > { > FORWARD_TO_OUTER_VOID(SetIsApp, (aValue)); > >+ NS_ASSERTION(mIsApp == TriState_Unknown, >+ "You shouldn't call SetIsApp() more than once!"); MOZ_ASSERT? > mIsApp = aValue ? TriState_True : TriState_False; > } > > bool > nsGlobalWindow::IsPartOfApp() > { > FORWARD_TO_OUTER(IsPartOfApp, (), TriState_False); > > // We go trough all window parents until we find one with |mIsApp| set to > // something. If none is found, we are not part of an application. > for (nsGlobalWindow* w = this; w; > w = static_cast<nsGlobalWindow*>(w->GetParentInternal())) { > if (w->mIsApp == TriState_True) { >+ NS_ASSERTION(mApp, >+ "Window is part of an application which has no application object!"); MOZ_ASSERT? > return true; > } else if (w->mIsApp == TriState_False) { > return false; > } > } > > return false; > } > > nsresult > nsGlobalWindow::SetApp(const nsAString& aManifestURL) > { >- printf("\n URL: %s\n\n", NS_ConvertUTF16toUTF8(aManifestURL).get()); >- > if (mIsApp != TriState_True) { >+ NS_ERROR("You should call SetIsApp(true) before calling SetApp()!"); > return NS_ERROR_FAILURE; > } MOZ_ASSERT? >+ nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID); >+ if (!appsService) { >+ NS_ERROR("Apps Service is not available!"); >+ return NS_ERROR_FAILURE; >+ } >+ >+ 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; >+ } >+ >+ mApp = app.forget(); or just |mApp = app| -- no need to avoid an addref/release here. (It just makes the code more confusing; "is he trying to do something clever?) >diff --git a/dom/base/nsGlobalWindow.h b/dom/base/nsGlobalWindow.h >--- a/dom/base/nsGlobalWindow.h >+++ b/dom/base/nsGlobalWindow.h >+ // Application associated with this window. >+ // This should only be non-null if mIsApp's value is TriState_True. >+ nsCOMPtr<mozIDOMApplication> mApp; Nit: "The application associated with this window."

(In reply to Justin Lebar [:jlebar] from comment #7) > OOC how does one call this function without being universal xpconnect > capable? There's nothing that stops content from calling QueryInterface and accessing Components.interfaces, so you can just call it as anyone else would: window.QueryInterface(Components.interfaces.nsIInterfaceRequestor) .getInterface(Components.interfaces.nsIDOMWindowUtils) .isApp = true;

Comment on attachment 623313 [details] [diff] [review] Part 2 - Add a SetApp() method that takes the manifest URL value Sorry, but these are r- until I see patches with all review comments addressed.

I have skipped some comments because they sounded like recommendations and I wasn't fully agreeing. If you see anything that I did not apply and you think should have been applied, feel free to insist.

Better if the patch is typo-less, right? :)

Comment on attachment 624212 [details] [diff] [review] Part 2 - Add a SetApp() method that takes the manifest URL value >diff --git a/dom/base/BrowserElementChild.js b/dom/base/BrowserElementChild.js >--- a/dom/base/BrowserElementChild.js >+++ b/dom/base/BrowserElementChild.js >@@ -51,20 +51,25 @@ BrowserElementChild.prototype = { > // also mozapp). That is, mozapp is transitive down to its children, but > // mozbrowser serves as a barrier. > // > // This is because mozapp iframes have some privileges which we don't want > // to extend to untrusted mozbrowser content. > // > // Get the app manifest from the parent, if our frame has one. > let appManifestURL = sendSyncMsg('get-mozapp-manifest-url')[0]; >+ let windowUtils = content.QueryInterface(Ci.nsIInterfaceRequestor) >+ .getInterface(Components.interfaces.nsIDOMWindowUtils); > >- content.QueryInterface(Ci.nsIInterfaceRequestor) >- .getInterface(Components.interfaces.nsIDOMWindowUtils) >- .setIsApp(!!appManifestURL); >+ if (!!appManifestURL) { >+ windowUtils.setIsApp(true); >+ windowUtils.setApp(mozApp); windowUtils.setApp(appManifestURL)?

Comment on attachment 624215 [details] [diff] [review] Part 3 - Get the application object from an AppsService based on the manifest URL and save it on the window object Thanks.

Please follow the tree rules when landing on inbound (posting a changeset link, setting target milestone, etc.) https://hg.mozilla.org/integration/mozilla-inbound/rev/dda070f5ce28 https://hg.mozilla.org/integration/mozilla-inbound/rev/f6cd8c9e5f44 https://hg.mozilla.org/integration/mozilla-inbound/rev/4fd8a9d9aef8 https://hg.mozilla.org/integration/mozilla-inbound/rev/bb8b085db9ee https://hg.mozilla.org/mozilla-central/rev/dda070f5ce28 https://hg.mozilla.org/mozilla-central/rev/f6cd8c9e5f44 https://hg.mozilla.org/mozilla-central/rev/4fd8a9d9aef8 https://hg.mozilla.org/mozilla-central/rev/bb8b085db9ee