912340 - Add the manifest's 'role' property to mozIApplication

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[[:fabrice] Fabrice Desré]

We need that to check some keyboard-related permissions.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Once it is implemented I think we could use that as well for bug 899994

Comment 2

[Ivan Tsay (:ITsay)]

Hi Fabrice,

Is this the blocker for Keyboard/IME API needed for FFOS v1.2? If so, I wonder if it is possible to be implemented and landed before 9/13? I am reviewing all the system platform related engineering works now and need some input about this. Thank you very much.

Comment 3

[[:fabrice] Fabrice Desré]

(In reply to Ivan Tsay (:ITsay) from comment #2)
> Hi Fabrice,
> 
> Is this the blocker for Keyboard/IME API needed for FFOS v1.2? If so, I
> wonder if it is possible to be implemented and landed before 9/13? I am
> reviewing all the system platform related engineering works now and need
> some input about this. Thank you very much.

I'm not 100% sure it's blocking the IME API for 1.2, but it's blocking 3rd party keyboard security checks.

Comment 4

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: role.patch v1

I'm not sure if an app can be assigned one role or more. The patch assumes that each app can have only one role. So it exposes `role` as a string property.

Comment 5

[[:fabrice] Fabrice Desré]

Comment on attachment 802123 [details] [diff] [review]
role.patch v1

Review of attachment 802123 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to make sure we actually need that. Do we have a clear plan to use the roles in gecko?

::: dom/apps/src/AppsUtils.jsm
@@ +646,5 @@
>      return this._origin.resolve(packagePath ? packagePath : "");
> +  },
> +
> +  get role() {
> +    return this._localeProp("role");

We don't want to localize this property, you can just return this._,manifest.role

::: dom/apps/src/Webapps.jsm
@@ +276,5 @@
>      if (supportUseCurrentProfile()) {
>        this._readManifests([{ id: aId }], (function(aResult) {
>          let data = aResult[0];
> +        // Update role.
> +        this.webapps[aId].role = data.manifest.role || "";

remove that line, it does not belong there.
You need one at https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#251 and one at https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#736

Comment 6

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Fabrice Desré [:fabrice] from comment #5)
> 
> I'd like to make sure we actually need that. Do we have a clear plan to use
> the roles in gecko?
I don't see any clear plan until now.

Comment 7

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: role.patch v2

The patch addresses all of comment 5. The diff with previous patch:

diff --git a/dom/apps/src/AppsUtils.jsm b/dom/apps/src/AppsUtils.jsm
--- a/dom/apps/src/AppsUtils.jsm
+++ b/dom/apps/src/AppsUtils.jsm
@@ -642,11 +642,11 @@ ManifestHelper.prototype = {
   },
 
   fullPackagePath: function() {
     let packagePath = this._localeProp("package_path");
     return this._origin.resolve(packagePath ? packagePath : "");
   },
 
   get role() {
-    return this._localeProp("role");
+    return this._manifest.role || "";
   }
 }
diff --git a/dom/apps/src/Webapps.jsm b/dom/apps/src/Webapps.jsm
--- a/dom/apps/src/Webapps.jsm
+++ b/dom/apps/src/Webapps.jsm
@@ -242,23 +242,24 @@ this.DOMApplicationRegistry = {
     this.notifyAppsRegistryStart();
     let ids = [];
     for (let id in this.webapps) {
       ids.push({ id: id });
     }
     if (supportSystemMessages()) {
       this._processManifestForIds(ids, aRunUpdate);
     } else {
-      // Read the CSPs. If MOZ_SYS_MSG is defined this is done on
+      // Read the CSPs and roles. If MOZ_SYS_MSG is defined this is done on
       // _processManifestForIds so as to not reading the manifests
       // twice
       this._readManifests(ids, (function readCSPs(aResults) {
         aResults.forEach(function registerManifest(aResult) {
           let app = this.webapps[aResult.id];
           app.csp = aResult.manifest.csp || "";
+          app.role = aResult.manifest.role || "";
           if (app.appStatus >= Ci.nsIPrincipal.APP_STATUS_PRIVILEGED) {
             app.redirects = this.sanitizeRedirects(aResult.redirects);
           }
         }, this);
       }).bind(this));
 
       // Nothing else to do but notifying we're ready.
       this.notifyAppsRegistryReady();
@@ -271,18 +272,16 @@ this.DOMApplicationRegistry = {
     }
 
     // Install the permissions for this app, as if we were updating
     // to cleanup the old ones if needed.
     // TODO It's not clear what this should do when there are multiple profiles.
     if (supportUseCurrentProfile()) {
       this._readManifests([{ id: aId }], (function(aResult) {
         let data = aResult[0];
-        // Update role.
-        this.webapps[aId].role = data.manifest.role || "";
         PermissionsInstaller.installPermissions({
           manifest: data.manifest,
           manifestURL: this.webapps[aId].manifestURL,
           origin: this.webapps[aId].origin
         }, true, function() {
           debug("Error installing permissions for " + aId);
         });
       }).bind(this));
@@ -736,16 +735,17 @@ this.DOMApplicationRegistry = {
   _processManifestForIds: function(aIds, aRunUpdate) {
     this._readManifests(aIds, (function registerManifests(aResults) {
       let appsToRegister = [];
       aResults.forEach(function registerManifest(aResult) {
         let app = this.webapps[aResult.id];
         let manifest = aResult.manifest;
         app.name = manifest.name;
         app.csp = manifest.csp || "";
+        app.role = manifest.role || "";
         if (app.appStatus >= Ci.nsIPrincipal.APP_STATUS_PRIVILEGED) {
           app.redirects = this.sanitizeRedirects(manifest.redirects);
         }
         this._registerSystemMessages(manifest, app);
         appsToRegister.push({ manifest: manifest, app: app });
       }, this);
       this._registerActivitiesForApps(appsToRegister, aRunUpdate);
     }).bind(this));

Comment 8

[Marco Castelluccio [:marco]]

Fabrice, can't we make this just a manifest property? If we're worried about performance issues, the users of this property could just cache its value.

Comment 9

[[:fabrice] Fabrice Desré]

I'm not even sure we are gonna use it, but if we need to access it from c++ we have no other choice than to add it to mozIDOMApplication.

Comment 10

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Marco Castelluccio [:marco] from comment #8)
> Fabrice, can't we make this just a manifest property? If we're worried about
> performance issues, the users of this property could just cache its value.

I believe the implementation won't degrade the performance.

The `role` is useful for defining special kinds of app and gives Firefox OS the capability of building a service for keyboard, lockscreen and so on.

I'm in favor of this property.

Comment 11

[Marco Castelluccio [:marco]]

(In reply to Yuan Xulei [:yxl] from comment #10)
> (In reply to Marco Castelluccio [:marco] from comment #8)
> > Fabrice, can't we make this just a manifest property? If we're worried about
> > performance issues, the users of this property could just cache its value.
> 
> I believe the implementation won't degrade the performance.

I was talking about my alternative idea, that could cause performance issues compared to your solution.

> The `role` is useful for defining special kinds of app and gives Firefox OS
> the capability of building a service for keyboard, lockscreen and so on.
> 
> I'm in favor of this property.

I'm not against this manifest property, I just don't realize why it is needed in mozIApplication. If it's only necessary to check some permissions, we could just have it in the manifest, couldn't we?

Comment 12

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Marco Castelluccio [:marco] from comment #11)
> I'm not against this manifest property, I just don't realize why it is
> needed in mozIApplication. If it's only necessary to check some permissions,
> we could just have it in the manifest, couldn't we?

For the implementation of Keyboard/IME API with JavaScript, I don't need role in
mozIApplication. So it's OK for me not to include `role` in mozIApplication.

Comment 13

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/integration/b2g-inbound/rev/7940f317643d

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7940f317643d

Comment 15

[Kevin Grandon :kgrandon]

Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.