Closed
Bug 912340
Opened 11 years ago
Closed 11 years ago
Add the manifest's 'role' property to mozIApplication
Categories
(Core Graveyard :: DOM: Apps, defect)
Core Graveyard
DOM: Apps
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)
7.20 KB,
patch
|
fabrice
:
review+
|
Details | Diff | Splinter Review |
We need that to check some keyboard-related permissions.
Comment 1•11 years ago
|
||
Once it is implemented I think we could use that as well for bug 899994
Blocks: 899994
Blocks: 819882
Comment 2•11 years ago
|
||
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.
Updated•11 years ago
|
Flags: needinfo?(fabrice)
Whiteboard: [FT:System-Platform], [Sprint:4]
Reporter | ||
Comment 3•11 years ago
|
||
(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)
Assignee | ||
Comment 4•11 years ago
|
||
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)
Reporter | ||
Comment 5•11 years ago
|
||
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+
Assignee | ||
Comment 6•11 years ago
|
||
(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.
Assignee | ||
Comment 7•11 years ago
|
||
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)
Comment 8•11 years ago
|
||
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.
Reporter | ||
Comment 9•11 years ago
|
||
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.
Assignee | ||
Comment 10•11 years ago
|
||
(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•11 years ago
|
||
(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?
Assignee | ||
Comment 12•11 years ago
|
||
(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.
Reporter | ||
Updated•11 years ago
|
Attachment #802378 -
Flags: review?(fabrice) → review+
Comment 14•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7940f317643d
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment 15•10 years ago
|
||
Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.
status-b2g-v2.0:
--- → fixed
Updated•7 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•