754141 - nsGlobalWindow should know in which web app it is running

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

This is at least needed for permission management.

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Get the @mozapp value from the parent process instead of its presence.

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Add a SetApp() method that takes the manifest URL value

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - Get the application object from an AppsService based on the manifest URL and save it on the window object

Fabrice, could you review the apps/service part?

Comment 4

[Mounir Lamouri (:mounir)]

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 5

[[:fabrice] Fabrice Desré]

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 6

[Justin Lebar (not reading bugmail)]

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 7

[Justin Lebar (not reading bugmail)]

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.

Comment 8

[Mounir Lamouri (:mounir)]

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).

Comment 9

[Justin Lebar (not reading bugmail)]

> 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 10

[Justin Lebar (not reading bugmail)]

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."

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(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 12

[Justin Lebar (not reading bugmail)]

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.

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Get the @mozapp value from the parent process instead of its presence.

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Add a SetApp() method that takes the manifest URL value

Comment 15

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - Get the application object from an AppsService based on the manifest URL and save it on the window object

Comment 16

[Mounir Lamouri (:mounir)]

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.

Comment 17

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - Get the application object from an AppsService based on the manifest URL and save it on the window object

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

Comment 18

[Justin Lebar (not reading bugmail)]

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 19

[Justin Lebar (not reading bugmail)]

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.

Comment 20

[Mounir Lamouri (:mounir)]

Attachment: Part 4 - Add those files to package-manifest.in

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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