nsGlobalWindow should know in which web app it is running

RESOLVED FIXED in mozilla15

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Blocks: 1 bug)

Trunk
mozilla15
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(4 attachments, 4 obsolete attachments)

3.00 KB, patch
Justin Lebar (not reading bugmail)
: review+
Details | Diff | Splinter Review
5.09 KB, patch
Justin Lebar (not reading bugmail)
: review+
Details | Diff | Splinter Review
11.96 KB, patch
Justin Lebar (not reading bugmail)
: review+
Details | Diff | Splinter Review
2.39 KB, patch
vingtetun
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
This is at least needed for permission management.
(Assignee)

Comment 1

5 years ago
Created attachment 623312 [details] [diff] [review]
Part 1 - Get the @mozapp value from the parent process instead of its presence.
Attachment #623312 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 2

5 years ago
Created attachment 623313 [details] [diff] [review]
Part 2 - Add a SetApp() method that takes the manifest URL value
Attachment #623313 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 3

5 years ago
Created 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

Fabrice, could you review the apps/service part?
Attachment #623314 - Flags: review?(fabrice)
(Assignee)

Comment 4

5 years ago
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?
Attachment #623314 - Flags: review?(justin.lebar+bug)
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
Attachment #623314 - Flags: review?(fabrice) → review+
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.
Attachment #623312 - Flags: review?(justin.lebar+bug) → review-
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.
Attachment #623313 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 8

5 years ago
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."
Attachment #623314 - Flags: review?(justin.lebar+bug) → review+
(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.
Attachment #623313 - Flags: review+ → review-
Attachment #623314 - Flags: review+ → review-
(Assignee)

Comment 13

5 years ago
Created attachment 624211 [details] [diff] [review]
Part 1 - Get the @mozapp value from the parent process instead of its presence.
Attachment #623312 - Attachment is obsolete: true
Attachment #624211 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 14

5 years ago
Created attachment 624212 [details] [diff] [review]
Part 2 - Add a SetApp() method that takes the manifest URL value
Attachment #623313 - Attachment is obsolete: true
Attachment #624212 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 15

5 years ago
Created attachment 624213 [details] [diff] [review]
Part 3 - Get the application object from an AppsService based on the manifest URL and save it on the window object
Attachment #623314 - Attachment is obsolete: true
Attachment #624213 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 16

5 years ago
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.
(Assignee)

Comment 17

5 years ago
Created 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

Better if the patch is typo-less, right? :)
Attachment #624213 - Attachment is obsolete: true
Attachment #624213 - Flags: review?(justin.lebar+bug)
Attachment #624215 - Flags: review?(justin.lebar+bug)
Attachment #624211 - Flags: review?(justin.lebar+bug) → review+
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)?
Attachment #624212 - Flags: review?(justin.lebar+bug) → review+
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.
Attachment #624215 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 20

5 years ago
Created attachment 624328 [details] [diff] [review]
Part 4 - Add those files to package-manifest.in
Attachment #624328 - Flags: review?(21)
Attachment #624328 - Flags: review?(21) → review+
(Assignee)

Updated

5 years ago
Target Milestone: --- → mozilla15
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Blocks: 756499
Blocks: 756644
No longer blocks: 756644
Blocks: 756644
Depends on: 757415

Updated

5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.