Closed Bug 912340 Opened 6 years ago Closed 6 years ago

Add the manifest's 'role' property to mozIApplication

Categories

(Core Graveyard :: DOM: Apps, defect)

defect
Not set

Tracking

(b2g-v2.0 fixed)

RESOLVED FIXED
mozilla26
Tracking Status
b2g-v2.0 --- fixed

People

(Reporter: fabrice, Assigned: xyuan)

References

Details

(Whiteboard: [FT:System-Platform], [Sprint:4])

Attachments

(1 file, 1 obsolete file)

We need that to check some keyboard-related permissions.
Assignee: nobody → xyuan
Blocks: 901434
Once it is implemented I think we could use that as well for bug 899994
Blocks: 899994
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.
Flags: needinfo?(fabrice)
Whiteboard: [FT:System-Platform], [Sprint:4]
(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.
Flags: needinfo?(fabrice)
Attached patch role.patch v1 (obsolete) — Splinter Review
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.
Attachment #802123 - Flags: review?(fabrice)
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
Attachment #802123 - Flags: review?(fabrice) → feedback+
(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.
Attached patch role.patch v2Splinter Review
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));
Attachment #802123 - Attachment is obsolete: true
Attachment #802378 - Flags: review?(fabrice)
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'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.
(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.
(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?
(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.
Attachment #802378 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/7940f317643d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
No longer blocks: 914541
Blocks: 1000305
Blocks: 1000313
Blocks: 1000315
Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.
Blocks: 1042797
No longer blocks: 1042797
Blocks: 1042807
No longer blocks: 1042807
Blocks: 1092726
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.